Hi Dave,
On 05/09/2025 17:40, Dave Martin wrote:
> On Fri, Aug 22, 2025 at 03:29:55PM +0000, James Morse wrote:
>> Because an MSC can only by accessed from the CPUs in its cpu-affinity
>> set we need to be running on one of those CPUs to probe the MSC
>> hardware.
>>
>> Do this work in the cpuhp callback. Probing the hardware will only
>> happen before MPAM is enabled, walk all the MSCs and probe those we can
>> reach that haven't already been probed.
>
> It may be worth mentioning that the low-level MSC register accessors
> are added by this patch.
Sure,
>> Later once MPAM is enabled, this cpuhp callback will be replaced by
>> one that avoids the global list.
>
> I misread this is as meaning "later in the patch series" and got
> confused.
>
> Perhaps, something like the following? (though this got a bit verbose)
>
> --8<--
>
> Once all MSCs reported by the firmware have been probed from a CPU in
> their respective cpu-affinity set, the probe-time cpuhp callbacks are
> replaced. The replacement callbacks will ultimately need to handle
> save/restore of the runtime MSC state across power transitions, but for
> now there is nothing to do in them: so do nothing.
>
> -->8--
Done.
>> Enabling a static key will also take the cpuhp lock, so can't be done
>
> What static key?
The one that enables the architectures context-switch code. That was added by an
earlier patch, but got moved later to reduce the number of trees that this series touches.
(also, there is no point having the context switch code if you can't have different values
until the resctrl code shows up.)
This is trying to describe why mpam_enable() is scheduled, instead of just being called in
cpuhp context. (again - to reduce the churn caused by changing that later).
I'll rephrase it as:
| The architecture's context switch code will be enabled by a static-key, this can be set
| by mpam_enable(), but must be done from process context, not a cpuhp callback because
| both take the cpuhp lock.
| Whenever a new MSC has been probed, the mpam_enable() work is scheduled to test if all
| the MSCs have been probed.
> None in this patch that I can see.
>
>> from the cpuhp callback. Whenever a new MSC has been probed schedule
>> work to test if all the MSCs have now been probed.
>> diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c
>
> [...]
>
>> @@ -511,9 +539,84 @@ int mpam_ris_create(struct mpam_msc *msc, u8 ris_idx,
>
> [...]
>
>> +static int mpam_cpu_online(unsigned int cpu)
>> {
>> - pr_err("Discovered all MSC\n");
>
> I guess this disappears later?
>
> If we print anything, it feels like it should be in the
> mpam_enable_once() path, otherwise it looks like dmesg is going to get
> spammed on every hotplug. I might have missed something, here.
Yes - there is a print that happens on the mpam_enable_once() path that shows the number
of PARTID/PMG discovered. That "Discovered all MSC" message was so that probing had this
shape from the very beginning - it is unfortunately not as simple as probing a stand-alone
driver.
>> + return 0;
>> +}
>> +
>> +/* Before mpam is enabled, try to probe new MSC */
>> +static int mpam_discovery_cpu_online(unsigned int cpu)
>> +{
>> + int err = 0;
>> + struct mpam_msc *msc;
>> + bool new_device_probed = false;
>> +
>> + mutex_lock(&mpam_list_lock);
> I take it nothing breaks if we sleep here?
