Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <9a9b0de5-0e19-d68f-606f-c10817b873bc@intel.com>
Date:   Mon, 24 Apr 2023 09:45:01 -0700
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Firefox/102.0 Thunderbird/102.10.0
Subject: Re: [PATCH v2 16/24] selftests/resctrl: Create cache_alloc_size()
 helper
Content-Language: en-US
To:     =?UTF-8?Q?Ilpo_J=c3=a4rvinen?= <ilpo.jarvinen@linux.intel.com>
CC:     <linux-kselftest@vger.kernel.org>,
        Fenghua Yu <fenghua.yu@intel.com>,
        Shuah Khan <shuah@kernel.org>,
        LKML <linux-kernel@vger.kernel.org>,
        "Shaopeng Tan" <tan.shaopeng@jp.fujitsu.com>
References: <20230418114506.46788-1-ilpo.jarvinen@linux.intel.com>
 <20230418114506.46788-17-ilpo.jarvinen@linux.intel.com>
 <287e5f1f-87d5-473a-2bff-271adca8d458@intel.com>
 <5db5680-4123-17b8-e657-39ba20356259@linux.intel.com>
From:   Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <5db5680-4123-17b8-e657-39ba20356259@linux.intel.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?RVc2NzNIS3JJaGtURTh3Q09NQ2oreXV2N2JpeFJ5d1dBZE5oSGJpcVVDUWN1?=
 =?utf-8?B?a3BaM0JZNVlMV2t2ZGhNU0EwUUE0NEtoWWVIL2RQZFh6bFR0NzE0NVhvYk0w?=
 =?utf-8?B?YVlNR2tNL3pldG8wWkhCS2JNa3NRRmNtcnlvZzZQeXgrNDJkRFNxZ1FmTmtV?=
 =?utf-8?B?dklIelNPMHhLSDBoY0h2TDcxZEgwbGRIVVVSQStSSkg1U296b3EwYWtpS3Fq?=
 =?utf-8?B?bEFWYTJxcGh5MXJaa2hsQ0NVaHJzclNvVUcyZEMrYm5uM0JDeWFHenJmbEdP?=
 =?utf-8?B?TC9MdjJUVjh1OFpxdFBnTDIrMEtqU2lPUkFRQkRhYkM2QWR0a1dQanZkdEM0?=
 =?utf-8?B?ampIcy9iNGlRTmhUL0pEenZDT25KT1RhQXp0UmlwSGFEeURTVmxrTTdLYVlR?=
 =?utf-8?B?dDd2N2NNZVRPQ3YxVXBnOVVUclJGempJTDFEUVloUXhnUEc4eEVOSXMxR1Zj?=
 =?utf-8?B?ZlR0WVBRVGs4WHRQS0RnUTkyVE1mbzVETWV1VXpmU0ZJN3ZFSkZNeXNQd09s?=
 =?utf-8?B?SXBJcGo0VXl5OWlmM3VDTVlyMEtxYncxVzBNRytjSHZZQnJnSUVwNHVaVUhI?=
 =?utf-8?B?bVR6cWpqK1I0RGFzdjY0U1RNOENCaFFlY21Ta0J2SEh1c2I5OHp1R01FbE9n?=
 =?utf-8?B?N3BNbTFhMEE0cUIxSXJSTmlOM3JPZjZCYUFpNXpTQm5oWDhXYWZpYU5SSS9u?=
 =?utf-8?B?V3p5MVJXTFhPUnVmQkp5Uy90d1JkL3k3RWFFVStsRWxMcW4zV2RnTWJYcFR3?=
 =?utf-8?B?Yms1aGc3NHZlWUlOUFZYSmxLcFVWSjNncVdaVUVCeEtRbUozN1BXVFJGendw?=
 =?utf-8?B?SExEQmxFS1pCQklTbUV5OWd3VFBOalI1dlNKcGxSWlJzUjZpczEwRGVVYXE1?=
 =?utf-8?B?UU8ranFwRGxXVzVsamc5Ykkrb2hGeDFOOGNNR0libGxvVmliSTV3Y01DaHUz?=
 =?utf-8?B?cm9CLy9TdUprQVBaeGoyL2U5aGR0UnNVU0pFTk1oZlplMkU4aEp4a2M0ajZH?=
 =?utf-8?B?dnlnayt0by9UaTUrTnBVdGF4RUFsamJZclU1c1ZVY2RyRDR2dkdkejAvSkxX?=
 =?utf-8?B?NStqKzkzUmwvcmwrWDl4aHRFWDNEd0NiamNmdU1aM3lScnpCMjZwZjJwUmxK?=
 =?utf-8?B?RWpjSkRpWUxsS0hrV0Q4RDM0MHQ5cnNzT3llSVJOZ0duMU41ZXB0dG9kRGhi?=
 =?utf-8?B?T1d6R0E0TlEyUS93SEx4MWU4eWlxdWxxUldwYzBuY0VUS0c2TFRpREpsWmtR?=
 =?utf-8?B?cEhOenlqS0dZMWsxVW1qMXZjTGw4bkQwdEl2cmpRaTJndVdqaEZ0R3UzUXFB?=
 =?utf-8?B?Vmk2YTY5Qyszd3lvb0hvNTcwcDdsMEpCOGI1WVJtdGtFWkplZUt6LzRrU1Zn?=
 =?utf-8?B?ZDNucmtWRHJhN25NNll2MEt2bHRrUC9aWlpUcUd4MzFpWUc3VVhVak5NWlhs?=
 =?utf-8?B?V3pNTHZzRm53YU1kZkZYU2M3Y3JZdVAzbzhhaHpOS0tad1E4Qzgzb1g1ejMr?=
 =?utf-8?B?TSs0ZWo2YWFPaEs5RStsVkdydFl3Q1lWdXI1ak5rS2wvWWhGYkxyWWRBTy9s?=
 =?utf-8?B?dUpaTzZ1QXAxUWhHcW5DRE5Qc1AwMXpTME0zanMxRlhRVXFNODljV2NtNFVX?=
 =?utf-8?B?TGVYa29lQ1NkZlNRUWNSMkFSWU0vZ3QwOHJWeGt3Ums4MlFiMy9zRlFRNmhJ?=
 =?utf-8?B?V3d6bmhmaWpTVEhKMXBkQ3dOdEJEb3ZwUjNnaUFIVXN6ckdaZ0NsamtsRmw4?=
 =?utf-8?B?bklqQnZTQ0ZseVpEZDJnQ3ZOTldmYWY0aWRJRW15Vm9KOHpBMVdOeWNKSnhV?=
 =?utf-8?B?MHlhczlTVFZzNEp0VXFDaFhEdGIvVDN1TlhSK25ndWhwVmNWTld6WndIRzQw?=
 =?utf-8?B?eFhFTWVacDRjMm5va3FNTkIvVjVBRGM5UmxQS3AvV3FMUGdienFFdnZ0bWlw?=
 =?utf-8?B?RFFGM0hGWWx5d3AzMW5RYW9GWWpXcXl2bjhmL2tMcTlTRmZSZGhpYlA4NzZm?=
 =?utf-8?B?M2RjVGkxZk5tbUI0WmM5d2hzZXlIaHIzVjlhMDY4NTJkWWRZRzd5OGlwdUpi?=
 =?utf-8?B?ZWNoN2NxYnpYWWFxYUlTTXZBSzczN1RpVnFOaDNZY1RGeE5CWkJXY1BFWmZs?=
 =?utf-8?B?T2NhQjJRTnJLdmlvbmhEWXdwSVJ2emJKVzR1ak96UzBJaXFjNUp6NlI3dlha?=
 =?utf-8?B?c3c9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 38b67823-6dc9-40a9-2afd-08db44e340ca
X-MS-Exchange-CrossTenant-AuthSource: CY4PR11MB1862.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 24 Apr 2023 16:45:04.0176
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: pq4WtTQysKdkw7CHvv2PP91uqxEUaWHknZ5BiVkANCiWGfq8ju0idNXM7cB+y15dsUfDeYi6n7t8Roepq5DhpJxbjcSAsaruJ5oq8b+Ucps=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MW5PR11MB5785
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-5.8 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,
        RCVD_IN_DNSWL_MED,SPF_HELO_NONE,SPF_NONE,T_SCC_BODY_TEXT_LINE
        autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi Ilpo,

On 4/24/2023 9:28 AM, Ilpo Järvinen wrote:
> On Fri, 21 Apr 2023, Reinette Chatre wrote:
> 
>> On 4/18/2023 4:44 AM, Ilpo Järvinen wrote:
>>> CAT and CMT tests calculate the span size from the n-bits cache
>>> allocation on their own.
>>>
>>> Add cache_alloc_size() helper which calculates size of the cache
>>> allocation for the given number of bits to avoid duplicating code.
>>
>> This patch is very heavy on the usage of allocation when I think it
>> only refers to the cache size ... how that size is used by the caller
>> is independent from this. 
>>
>> Compare to how it sounds with some small changes to changelog:
>>
>> 	CAT and CMT tests calculate the span size from the capacity
>> 	bitmask independently.
>> 	
>> 	Add cache_size() helper which calculates the size of the
>> 	cache for the given number of bits to avoid duplicating code.
>>
>> I think removing "alloc" helps to convey what this code actually does.
> 
> Does it? Without something to indicate its not the full cache size, 
> there's possiblity for confusion. While the tests are mostly interested 
> in the allocated size, the full cache size is also collected (solely for 
> printing it out, IIRC). Maybe I should rename those variable to 
> total_cache_size or something like that to mitigate the confusion?

This patch adds and use a utility that converts a bitmask into bytes.
I do not think it should dictate what the meaning or usage of the bitmask
is.

>>> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
>>> ---
>>>  tools/testing/selftests/resctrl/cache.c    | 27 ++++++++++++++++++++++
>>>  tools/testing/selftests/resctrl/cat_test.c |  8 +++++--
>>>  tools/testing/selftests/resctrl/cmt_test.c |  4 +++-
>>>  tools/testing/selftests/resctrl/resctrl.h  |  2 ++
>>>  4 files changed, 38 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/tools/testing/selftests/resctrl/cache.c b/tools/testing/selftests/resctrl/cache.c
>>> index 6bc912de38be..b983af394e33 100644
>>> --- a/tools/testing/selftests/resctrl/cache.c
>>> +++ b/tools/testing/selftests/resctrl/cache.c
>>> @@ -15,6 +15,33 @@ static struct read_format rf_cqm;
>>>  static int fd_lm;
>>>  char llc_occup_path[1024];
>>>  
>>> +/*
>>> + * cache_alloc_size - Calculate alloc size for given cache alloc mask
>>
>> "cache_size - Calculate number of bytes represented by bitmask" ?
>> Please feel free to improve.
>>
>>
>>> + * @cpu_no:		CPU number
>>> + * @cache_type:		Cache level L2/L3
>>> + * @alloc_mask:		Cache alloc mask
>>
>> The description is mostly a rewrite of the variable name. Can it be
>> more descriptive?
>>
>>> + * @alloc_size:		Alloc size returned on success
>>
>> I do not think the utility should assume anything about how
>> the value it provides should be used. Instead it should just reflect
>> what the value is.
> 
> I was just referring to that the value is filled only on success.

I understand. My comment was about the naming of the parameter, which can,
for example, just be "size".

>>> + * Returns: 0 on success with @alloc_size filled, non-zero on error.
>>> + */
>>> +int cache_alloc_size(int cpu_no, char *cache_type, unsigned long alloc_mask,
>>> +		     unsigned long *alloc_size)
>>> +{
>>> +	unsigned long cache_size, full_mask;
>>> +	int ret;
>>> +
>>> +	ret = get_cbm_mask(cache_type, &full_mask);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	ret = get_cache_size(cpu_no, cache_type, &cache_size);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	*alloc_size = cache_size * count_bits(alloc_mask) / count_bits(full_mask);
>>> +	return 0;
>>> +}
>>> +
>>>  static void initialize_perf_event_attr(void)
>>>  {
>>>  	pea_llc_miss.type = PERF_TYPE_HARDWARE;
>>> diff --git a/tools/testing/selftests/resctrl/cat_test.c b/tools/testing/selftests/resctrl/cat_test.c
>>> index 9bf5d05d9e74..d3fbd4de9f8a 100644
>>> --- a/tools/testing/selftests/resctrl/cat_test.c
>>> +++ b/tools/testing/selftests/resctrl/cat_test.c
>>> @@ -140,7 +140,9 @@ int cat_perf_miss_val(int cpu_no, int n, char *cache_type)
>>>  	/* Set param values for parent thread which will be allocated bitmask
>>>  	 * with (max_bits - n) bits
>>>  	 */
>>> -	param.span = cache_size * (count_of_bits - n) / count_of_bits;
>>> +	ret = cache_alloc_size(cpu_no, cache_type, l_mask, &param.span);
>>> +	if (ret)
>>> +		return ret;
>>>  	strcpy(param.ctrlgrp, "c2");
>>>  	strcpy(param.mongrp, "m2");
>>>  	strcpy(param.filename, RESULT_FILE_NAME2);
>>> @@ -162,7 +164,9 @@ int cat_perf_miss_val(int cpu_no, int n, char *cache_type)
>>>  		param.mask = l_mask_1;
>>>  		strcpy(param.ctrlgrp, "c1");
>>>  		strcpy(param.mongrp, "m1");
>>> -		param.span = cache_size * n / count_of_bits;
>>> +		ret = cache_alloc_size(cpu_no, cache_type, l_mask_1, &param.span);
>>> +		if (ret)
>>> +			exit(-1);
>>>  		strcpy(param.filename, RESULT_FILE_NAME1);
>>>  		param.num_of_runs = 0;
>>>  		param.cpu_no = sibling_cpu_no;
>>
>> Did this change intend to remove the duplicate code mentioned
>> in the changelog?
> 
> It removes n CBM bits -> cache size calculations by collecting the 
> calculation into one place.
> 
> cache_alloc_size() takes mask instead of n (CBM bits) as input which makes 
> things easier down the line when the new CAT test starts to tweak the 
> alloc size. The new CAT test would otherwise need to track both the mask 
> and n.
> 
> cache_alloc_size() is independent of what caller requires so the full mask 
> is not passed from the caller.
> 
>> I was expecting the calls to get_cbm_mask() and get_cache_size() within 
>> cat_perf_miss_val() to be removed.
> 
> I would have wanted to remove get_cache_size() but it would mean removing 
> cache size print or moving it to elsewhere.
> 
> get_cbm_mask() cannot be removed as it's used by the test to calculate the 
> mask the test wants (but it no longer has to determine the size itself but 
> uses this new helper instead).
> 
> I can try to amend the changelog to explain things better.

The current motivation for this patch is to avoid duplicating code but as I
see it it introduces more duplicated code.

Reinette