Re: [PATCH v8 7/8] x86/mm/ibs: In-kernel driver for AMD IBS Memory Profiler

[Reply to Shashiko review]

On 28-Jul-26 11:13 AM, Bharata B Rao wrote:
> diff --git a/arch/x86/mm/ibs-mprof.c b/arch/x86/mm/ibs-mprof.c
> new file mode 100644
> index 000000000000..923fb8f99552
> --- /dev/null
> +++ b/arch/x86/mm/ibs-mprof.c
> +
> +static bool mprof_pop_sample(struct mprof_sample_pcpu *pcpu, struct mprof_sample *s)
> +{
> +	int tail = READ_ONCE(pcpu->tail);
> +	/*
> +	 * Pairs with the smp_store_release() of head in mprof_push_sample();
> +	 * ensures the sample slot stores are visible before we read the slot.
> +	 */
> +	int head = smp_load_acquire(&pcpu->head);
> +	int next = tail + 1;
> +
> +	if (head == tail)
> +		return false;
> +
> +	if (next >= IBS_NR_SAMPLES)
> +		next = 0;
> +
> +	*s = pcpu->samples[tail];
> +
> +	WRITE_ONCE(pcpu->tail, next);
> +	return true;
> +}

> Could the compiler reorder the non-volatile struct assignment of the sample
> after the volatile store to tail?
>
> If a hardware interrupt preempts the worker thread exactly after the tail
> update is published but before the sample copy is complete, the interrupt
> handler could observe the advanced tail and overwrite the sample data being
> read.
>
> Would it be safer to use smp_store_release(&pcpu->tail, next) here to ensure
> memory reads are strictly ordered before the tail update is published?

In this sample buffer SPSC ring, there is always one empty slot which keeps the
producer at least one slot behind the consumer. So writes to the same slot that
is being read is not possible even if the tail update is observed early.

Having said that, making the tail publish an explicit release is clearer and
symmetric with the head handling. Hence will update this tail publish using
smp_store_release().

> +static void mprof_overflow_handler(void)
> +{
> +	u64 mem_ctl, mem_data3, mem_data2, paddr, data_src;
> +	struct work_struct *w = &this_cpu_ptr(&mprof_work)->work;
> +	unsigned long pfn;
> +	struct page *page;
> +
> +	rdmsrq(MSR_AMD64_IBS_MPROF_CTL, mem_ctl);
> +	if (!(mem_ctl & IBS_MPROF_CTL_VAL))
> +		return;
> +
> +	mprof_disable_profiling(mem_ctl);
> +	count_vm_event(HWHINT_TOTAL_EVENTS);
> +
> +	rdmsrq(MSR_AMD64_IBS_MPROF_DATA3, mem_data3);
> +	rdmsrq(MSR_AMD64_IBS_MPROF_DATA2, mem_data2);
> +
> +	data_src = mem_data2 & IBS_MPROF_DATA2_DATASRC_MASK;
> +	data_src |= ((mem_data2 & IBS_MPROF_DATA2_DATASRC_MASK_HIGH) >>
> +			IBS_MPROF_DATA2_DATASRC_MASK_HIGH_SHIFT);
> +
> +	switch (data_src) {
> +	case IBS_MPROF_DATA2_DATASRC_DRAM:
> +		count_vm_event(HWHINT_DRAM_ACCESSES);
> +		break;
> +	case IBS_MPROF_DATA2_DATASRC_EXT_MEM:
> +		count_vm_event(HWHINT_EXTMEM_ACCESSES);
> +		break;
> +	}
> +

> Does this switch intentionally omit the IBS_MPROF_DATA2_DATASRC_DRAM_FAR case?
> It appears that if remote DRAM accesses occur, they are currently silently
> dropped from the statistics rather than being counted under
> HWHINT_DRAM_ACCESSES.

No it was an oversight, remote DRAM access is still a DRAM access. It is just a
stats reporting mismatch and promotion behaviour isn't affected. Will include
remote DRAM access into HWHINT_DRAM_ACCESSES counter.

> +
> +static int x86_amd_ibs_mprof_teardown(unsigned int cpu)
> +{
> +	int offset = get_mprof_lvt_offset();
> +	u64 mem_ctl;
> +
> +	if (offset >= 0)
> +		setup_APIC_eilvt(offset, IBS_MEMPROF_VECTOR, APIC_DELIVERY_MODE_FIXED, 1);
> +
> +	rdmsrq(MSR_AMD64_IBS_MPROF_CTL, mem_ctl);
> +	mprof_disable_profiling(mem_ctl);
> +
> +	/*
> +	 * The producer is now silenced and this CPU's worker is gone. Drop
> +	 * any unconsumed samples (see mprof_drain_cpu) and clear the pending
> +	 * flag so a subsequent re-online of this CPU starts from a clean
> +	 * state.
> +	 */
> +	mprof_drain_cpu(cpu);
> +	per_cpu(mprof_work_pending, cpu) = false;
> +
> +	return 0;
> +}
> +

> Is the assumption that "this CPU's worker is gone" always true here?
>
> When a CPU goes offline, its per-CPU workqueues are unbound during the
> CPUHP_AP_WORKQUEUE_ONLINE phase, meaning pending work items like
> mprof_work_handler can migrate to another CPU and continue executing.
>
> Because x86_amd_ibs_mprof_teardown runs later in the STARTING phase,
> mprof_drain_cpu() modifies the ring buffer unsynchronized. If the migrated
> worker is concurrently popping a sample, this could happen:
>
> Worker CPU (migrated):
> mprof_pop_sample() reads tail
>
> Dying CPU:
> mprof_drain_cpu() zeroes head and tail
>
> Worker CPU (migrated):
> mprof_pop_sample() updates tail = next
>
> Could this desync the tail pointer, potentially causing the worker loop to
> read up to 149 uninitialized or stale samples and falsely report them to
> pghot_record_access?
>
> Would it be safer to register the teardown at an ONLINE-level hotplug state
> so it can use cancel_work_sync() after disabling profiling?

That comment/assumption was inaccurate: after the workqueue unbind at offline,
the worker can still be running on another CPU, so mprof_drain_cpu()'s ring
reset could race it.

The migrated worker operates on its own ring buffer, so it is a single consumer
which completely drains the ring on its own. The teardown reset was therefore
both redundant and the only thing racing the worker. I will drop it in the next
version.

Regards,
Bharata.