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: <c54c301e-ff18-90a6-3e68-d77d0df23b77@intel.com>
Date:   Fri, 21 Apr 2023 17:17: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 15/24] selftests/resctrl: Refactor get_cbm_mask()
Content-Language: en-US
To:     =?UTF-8?Q?Ilpo_J=c3=a4rvinen?= <ilpo.jarvinen@linux.intel.com>,
        <linux-kselftest@vger.kernel.org>,
        Fenghua Yu <fenghua.yu@intel.com>,
        "Shuah Khan" <shuah@kernel.org>, <linux-kernel@vger.kernel.org>
CC:     Shaopeng Tan <tan.shaopeng@jp.fujitsu.com>
References: <20230418114506.46788-1-ilpo.jarvinen@linux.intel.com>
 <20230418114506.46788-16-ilpo.jarvinen@linux.intel.com>
From:   Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <20230418114506.46788-16-ilpo.jarvinen@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?ZlpGY1lXTExzaGNmK0JkU2hPaWZEd1FqRHVJQWo0RE5IUzN2cVZNenFYSk0r?=
 =?utf-8?B?aHl3ZTdkK2tlalB2djI0RGJjcGdNVzByTWp4VC9CTjVzTm1kUzlzeGxRRnZK?=
 =?utf-8?B?U0pXcXRMVXZNSkRic1FIZDB2d3RnSHUzZEJZNHFlM1pjaU1CN2NuM0ZtbnIx?=
 =?utf-8?B?MmlPVVJvVzNIaFViWUx2QWtzaytJZGN3UTJUY3FsQSs2WmJRU1NqbFJuOVZB?=
 =?utf-8?B?RkJkS2N4eUp4VmgrTkhaNmt2UXB5YTNMdE5HdC9mZXhLRm1weCtKWjZ3VTEv?=
 =?utf-8?B?SU5ranR3R09odW95OGZtTDVRQmZxUEJwelhSci9HMGVDcVlBUC82eGZRZlBK?=
 =?utf-8?B?WGs3Nm9WTXI2Z0tDckZMVzhtT2VvTm43TXBxZTBobzF4YmhETkFxWDhySXRk?=
 =?utf-8?B?b2N3V0lITW50U005QzZESTIyanF0MEZlZDUzdG5lVTl5bDBVc0xHUCt0NVp4?=
 =?utf-8?B?OGw2NzFncVduQ20zVFNMTjRsUk1qSXpSU3dqYmlxV250NUw1NE84SVJEYTNN?=
 =?utf-8?B?ZzFiZ2tmcTAzSk1YSk1oWDRyU0NEL2sxY2NCbkZ0U1JpVEUwSXJGRXJWblB4?=
 =?utf-8?B?NUV0RnBvdHQ1WXQyTFR6OXVxUjNuY1ZXbjZTaW4zSS8zUEc3aHZ6cFdNK1V6?=
 =?utf-8?B?TmdNbTZaR00wTHFqUENldVZIc2NHZFJuN3ZYc29JSFdEV1N6V3JWc3IzaGdL?=
 =?utf-8?B?R2VWSHI2YXlrMWh4RSsvM0dteVF6UnFMbm9ScDNoNktBTDBXczIxTFFKekZT?=
 =?utf-8?B?MnlzMGI2cGFjaFlYdDlFa2phanlyV1pHRVlESjd0OGZwcHZZbVNOdEhFR3h0?=
 =?utf-8?B?bXBlUjFPdUU0d0lLMXFsR3BSMGNtSWVIT3lNeEFFZlZvM1lRMzdKbzc3RlEw?=
 =?utf-8?B?dXZJQVA0cUJSWWRXZ0hidGtqVG9GcmF2OEpYNlBGUDlQTFVCSUFkV3JaZ0dv?=
 =?utf-8?B?SXVTR20zWXY0VXRLVU9KOG5VRVFFeVFsd1JGNUdFRUJFNVQ5TFd0c0VLSysw?=
 =?utf-8?B?djJsU3o5RVh2ZnlTdlgzYktLRVhpemV2RnRDajgxcHM2U0JneXN1cU0rajdm?=
 =?utf-8?B?K2FMNFlMQmZBNDB4cmxsNTNzSEw2SFlBSFVnQm1YZGk0cDhxclNSaXhtRThE?=
 =?utf-8?B?N1NWelU3VGpLYlpPeVc5UnVyQnRJSWFuVlNiZHJpdWhYNFhLU3VJbC9VT0Vz?=
 =?utf-8?B?WDEvRWRXRlZqNk5rcG45WFhNbGNtdmxXUjhma0kwcWVJdU1QSHFYRFhLZnds?=
 =?utf-8?B?VEZkenN2dlhDQTJBVkkyOG1OQ1Q2QVdKaUFXYkNrekFDK3BIMXk4QWU4NWow?=
 =?utf-8?B?OWNNKzAzbUJNOTFzTEpPbjN2RHJHSktSQ0U5M0FPSm9UUWxPR1dwYkpTVU5t?=
 =?utf-8?B?Y0p0UElmQXRrQnFGT25Cc1BIa2FlaVpYTEVWbHBUQkJqMzVPbHJoWHc2dlB3?=
 =?utf-8?B?ZEVHcU93TVJURVhpcUNhYm54N1RPUU51d2x3VDVlL3FNd05tRE8rVHhweGtm?=
 =?utf-8?B?WEpobFdkck5jUm9yL0R3VVB0d1NWKzFYSjhvcEV0TExRMitpeFZUYjc2alpM?=
 =?utf-8?B?T2NzcGlyVTNWd1dNdWkzblJjSHphT2FUTjcvcFlWd1ltRisrVVUrZDRiWDZS?=
 =?utf-8?B?cExNTy82YzdXck9JRUhyZDFQVCtWSm96LzdhTkYycVN1QnhIWHBWT3dXU0xu?=
 =?utf-8?B?MThnWkQrVExRVTRmTUJKalFYOUVjMktVOVc1MThuRnk4QXVCZXY2MDNRbVhO?=
 =?utf-8?B?MFNWM3o1Tks1NXlrazFHcENmc2ZHaml0SEo2UzBlQ3lKWityUk9IMVFhcHRw?=
 =?utf-8?B?S1c3MWJvRHk1eDkweGl2dis0aVhTVllxRmdybVBGQTdhekNkbXErYVFDM3ht?=
 =?utf-8?B?bjliSXo5ZG44OXcvazE0akEyL0lSTmxvSkFRd1gxSkFGcVozRU93RklBbktO?=
 =?utf-8?B?M3RnQ1Y5R0dCV1BwR0drQW5qYWRpbUN0dks0WXlsZlcwd2psdEtpRFNaQk1S?=
 =?utf-8?B?WC9abTdscXJYemJyZzVta0RTYW5wN09JYU9PZmdMdmpFdmFTcTNhc2Z2QWdT?=
 =?utf-8?B?dUxiUDMxbFI5TzFLYkxJdTcyaWhxMnh5QlVkWGIyRlpUZWt3bUpORndyRVUz?=
 =?utf-8?B?K0dydm85WnpCNDVFQTNtdmdXWXFyanV5TlJtMVdjZUlUZ2Q4aFF5dVNmN0th?=
 =?utf-8?B?M0E9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 3c7e7db1-dd08-4da9-6d8e-08db42c6e64b
X-MS-Exchange-CrossTenant-AuthSource: CY4PR11MB1862.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 22 Apr 2023 00:17:03.9552
 (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: e9z5I+3ysstZxLwDOzF2kXwnRNPJ012ou7JSt3lPbDBuT9ipMoXDzVUU15PE57wofGxUlH56rOCwqx6RhVjzJg+mgLA+vfh97ZdncCSIDqI=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN9PR11MB5227
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-7.7 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,
        URIBL_BLOCKED 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/18/2023 4:44 AM, Ilpo Järvinen wrote:
> Callers of get_cbm_mask() are required to pass a string into which
> the CBM bit mask is read into. Neither CAT nor CMT tests need the

There is a double "into" above. Perhaps the second can be dropped?

> mask as string but just convert it into an unsigned long value.
> 
> The bit mask reader can only read .../cbm_mask files.
> 
> Generalize the bit mask reading function into get_bit_mask() such that
> it can be used to handle other files besides the .../cbm_mask and
> handle the unsigned long conversion within within get_bit_mask() using
> fscanf(). Alter get_cbm_mask() to construct the filename for
> get_bit_mask().
> 
> Co-developed-by: Fenghua Yu <fenghua.yu@intel.com>
> Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> ---
>  tools/testing/selftests/resctrl/cat_test.c  |  5 +--
>  tools/testing/selftests/resctrl/cmt_test.c  |  5 +--
>  tools/testing/selftests/resctrl/resctrl.h   |  2 +-
>  tools/testing/selftests/resctrl/resctrlfs.c | 50 +++++++++++++++------
>  4 files changed, 40 insertions(+), 22 deletions(-)
> 
> diff --git a/tools/testing/selftests/resctrl/cat_test.c b/tools/testing/selftests/resctrl/cat_test.c
> index a998e6397518..9bf5d05d9e74 100644
> --- a/tools/testing/selftests/resctrl/cat_test.c
> +++ b/tools/testing/selftests/resctrl/cat_test.c
> @@ -18,7 +18,6 @@
>  #define MAX_DIFF		1000000
>  
>  static int count_of_bits;
> -static char cbm_mask[256];
>  static unsigned long long_mask;
>  static unsigned long cache_size;
>  
> @@ -101,12 +100,10 @@ int cat_perf_miss_val(int cpu_no, int n, char *cache_type)
>  	cache_size = 0;
>  
>  	/* Get default cbm mask for L3/L2 cache */
> -	ret = get_cbm_mask(cache_type, cbm_mask);
> +	ret = get_cbm_mask(cache_type, &long_mask);
>  	if (ret)
>  		return ret;
>  
> -	long_mask = strtoul(cbm_mask, NULL, 16);
> -
>  	/* Get L3/L2 cache size */
>  	ret = get_cache_size(cpu_no, cache_type, &cache_size);
>  	if (ret)
> diff --git a/tools/testing/selftests/resctrl/cmt_test.c b/tools/testing/selftests/resctrl/cmt_test.c
> index 2d434c03cbba..ae54bbabbd91 100644
> --- a/tools/testing/selftests/resctrl/cmt_test.c
> +++ b/tools/testing/selftests/resctrl/cmt_test.c
> @@ -17,7 +17,6 @@
>  #define MAX_DIFF_PERCENT	15
>  
>  static int count_of_bits;
> -static char cbm_mask[256];
>  static unsigned long long_mask;
>  static unsigned long cache_size;
>  
> @@ -82,12 +81,10 @@ int cmt_resctrl_val(int cpu_no, int n, char **benchmark_cmd)
>  	if (!validate_resctrl_feature_request(CMT_STR))
>  		return -1;
>  
> -	ret = get_cbm_mask("L3", cbm_mask);
> +	ret = get_cbm_mask("L3", &long_mask);
>  	if (ret)
>  		return ret;

I think this is a good change. It does raise the question why long_mask
is a global variable so I think it may make things go smoother if
the patch making long_mask local is moved to be before this patch.

Reinette