Re: [PATCH v8 8/8] x86/mm/ibs: Add runtime controls for IBS memprofiler

[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
> index 923fb8f99552..07e0516db2ee 100644
> --- a/arch/x86/mm/ibs-mprof.c
> +++ b/arch/x86/mm/ibs-mprof.c
>  /*
>   * Record the IBS-reported access sample in percpu buffer.
>   * Called from IBS interrupt handler.
> @@ -159,28 +199,55 @@ static inline void mprof_drain_cpu(unsigned int cpu)

> Does this code safely handle CPU hotplug teardown?
> During a CPU hotplug offline event, the high-priority stop_machine thread can
> preempt the mprof_work_handler worker mid-execution (for example, after
> reading head and tail).
> x86_amd_ibs_mprof_teardown() then locklessly zeroes head and tail via
> mprof_drain_cpu().
> When the CPU goes offline, the preempted worker thread is migrated to another
> CPU and resumes. Could it compute and write back its old tail value,
> desynchronizing tail from the newly reset head?
> This might cause the ring buffer to appear full of old or uninitialized
> samples, which are then passed to pghot_record_access(), potentially leading
> to wild page promotions or memory corruption in the pghot subsystem.

This comment is same as the one provided in 7/8 which has been replied-to there.

> +/*
> + * Publish a new config snapshot and push it to every online CPU
> + * immediately. Must be called with mprof_cfg_lock held.
> + */
> +static void mprof_publish(const struct mprof_config *newcfg)
> +{
> +	struct mprof_config *slot;
> +
> +	lockdep_assert_held(&mprof_cfg_lock);
> +
> +	/* Fill the slot that is not currently published, then flip to it. */
> +	slot = (mprof_cfg == &mprof_cfg_slots[0]) ?
> +		&mprof_cfg_slots[1] : &mprof_cfg_slots[0];
> +	*slot = *newcfg;

> Can this struct copy cause a data race with sysfs and debugfs readers?
>
> Because sysfs and debugfs attribute readers run in standard preemptible
> process context without locking, a reader task (like enabled_show) can execute
> smp_load_acquire() to get a pointer to the active slot and then be preempted.
>
> If a concurrent writer updates the configuration twice, it flips the active
> pointer back to the reused slot and directly overwrites its contents here via
> a plain struct copy.
>
> When the preempted reader resumes, it performs a plain C read concurrently
> with this struct assignment, which might result in a data race and torn reads
> of configuration fields.

Will fix this by adding mutex protection in the read side.


> +	mprof_compose(slot);
> +	/* Publish the fully composed slot; pairs with smp_load_acquire() in readers. */
> +	smp_store_release(&mprof_cfg, slot);
> +
> +	/*
> +	 * on_each_cpu() with wait serializes against any in-flight interrupt
> +	 * handler on each CPU, so the previously published slot has no readers
> +	 * once this returns and can be safely reused by the next writer.
> +	 */
> +	on_each_cpu(mprof_reprogram_this_cpu, NULL, 1);

> Does this unconditionally arm the hardware profiler on all CPUs?
>
> In x86_amd_ibs_mprof_startup(), if setup_APIC_eilvt() fails to configure the
> APIC interrupt vector, the function logs a warning and returns 0, allowing the
> CPU to successfully come online without configuring its APIC routing for IBS.
>
> If the user enables the profiler via sysfs, this on_each_cpu() call executes
> mprof_enable_profiling() on all online CPUs, including any that failed setup.
>
> When the hardware sample overflows, will the CPU deliver the interrupt using
> the previously configured (or uninitialized) EILVT vector, potentially
> resulting in spurious interrupts delivered to other subsystems or a system
> panic?

I will prevent this situation (where failure from EILVT setup is observed from
online handler) by validating the EILVT offset at init time like how primary IBS
does.

> +static ssize_t enabled_show(struct device *dev,
> +			    struct device_attribute *attr, char *buf)
> +{
> +	/* Acquire the snapshot; pairs with smp_store_release() in the writers. */
> +	struct mprof_config *cfg = smp_load_acquire(&mprof_cfg);
> +
> +	return sysfs_emit(buf, "%s\n", str_enabled_disabled(cfg->enabled));
> +}
> +
> +static ssize_t enabled_store(struct device *dev, struct device_attribute *attr,
> +			     const char *buf, size_t count)
> +{
> +	struct mprof_config new;
> +	bool enabled;
> +	int ret;
> +
> +	ret = kstrtobool(buf, &enabled);

> Does this break standard sysfs read-modify-write interactions?
>
> The enabled_show() function outputs the state using str_enabled_disabled(),
> which prints "enabled\n" or "disabled\n".
>
> However, kstrtobool() does not recognize "enabled" or "disabled" as valid
> boolean strings. As a result, standard boolean flag interactions like
> echo $(cat enabled) > enabled will fail with -EINVAL.

Doesn't look like.

kstrtobool() matches the first character, with the accepted set 'EeYyTt1DdNnFf0'
plus "on"/"off". So enabled with pass. Same with disabled.

Regards,
Bharata.