[tip:x86/cache 9/13] arch/x86/kernel/cpu/resctrl/rdtgroup.c:1456:6: warning: variable 'h' set but not used

[tip:x86/cache 9/13] arch/x86/kernel/cpu/resctrl/rdtgroup.c:1456:6: warning: variable 'h' set but not used

[kernel test robot]

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git x86/cache
head:   0a363fb23ee2f7beb08437ad7db86d195878d79f
commit: dc2a3e857981f859889933cf66ded117d74edff1 [9/13] x86/resctrl: Add interface to read mbm_total_bytes_config
config: x86_64-randconfig-a013-20230123 (https://download.01.org/0day-ci/archive/20230124/202301242015.kbzkVteJ-lkp@intel.com/config)
compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project f28c006a5895fc0e329fe15fead81e37457cb1d1)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git/commit/?id=dc2a3e857981f859889933cf66ded117d74edff1
        git remote add tip https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git
        git fetch --no-tags tip x86/cache
        git checkout dc2a3e857981f859889933cf66ded117d74edff1
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash arch/x86/kernel/cpu/resctrl/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> arch/x86/kernel/cpu/resctrl/rdtgroup.c:1456:6: warning: variable 'h' set but not used [-Wunused-but-set-variable]
           u32 h;
               ^
   1 warning generated.


vim +/h +1456 arch/x86/kernel/cpu/resctrl/rdtgroup.c

  1451	
  1452	static void mon_event_config_read(void *info)
  1453	{
  1454		struct mon_config_info *mon_info = info;
  1455		unsigned int index;
> 1456		u32 h;
  1457	
  1458		index = mon_event_config_index_get(mon_info->evtid);
  1459		if (index == INVALID_CONFIG_INDEX) {
  1460			pr_warn_once("Invalid event id %d\n", mon_info->evtid);
  1461			return;
  1462		}
  1463		rdmsr(MSR_IA32_EVT_CFG_BASE + index, mon_info->mon_config, h);
  1464	
  1465		/* Report only the valid event configuration bits */
  1466		mon_info->mon_config &= MAX_EVT_CONFIG_BITS;
  1467	}
  1468	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

Re: [tip:x86/cache 9/13] arch/x86/kernel/cpu/resctrl/rdtgroup.c:1456:6: warning: variable 'h' set but not used

[Borislav Petkov]

On Tue, Jan 24, 2023 at 08:37:14PM +0800, kernel test robot wrote:
> tree:   https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git x86/cache
> head:   0a363fb23ee2f7beb08437ad7db86d195878d79f
> commit: dc2a3e857981f859889933cf66ded117d74edff1 [9/13] x86/resctrl: Add interface to read mbm_total_bytes_config

These patches have been around for a long while now. How come you test them just
now, after I take them?

> >> arch/x86/kernel/cpu/resctrl/rdtgroup.c:1456:6: warning: variable 'h' set but not used [-Wunused-but-set-variable]
>            u32 h;
>                ^

The fix is simple - use rsmsrl(), see below.

If it weren't that simple I'd simply ignore this warning altogether. Yes, it is
unused because the damn high MSR value needs to go somewhere, for chrissakes.
And if it didn't need that masking with MAX_EVT_CONFIG_BITS but the low 32-bit
MSR value would fit right into mon_info->mon_config, then I would've ignored
this one even faster.

And besides, I'm wondering if you're spending your 0day resources properly if
you're testing patches one-by-one for W=1 warnings?!

If it were me, I'd prefer if you build-test patches on lkml for build *errors*,
*before* they get picked up in some tree instead of testing the applied branches
already for some questionable warnings...

Thx.

---

diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index cd4e668e5019..9bd0eb050e7a 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -1453,17 +1453,17 @@ static void mon_event_config_read(void *info)
 {
 	struct mon_config_info *mon_info = info;
 	unsigned int index;
-	u32 h;
+	u64 msrval;
 
 	index = mon_event_config_index_get(mon_info->evtid);
 	if (index == INVALID_CONFIG_INDEX) {
 		pr_warn_once("Invalid event id %d\n", mon_info->evtid);
 		return;
 	}
-	rdmsr(MSR_IA32_EVT_CFG_BASE + index, mon_info->mon_config, h);
+	rdmsrl(MSR_IA32_EVT_CFG_BASE + index, msrval);
 
 	/* Report only the valid event configuration bits */
-	mon_info->mon_config &= MAX_EVT_CONFIG_BITS;
+	mon_info->mon_config = msrval & MAX_EVT_CONFIG_BITS;
 }
 
 static void mondata_config_read(struct rdt_domain *d, struct mon_config_info *mon_info)


-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

[PATCH] x86/resctrl: Fix a silly -Wunused-but-set-variable warning

[Borislav Petkov]

From: "Borislav Petkov (AMD)" <bp@alien8.de>

clang correctly complains

  arch/x86/kernel/cpu/resctrl/rdtgroup.c:1456:6: warning: variable \
     'h' set but not used [-Wunused-but-set-variable]
          u32 h;
              ^

but it can't know whether this use is innocuous or really a problem.
There's a reason why those warning switches are behind a W=1 and not
enabled by default - yes, one needs to do:

  make W=1 CC=clang HOSTCC=clang arch/x86/kernel/cpu/resctrl/

with clang 14 in order to trigger it.

I would normally not take a silly fix like that but this one is simple
and doesn't make the code uglier so...

Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: Borislav Petkov (AMD) <bp@alien8.de>
Link: https://lore.kernel.org/r/202301242015.kbzkVteJ-lkp@intel.com
---
 arch/x86/kernel/cpu/resctrl/rdtgroup.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index 5990589f8a21..e2c1599d1b37 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -1453,17 +1453,17 @@ static void mon_event_config_read(void *info)
 {
 	struct mon_config_info *mon_info = info;
 	unsigned int index;
-	u32 h;
+	u64 msrval;
 
 	index = mon_event_config_index_get(mon_info->evtid);
 	if (index == INVALID_CONFIG_INDEX) {
 		pr_warn_once("Invalid event id %d\n", mon_info->evtid);
 		return;
 	}
-	rdmsr(MSR_IA32_EVT_CFG_BASE + index, mon_info->mon_config, h);
+	rdmsrl(MSR_IA32_EVT_CFG_BASE + index, msrval);
 
 	/* Report only the valid event configuration bits */
-	mon_info->mon_config &= MAX_EVT_CONFIG_BITS;
+	mon_info->mon_config = msrval & MAX_EVT_CONFIG_BITS;
 }
 
 static void mondata_config_read(struct rdt_domain *d, struct mon_config_info *mon_info)
-- 
2.35.1

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH] x86/resctrl: Fix a silly -Wunused-but-set-variable warning

[Reinette Chatre]

Hi Boris,

On 1/24/2023 10:13 AM, Borislav Petkov wrote:
> From: "Borislav Petkov (AMD)" <bp@alien8.de>
> 
> clang correctly complains
> 
>   arch/x86/kernel/cpu/resctrl/rdtgroup.c:1456:6: warning: variable \
>      'h' set but not used [-Wunused-but-set-variable]
>           u32 h;
>               ^
> 
> but it can't know whether this use is innocuous or really a problem.
> There's a reason why those warning switches are behind a W=1 and not
> enabled by default - yes, one needs to do:
> 
>   make W=1 CC=clang HOSTCC=clang arch/x86/kernel/cpu/resctrl/
> 
> with clang 14 in order to trigger it.
> 
> I would normally not take a silly fix like that but this one is simple
> and doesn't make the code uglier so...
> 
> Reported-by: kernel test robot <lkp@intel.com>
> Signed-off-by: Borislav Petkov (AMD) <bp@alien8.de>
> Link: https://lore.kernel.org/r/202301242015.kbzkVteJ-lkp@intel.com
> ---

Thank you very much.

Acked-by: Reinette Chatre <reinette.chatre@intel.com>

Reinette

Re: [PATCH] x86/resctrl: Fix a silly -Wunused-but-set-variable warning

[Moger, Babu]

Boris, Thanks for the fix.

Tested-by: Babu Moger <babu.moger@amd.com>


On 1/24/23 12:13, Borislav Petkov wrote:
> From: "Borislav Petkov (AMD)" <bp@alien8.de>
>
> clang correctly complains
>
>   arch/x86/kernel/cpu/resctrl/rdtgroup.c:1456:6: warning: variable \
>      'h' set but not used [-Wunused-but-set-variable]
>           u32 h;
>               ^
>
> but it can't know whether this use is innocuous or really a problem.
> There's a reason why those warning switches are behind a W=1 and not
> enabled by default - yes, one needs to do:
>
>   make W=1 CC=clang HOSTCC=clang arch/x86/kernel/cpu/resctrl/
>
> with clang 14 in order to trigger it.
>
> I would normally not take a silly fix like that but this one is simple
> and doesn't make the code uglier so...
>
> Reported-by: kernel test robot <lkp@intel.com>
> Signed-off-by: Borislav Petkov (AMD) <bp@alien8.de>
> Link: https://lore.kernel.org/r/202301242015.kbzkVteJ-lkp@intel.com
> ---
>  arch/x86/kernel/cpu/resctrl/rdtgroup.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index 5990589f8a21..e2c1599d1b37 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -1453,17 +1453,17 @@ static void mon_event_config_read(void *info)
>  {
>  	struct mon_config_info *mon_info = info;
>  	unsigned int index;
> -	u32 h;
> +	u64 msrval;
>  
>  	index = mon_event_config_index_get(mon_info->evtid);
>  	if (index == INVALID_CONFIG_INDEX) {
>  		pr_warn_once("Invalid event id %d\n", mon_info->evtid);
>  		return;
>  	}
> -	rdmsr(MSR_IA32_EVT_CFG_BASE + index, mon_info->mon_config, h);
> +	rdmsrl(MSR_IA32_EVT_CFG_BASE + index, msrval);
>  
>  	/* Report only the valid event configuration bits */
> -	mon_info->mon_config &= MAX_EVT_CONFIG_BITS;
> +	mon_info->mon_config = msrval & MAX_EVT_CONFIG_BITS;
>  }
>  
>  static void mondata_config_read(struct rdt_domain *d, struct mon_config_info *mon_info)

-- 
Thanks
Babu Moger

Re: [tip:x86/cache 9/13] arch/x86/kernel/cpu/resctrl/rdtgroup.c:1456:6: warning: variable 'h' set but not used

[Philip Li]

On Tue, Jan 24, 2023 at 02:05:37PM +0100, Borislav Petkov wrote:
> On Tue, Jan 24, 2023 at 08:37:14PM +0800, kernel test robot wrote:
> > tree:   https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git x86/cache
> > head:   0a363fb23ee2f7beb08437ad7db86d195878d79f
> > commit: dc2a3e857981f859889933cf66ded117d74edff1 [9/13] x86/resctrl: Add interface to read mbm_total_bytes_config
> 
> These patches have been around for a long while now. How come you test them just
> now, after I take them?

Sorry Boris for the late reply. We will look into this to understand what happened
for the patches that why we didn't report it out earlier before they landed on tip.

> 
> > >> arch/x86/kernel/cpu/resctrl/rdtgroup.c:1456:6: warning: variable 'h' set but not used [-Wunused-but-set-variable]
> >            u32 h;
> >                ^
> 
> The fix is simple - use rsmsrl(), see below.
> 
> If it weren't that simple I'd simply ignore this warning altogether. Yes, it is
> unused because the damn high MSR value needs to go somewhere, for chrissakes.
> And if it didn't need that masking with MAX_EVT_CONFIG_BITS but the low 32-bit
> MSR value would fit right into mon_info->mon_config, then I would've ignored
> this one even faster.
> 
> And besides, I'm wondering if you're spending your 0day resources properly if
> you're testing patches one-by-one for W=1 warnings?!
> 
> If it were me, I'd prefer if you build-test patches on lkml for build *errors*,
> *before* they get picked up in some tree instead of testing the applied branches
> already for some questionable warnings...

Thanks a lot for the suggestions, we will think of this to continuously optimize the
service. Right now, we try to build-test the patches that we can find a suitable base
to apply the patches successfully, some of effort could fail. Then we only test them
when they appears on repo. We will keep monitoring the patch testing status to see
anything we can fix as well.

> 
> Thx.
> 
> ---
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index cd4e668e5019..9bd0eb050e7a 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -1453,17 +1453,17 @@ static void mon_event_config_read(void *info)
>  {
>  	struct mon_config_info *mon_info = info;
>  	unsigned int index;
> -	u32 h;
> +	u64 msrval;
>  
>  	index = mon_event_config_index_get(mon_info->evtid);
>  	if (index == INVALID_CONFIG_INDEX) {
>  		pr_warn_once("Invalid event id %d\n", mon_info->evtid);
>  		return;
>  	}
> -	rdmsr(MSR_IA32_EVT_CFG_BASE + index, mon_info->mon_config, h);
> +	rdmsrl(MSR_IA32_EVT_CFG_BASE + index, msrval);
>  
>  	/* Report only the valid event configuration bits */
> -	mon_info->mon_config &= MAX_EVT_CONFIG_BITS;
> +	mon_info->mon_config = msrval & MAX_EVT_CONFIG_BITS;
>  }
>  
>  static void mondata_config_read(struct rdt_domain *d, struct mon_config_info *mon_info)
> 
> 
> -- 
> Regards/Gruss,
>     Boris.
> 
> https://people.kernel.org/tglx/notes-about-netiquette

Re: [tip:x86/cache 9/13] arch/x86/kernel/cpu/resctrl/rdtgroup.c:1456:6: warning: variable 'h' set but not used

[Borislav Petkov]

On Thu, Feb 09, 2023 at 11:06:34AM +0800, Philip Li wrote:
> Thanks a lot for the suggestions, we will think of this to continuously optimize the
> service. Right now, we try to build-test the patches that we can find a suitable base
> to apply the patches successfully, some of effort could fail. Then we only test them
> when they appears on repo. We will keep monitoring the patch testing status to see
> anything we can fix as well.

Cool, thanks.

I see you've started doing silly tests like subdirectory builds for W=
warnings, picking one such report at random from lkml:

https://lore.kernel.org/r/202302091432.VgittDjI-lkp@intel.com

COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=powerpc SHELL=/bin/bash drivers/spi/

and then it says

drivers/spi/spi-mpc52xx-psc.c:195:5: warning: no previous prototype for 'mpc52xx_psc_spi_transfer_one_message' [-Wmissing-prototypes]

Yes, this is all fine and dandy but such tests should be the lowest prio
eva! If you have a way to schedule by prio, those should wait until all
the other build tests have happened.

I don't know how your resources are spread out and whether you even can
do as many so I'm only reporting from my experience, in case you were
wondering what you could improve:

People push branches to their trees and wait for the robot to test them.
And they wait and wait. But instead, such silly warnings come.

So it would be a lot better if you could expedite such pushed branches'
build tests first and then the rest.

And then if there are no branches, submitted patchsets on the ML.

If you're trying to figure out what base to use, you can put a doc
somewhere telling people how to specify the base for you and they will
start doing it, you will parse the 0th message for that info and use the
base.

And the long-standing feature request we have: a simple web page
somewhere which says how far is it with testing. So that people can go
and look at it and know whether to wait for test results before sending.

The web page doesn't have to be anything special - just a table of
branches being tested at the moment.

Anyway, I thought I should give you some suggestions if you were looking
for some. :-)

Thanks for the testing work - it is appreciated!

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [tip:x86/cache 9/13] arch/x86/kernel/cpu/resctrl/rdtgroup.c:1456:6: warning: variable 'h' set but not used

[Philip Li]

On Thu, Feb 09, 2023 at 10:53:47AM +0100, Borislav Petkov wrote:
> On Thu, Feb 09, 2023 at 11:06:34AM +0800, Philip Li wrote:
> > Thanks a lot for the suggestions, we will think of this to continuously optimize the
> > service. Right now, we try to build-test the patches that we can find a suitable base
> > to apply the patches successfully, some of effort could fail. Then we only test them
> > when they appears on repo. We will keep monitoring the patch testing status to see
> > anything we can fix as well.
> 
> Cool, thanks.

Thanks a lot Boris for your time and suggestions. I add some extra information and next step
info below.

> 
> I see you've started doing silly tests like subdirectory builds for W=
> warnings, picking one such report at random from lkml:
> 
> https://lore.kernel.org/r/202302091432.VgittDjI-lkp@intel.com
> 
> COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=powerpc SHELL=/bin/bash drivers/spi/

Sorry for confusion, we do full make for the kernel to gather all issues.
And we try to provide a quicker way for developer to reproduce the issue,
thus the step in reproduce part shows the subdirectories that could reproduce
the reported issue.

> 
> and then it says
> 
> drivers/spi/spi-mpc52xx-psc.c:195:5: warning: no previous prototype for 'mpc52xx_psc_spi_transfer_one_message' [-Wmissing-prototypes]
> 
> Yes, this is all fine and dandy but such tests should be the lowest prio
> eva! If you have a way to schedule by prio, those should wait until all
> the other build tests have happened.

Thanks for suggestion, we will think of how to prioritize different issues.

> 
> I don't know how your resources are spread out and whether you even can
> do as many so I'm only reporting from my experience, in case you were
> wondering what you could improve:
> 
> People push branches to their trees and wait for the robot to test them.
> And they wait and wait. But instead, such silly warnings come.
> 
> So it would be a lot better if you could expedite such pushed branches'
> build tests first and then the rest.

Got it. Internally, we use the merge strategy to combine as many branches
as we can to run build testing, and bisect the issues if found. We need
further think of how to let high priority issues to be reported out earlier.

> 
> And then if there are no branches, submitted patchsets on the ML.
> 
> If you're trying to figure out what base to use, you can put a doc
> somewhere telling people how to specify the base for you and they will
> start doing it, you will parse the 0th message for that info and use the
> base.

Thanks, so far, we recommend to use --base option in our report mail, like

	[If your patch is applied to the wrong git tree, kindly drop us a note.
	And when submitting patch, we suggest to use '--base' as documented in
	https://git-scm.com/docs/git-format-patch#_base_tree_information]

And we do need a place to put such information, so people can refer to. Without
a public available site for us, we will update https://github.com/intel/lkp-tests/wiki
firstly to host enough information.

> 
> And the long-standing feature request we have: a simple web page
> somewhere which says how far is it with testing. So that people can go
> and look at it and know whether to wait for test results before sending.

Really apologize for this. Yes, we got the request long before, we did some
attempts to resolve this with a web site to show the progress, but it wasn't
able to be public yet.

> 
> The web page doesn't have to be anything special - just a table of
> branches being tested at the moment.

Thanks for the hint, this is doable, and we definitely need do this. I will
plan it to have initial version ready by Q1.

> 
> Anyway, I thought I should give you some suggestions if you were looking
> for some. :-)

Thanks a lot Boris, these suggestions are very helpful to further improve the
0-day ci service.

> 
> Thanks for the testing work - it is appreciated!
> 
> -- 
> Regards/Gruss,
>     Boris.
> 
> https://people.kernel.org/tglx/notes-about-netiquette

Re: [tip:x86/cache 9/13] arch/x86/kernel/cpu/resctrl/rdtgroup.c:1456:6: warning: variable 'h' set but not used

[Borislav Petkov]

On Fri, Feb 10, 2023 at 11:15:00AM +0800, Philip Li wrote:
> Sorry for confusion, we do full make for the kernel to gather all issues.
> And we try to provide a quicker way for developer to reproduce the issue,
> thus the step in reproduce part shows the subdirectories that could reproduce
> the reported issue.

Aah, ok.

I guess those reproduction instructions should then say:

"... the fastest way to reproduce is ..."

Or so.

> Got it. Internally, we use the merge strategy to combine as many branches
> as we can to run build testing, and bisect the issues if found. We need
> further think of how to let high priority issues to be reported out earlier.

Hmm, I wouldn't do that.

If you test a whole bunch of patchsets, then you're not really testing
each of them but you're testing the common thing which you've created by
merging.

I think it makes most sense to test single patchsets, single tree
branches which should hopefully contain commits which belong to a single
topic - this is how we aim to do them in the tip tree, at least, but
others do different things, and so on.

And then test linux-next. But I think you do that.

> Thanks, so far, we recommend to use --base option in our report mail, like
> 
> 	[If your patch is applied to the wrong git tree, kindly drop us a note.
> 	And when submitting patch, we suggest to use '--base' as documented in
> 	https://git-scm.com/docs/git-format-patch#_base_tree_information]
> 
> And we do need a place to put such information, so people can refer to. Without
> a public available site for us, we will update https://github.com/intel/lkp-tests/wiki
> firstly to host enough information.

Right, make that URL part of every report so that people can go and read
about it and know what are best practices. I'll try to remember myself
to use --base too.

> Thanks for the hint, this is doable, and we definitely need do this. I will
> plan it to have initial version ready by Q1.

You have a wiki page above already. Add a status subpage and dump into
it periodically what current trees are being tested. And this way you
have *something* to go with. You can always improve it later on.

> Thanks a lot Boris, these suggestions are very helpful to further improve the
> 0-day ci service.

You're welcome - I'm glad that can be of help.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette