Re: [PATCH v7 45/49] x86,fs/resctrl: Move the resctrl filesystem code to live in /fs/resctrl

[James Morse <james.morse@arm.com>]

Hi Fenghua,

On 06/03/2025 20:35, Fenghua Yu wrote:
> On 2/28/25 11:59, James Morse wrote:
>> Resctrl is a filesystem interface to hardware that provides cache
>> allocation policy and bandwidth control for groups of tasks or CPUs.
>>
>> To support more than one architecture, resctrl needs to live in /fs/.
>>
>> Move the code that is concerned with the filesystem interface to
>> /fs/resctrl.

>>   14 files changed, 7535 insertions(+), 7285 deletions(-)

(this patch is large - please trim it in your reply!)

[...]


>> diff --git a/arch/x86/kernel/cpu/resctrl/monitor_trace.h b/arch/x86/kernel/cpu/resctrl/
>> monitor_trace.h
>> index ade67daf42c2..b5a142dd0f0e 100644
>> --- a/arch/x86/kernel/cpu/resctrl/monitor_trace.h
>> +++ b/arch/x86/kernel/cpu/resctrl/monitor_trace.h
>> @@ -7,2m5 +7,11 @@
>>     #include <linux/tracepoint.h>
>>   -TRACE_EVENT(mon_llc_occupancy_limbo,
>> -        TP_PROTO(u32 ctrl_hw_id, u32 mon_hw_id, int domain_id, u64 llc_occupancy_bytes),
>> -        TP_ARGS(ctrl_hw_id, mon_hw_id, domain_id, llc_occupancy_bytes),
>> -        TP_STRUCT__entry(__field(u32, ctrl_hw_id)
>> -                 __field(u32, mon_hw_id)
>> -                 __field(int, domain_id)
>> -                 __field(u64, llc_occupancy_bytes)),
>> -        TP_fast_assign(__entry->ctrl_hw_id = ctrl_hw_id;
>> -               __entry->mon_hw_id = mon_hw_id;
>> -               __entry->domain_id = domain_id;
>> -               __entry->llc_occupancy_bytes = llc_occupancy_bytes;),
>> -        TP_printk("ctrl_hw_id=%u mon_hw_id=%u domain_id=%d llc_occupancy_bytes=%llu",
>> -              __entry->ctrl_hw_id, __entry->mon_hw_id, __entry->domain_id,
>> -              __entry->llc_occupancy_bytes)
>> -       );
>> -
>>   #endif /* _FS_RESCTRL_MONITOR_TRACE_H */
>>     #undef TRACE_INCLUDE_PATH
>>   #define TRACE_INCLUDE_PATH .
>> +
>>   #define TRACE_INCLUDE_FILE monitor_trace
>> +
>>   #include <trace/define_trace.h>

> Similarly this file doesn't define any trace event. make W=1 complains it:
> 
>                 from arch/x86/kernel/cpu/resctrl/monitor.c:32:
> ./include/trace/stages/init.h:2:23: error: ‘str__resctrl__trace_system_name’ defined but
> not used [-Werror=unused-const-variable=]
>     2 | #define __app__(x, y) str__##x##y
>       |                       ^~~~~
> ./include/trace/stages/init.h:3:21: note: in expansion of macro ‘__app__’
>     3 | #define __app(x, y) __app__(x, y)
>       |                     ^~~~~~~
> ./include/trace/stages/init.h:5:29: note: in expansion of macro ‘__app’
>     5 | #define TRACE_SYSTEM_STRING __app(TRACE_SYSTEM_VAR,__trace_system_name)
>       |                             ^~~~~
> ./include/trace/stages/init.h:8:27: note: in expansion of macro ‘TRACE_SYSTEM_STRING’
>     8 |         static const char TRACE_SYSTEM_STRING[] =       \
>       |                           ^~~~~~~~~~~~~~~~~~~
> ./include/trace/stages/init.h:11:1: note: in expansion of macro ‘TRACE_MAKE_SYSTEM_STR’
>    11 | TRACE_MAKE_SYSTEM_STR();
>       | ^~~~~~~~~~~~~~~~~~~~~
>   CC      fs/proc/consoles.o
> cc1: all warnings being treated as errors
> make[6]: *** [scripts/Makefile.build:207: arch/x86/kernel/cpu/resctrl/monitor.o] Error 1
> make[6]: *** Waiting for unfinished jobs....


> The above two compilation errors only happen when first 45 patches are applied. The whole
> patch set won't have the errors because patch 46 removes these empty trace events.
> 
> The fix is simple: just merge this patch and patch 46 together. Then there is no empty
> trace event in trace.h files and thus no compilation errors.

The intent is that these last few patches are merged together by the person that applies
them. I thought I'd described that in the cover-letter, but that text got lost.

It's like this because this patch is too large to review - but it is generated by a
script. If I merge these patches together, then its even harder to have confidence that
some hunk isn't getting reverted to an older version.


Thanks,

James