linux-kernel - Re: [PATCH 03/24] selftests/resctrl: Refactor get_cbm

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <964f1078-7e36-7e39-9bbe-687d5b2458de@linux.intel.com>
Date:   Fri, 3 Nov 2023 14:09:50 +0200 (EET)
From:   Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To:     Reinette Chatre <reinette.chatre@...el.com>
cc:     linux-kselftest@...r.kernel.org, Shuah Khan <shuah@...nel.org>,
        Shaopeng Tan <tan.shaopeng@...fujitsu.com>,
        Maciej Wieczór-Retman 
        <maciej.wieczor-retman@...el.com>,
        Fenghua Yu <fenghua.yu@...el.com>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 03/24] selftests/resctrl: Refactor get_cbm_mask()

On Thu, 2 Nov 2023, Reinette Chatre wrote:
> On 10/24/2023 2:26 AM, Ilpo Järvinen wrote:
> 
> > @@ -229,6 +228,31 @@ int get_cbm_mask(char *cache_type, char *cbm_mask)
> >  	return 0;
> >  }
> >  
> > +/*
> > + * get_cbm_mask - Get cbm bit mask
> 
> I know you just copied code here but please keep an eye out for acronyms
> to be written in caps.

Yeah, Maciej also commented on this. I've already made some changes but 
I'll incorporate some of your suggestions too.

> This needs not be named to reflect verbatim what the function does.
> Looking ahead I wonder if "get_full_mask()/get_max_mask()" may not be a
> clear indication of what this does?
>
> Something like:
> 	get_full_mask()/get_max_mask()  Get maximum Cache Bit Mask (CBM) 

Having max in the name sounds useful.

Also related to this, the local variables called long_mask should be 
renamed but perhaps not in this series to not block Maciej's work with 
neverending stream of cleanups :-).

> 	@cache_type:	Cache level(? or should this be "type") as "L2" or L3".
> 	@mask:		Full/Maximum portion of cache available for
> 			allocation represented by bit mask
> 			returned as unsigned long.
> 	
> 
> > + * @cache_type:		Cache level L2/L3
> > + * @mask:		cbm_mask returned as unsigned long
> > + *
> > + * Return: = 0 on success, < 0 on failure.
> > + */
> > +int get_cbm_mask(const char *cache_type, unsigned long *mask)
> > +{
> > +	char cbm_mask_path[1024];
> > +	int ret;
> > +
> > +	if (!cache_type)
> > +		return -1;
> 
> Just to confirm ... error checking on mask is intentionally deferred 
> until get_bit_mask()?

I tried to put as much as possible into get_bit_mask() since every caller 
will have to do the same things anyway. I cannot avoid checking cache_type 
here because snprintf() is using it.


Once again, very superb review of the whole series, thank you very much 
for all the effort! It's really appreciated!


-- 
 i.