Re: [PATCH v3 10/26] fs/resctrl: Improve handling for events that can be read from any CPU

[Reinette Chatre <reinette.chatre@intel.com>]

Hi Tony,

On 4/22/25 5:51 PM, Luck, Tony wrote:
>>>> cpumask_any() is just cpumask_first() so it will pick the first CPU in the
>>>> online mask that may not be the current CPU.
>>>>
>>>> fwiw ... there are some optimizations planned in this area that I have not yet studied:
>>>> https://lore.kernel.org/lkml/20250407153856.133093-1-yury.norov@gmail.com/
>>>
>>> I remember Peter complaining[1] about extra context switches when
>>> cpumask_any_housekeeping() was introduced, but it seems that the
>>> discussion died with no fix applied.
>>
>> The initial complaint was indeed that reading individual events is slower.
>>
>> The issue is that the intended use case read from many files at frequent
>> intervals and thus becomes vulnerable to any changes in this area that
>> really is already a slow path (reading from a file ... taking a mutex ...).
>>
>> Instead of working on shaving cycles off this path the discussion transitioned
>> to resctrl providing better support for the underlying use case. I
>> understood that this is being experimented with [2] and last I heard it
>> looks promising.
>>
>>>
>>> The blocking problem is that ARM may not be able to read a counter
>>> on a tick_nohz CPU because it may need to sleep.
>>>
>>> Do we need more options for events:
>>>
>>> 1) Must be read on a CPU in the right domain        // Legacy
>>> 2) Can be read from any CPU                 // My addtion
>>> 3) Must be read on a "housekeeping" CPU             // James' code in upstream
>>> 4) Cannot be read on a tick_nohz CPU                // Could be combined with 1 or 2?
>>
>> I do not see needing additional complexity here. I think it will be simpler
>> to just replace use of cpumask_any_housekeeping() in mon_event_read() with
>> open code that supports the particular usage. As I understand it is prohibited
>> for all CPUs to be in tick_nohz_full_mask so it looks to me as though the
>> existing "if (tick_nohz_full_cpu(cpu))" should never be true (since no CPU is being excluded).
>> Also, since mon_event_read() has no need to exclude CPUs, just a cpumask_andnot()
>> should suffice to determine what remains of given mask after accounting for all the
>> NO_HZ CPUs if tick_nohz_full_enabled().
> 
> Maybe there isn’t much complexity to make this "read one counter" better on systems
> where reading from any CPU is possible. Taking your advice from the earlier review
> the filesystem code can set a flag in the mon_evt structure. struct mon_data and
> struct rmid_read can change from holding the event id to holding a pointer to the
> mon_evt (as the source of truth).
> 
> Then mon_event_read() can just have a simple direct call to mon_event_count()
> just before the call to cpumask_any_housekeeping() like this:
> 
> 		if (evt->any_cpu) {
> 			mon_event_count(rr);
> 			goto done;
> 		}
> 
> The "goto done" jumps to the resctrl_arch_mon_ctx_free(r, evt->evtid, rr->arch_mon_ctx);
> at the end of mon_event_read()

Thanks, this looks great.

> 
> Folks can still pursue the bulk read of many counters (though I expect you might want
> one file per domain, rather than a single file to report everything).

I will have to re-read that thread to refresh myself on the discussion but scanning the thread
I did find a summary of points (end of [3]) and this was there.

> 
>>
>> Reinette
>>
>>>
>>>> Reinette
>>>
>>> [1] https://lore.kernel.org/all/20241031142553.3963058-2-peternewman@google.com/
>>>>
>>
>> [2] https://lore.kernel.org/lkml/CALPaoCgpnVORZfbKVLXDFUZvv8jhpShHPzB3cwdLTZQH1o9ULw@mail.gmail.com/
> 

[3] https://lore.kernel.org/lkml/34fd8713-3430-4e27-a2c2-fd8839f90f5a@intel.com/

Reinette