Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.33 as permitted sender) client-ip=23.128.96.33;
Date:   Fri, 3 Nov 2023 14:09:50 +0200 (EET)
From:   =?ISO-8859-15?Q?Ilpo_J=E4rvinen?= <ilpo.jarvinen@linux.intel.com>
To:     Reinette Chatre <reinette.chatre@intel.com>
cc:     linux-kselftest@vger.kernel.org, Shuah Khan <shuah@kernel.org>,
        Shaopeng Tan <tan.shaopeng@jp.fujitsu.com>,
        =?ISO-8859-15?Q?Maciej_Wiecz=F3r-Retman?= 
        <maciej.wieczor-retman@intel.com>,
        Fenghua Yu <fenghua.yu@intel.com>,
        LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 03/24] selftests/resctrl: Refactor get_cbm_mask()
In-Reply-To: <575b4c53-34c4-48ed-906d-b9dedb80e0ef@intel.com>
Message-ID: <964f1078-7e36-7e39-9bbe-687d5b2458de@linux.intel.com>
References: <20231024092634.7122-1-ilpo.jarvinen@linux.intel.com> <20231024092634.7122-4-ilpo.jarvinen@linux.intel.com> <575b4c53-34c4-48ed-906d-b9dedb80e0ef@intel.com>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="8323329-1072741397-1699013395=:1725"
Precedence: bulk

  This message is in MIME format.  The first part should be readable text,
  while the remaining parts are likely unreadable without MIME-aware tools.

--8323329-1072741397-1699013395=:1725
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8BIT

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.

--8323329-1072741397-1699013395=:1725--