Re: [PATCH v3 01/14] x86/msr: Move rdtsc{,_ordered}() to <asm/tsc.h>

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 4341 bytes --]

On Sat, 26 Apr 2025, Xin Li wrote:

> On 4/25/2025 8:45 AM, Ilpo Järvinen wrote:
> > To me this looks really a random set of source files, maybe it helped some
> > build success but it's hard for me to review this because there are still
> > cases that depend on indirect include chains.
> > 
> > Could you just look into solving all missing msr.h includes instead
> > as clearly some are still missing after 3 pre-existing ones and you adding
> > it into 3 files:
> > 
> > $ git grep -e rdmsr -e wrmsr -l drivers/platform/x86/
> > drivers/platform/x86/intel/ifs/core.c
> > drivers/platform/x86/intel/ifs/load.c
> > drivers/platform/x86/intel/ifs/runtest.c
> > drivers/platform/x86/intel/pmc/cnp.c
> > drivers/platform/x86/intel/pmc/core.c
> > drivers/platform/x86/intel/speed_select_if/isst_if_common.c
> > drivers/platform/x86/intel/speed_select_if/isst_if_mbox_msr.c
> > drivers/platform/x86/intel/speed_select_if/isst_tpmi_core.c
> > drivers/platform/x86/intel/tpmi_power_domains.c
> > drivers/platform/x86/intel/turbo_max_3.c
> > drivers/platform/x86/intel/uncore-frequency/uncore-frequency.c
> > drivers/platform/x86/intel_ips.c
> > 
> > $ git grep -e 'msr.h' -l drivers/platform/x86/
> > drivers/platform/x86/intel/pmc/core.c
> > drivers/platform/x86/intel/tpmi_power_domains.c
> > drivers/platform/x86/intel_ips.c
> 
> I think you want me to add all necessary direct inclusions, right?

For asm/msr.h yes, as it seems you're altering the inclusion paths and all 
non-direct includes have a chance of breaking so it seems prudent to just 
convert them into direct includes.

> This is the right thing to do, and I did try it but gave up later.
> 
> I will do it in the next iteration as you asked.  But I want to make my
> points:
> 
> 1) It's not just two patterns {rd,wr}msr, there are a lot of definitions
>    in <asm/msr.h> and we need to cover all of them:

I know and I don't expect you to get it 100% perfect, but taking a major 
step into the right direction would be way better than build testing one 
configuration and see what blows up and fix only those.

In this particular case, the amount of includes seemed really subpar with 
many users lacking the include.

>       struct msr_info
>       struct msr_regs_info
>       struct saved_msr
>       struct saved_msrs

Could be shortened to -e 'struct msr' -e 'struct saved_msr'.

>       {read,write}_msr
>       rdpmc
>       .*msr.*_on_cpu

Well, my pattern already caught rdmsr.*on_cpu and wrmsr.*on_cpu.

For the other patterns, I don't see those at all under 
drivers/platform/x86/ but I think when one typically implies the 
others tend appear as well so this might not be as hard as it seems.

> 2) Once all necessary direct inclusions are in place, it's easy to
>    overlook adding a header inclusion in practice, especially if the
>    build passes.  Besides we often forget to remove a header when a
>    definition is removed.  In other words, direct inclusion is hard to
>    maintain.

This is true as well but we should still try to move towards the right 
state affairs even if we will not get it near 100% until there's a real 
tool that relentlessly keeps exposing such human oversight.

And do I try to check also includes whenever I remember while reviewing 
patches (which requires some effort as they are not visible in the code 
context and might not appear in a patch at all).

> 3) Some random kernel configuration combinations can cause the current
>    kernel build to fail.  I hit one in x86 UML.

Yes, which why direct including is much better than relying on fragile 
indirects.

> We all know Ingo is the best person to discuss this with :).  While my
> understanding of the header inclusion issue may be inaccurate or
> outdated.
> 
> So for me, using "make allyesconfig" is a practical method for a quick
> local build check, plus I always send my patches to Intel LKP.

Even with LKP, randconfig builds may still require many tests to find 
issues.

> There probably wants a script that identifies all files that reference a
> definition in a header thus need to include it explicitly.  And indirect
> includes should be zapped.

Sadly, the clang based include-what-you-use tool is not yet there for 
the kernel AFAIK.

-- 
 i.