Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:5 as permitted sender) client-ip=2620:137:e000::3:5;
Message-ID: <04650d92-fb80-49e2-87b7-4f9dbec922b8@intel.com>
Date:   Wed, 13 Dec 2023 13:59:19 -0800
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v3 23/29] selftests/resctrl: Restore the CPU affinity
 after CAT test
Content-Language: en-US
To:     =?UTF-8?Q?Ilpo_J=C3=A4rvinen?= <ilpo.jarvinen@linux.intel.com>,
        <linux-kselftest@vger.kernel.org>, Shuah Khan <shuah@kernel.org>,
        "Shaopeng Tan" <tan.shaopeng@jp.fujitsu.com>,
        =?UTF-8?Q?Maciej_Wiecz=C3=B3r-Retman?= 
        <maciej.wieczor-retman@intel.com>,
        Fenghua Yu <fenghua.yu@intel.com>
CC:     <linux-kernel@vger.kernel.org>
References: <20231211121826.14392-1-ilpo.jarvinen@linux.intel.com>
 <20231211121826.14392-24-ilpo.jarvinen@linux.intel.com>
From:   Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <20231211121826.14392-24-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?T0N4M1pTZTVrS0tkRUcwQjFSdjRXYzBYQ2k0ZmNVUUNFRHIzNVdyZkducHh6?=
 =?utf-8?B?QSs3N2JCZUNPOGhHS2tuUkRsWkthek81ckYvSHcvaTI4MThzYlFpMnR5TTVa?=
 =?utf-8?B?L2gwb0VwZ2UvMFVzMlpnRlVGUjNrR2RCY1VvaTh2TzNBNUpCV0RZaHpQYlF6?=
 =?utf-8?B?M0V4c3prd0RYVHdJYkxqOVdpRUR0NTZvZXBqSFZVSnQzMWx5bnE1UXR3NWE3?=
 =?utf-8?B?RXMveVYvejFZTThwL2VZOVBZaFdpcEIrS3p5eUxXcE9keHNnZ1hhNThVdzlN?=
 =?utf-8?B?K1ZsSXlMNzFnYk9XN05Ua3N0V1RZc1hqT2toc2QzTG9peTVtdlF1bGNkK1JJ?=
 =?utf-8?B?a1c5bDdua2h2alFoN2lhdkl6ei8zUndmVWFGdkpJNjh0T29TTndzcC9qYmRH?=
 =?utf-8?B?eUE5ZkZrV001aHRtckh4RUJaK0JTK0dVb01mcGpvOGxOSlVlbU9RNTFWcXc2?=
 =?utf-8?B?cVdrQVo1SFVXVzU4TnlNTWw2aWJHanlpVEwzOUlUU1E2dExwRFBuSHZ3SXg2?=
 =?utf-8?B?Z2w4clU4MlJlMWt1ZkI3T3hlQ1BzaFJnaWtCOTJ1ek8wQ2FsYUp2UWNuQVZW?=
 =?utf-8?B?akIxbUpXUlNHL0t3Z05LUjlla3VPbzk1cExheXRhZ1V4ODRBTXMzTjRhQkFV?=
 =?utf-8?B?TmUvUmd0L3NlTDQweTVHR3FnOW1BQWYvYnhic2tRd09QOTRRYXFtZDdEYWRt?=
 =?utf-8?B?clQwWGtySkRnUVNoQ0xlY2dmd21RbVgvZ1VSZWJFRlk0aDJqTmg1Z3NaYXRP?=
 =?utf-8?B?b0VNU3FJQ3NmV2s5Z25oR1F3ckkyTmJ0Q3FxNUlkVXRlMVB1YnU0WWcrWnk4?=
 =?utf-8?B?bUpjWVpWU2ZpL25yaXhwbnFDUHdUV2x2d3ZSRWRRaEJjVDVUY2dwdldiUk9H?=
 =?utf-8?B?TEEzUERzK2ROcHVWRWlXSWUvNjhCOXZxNk9HUEFka0ZQM2s1bUhmU0I5eGdh?=
 =?utf-8?B?L2MzV2Z5dEg3YzZ4Q3E0eWdKUFU4RTljd1BrejVRMEphQlFVQ2ZPU1hmWmQz?=
 =?utf-8?B?RjJLa0NRR0txOGxHc0hmeHVwRXY2SGVTcDBpb01Ucjh0RDJRWGZMT3RwUTcy?=
 =?utf-8?B?MGp3eng3QVVSeFkrN04zNTVpc25tRlQvdnBMVDIxSXBPdExXUUVJdm1XNnZ6?=
 =?utf-8?B?WStnRjBJTEZ6QnFlczYwM0ZUTURhL0s4THN2L3FWNEx5TTh3bVRlMHFJMTlO?=
 =?utf-8?B?SEgwRFYxTi91SE9qSkYxYmt1NXp6TWRjbkM5Uy91RFhSQk1NMGluV3I1cGc5?=
 =?utf-8?B?bDJ3d2U2Z1R0QldOWmFwUHRjcmh0bWc4SVlMN1ZZRWRIZCtKVFdhdlVnWGh6?=
 =?utf-8?B?WE5SWXFySENXNUZmeE5jQmNtemVXSVFNbW5zYnl2Z1RoZUFCYUVZbnB4Y1Vv?=
 =?utf-8?B?eGVMeXhNSWZ2RC9DcElpVURYTllrZjgycTU4OWhqT1o2WktjcGRyWGlQbW95?=
 =?utf-8?B?MGk3Uy9Dd1U0N2lwQzRSUGVnQWRaSUpIUjhkbXc0YjhScjM1VkNsS3IvenlZ?=
 =?utf-8?B?T3duTG9rc3pOdUw4eXh0YzBpRC9aKzBhSk9JdEVwZWZGMXZSMm1zKzgrbjdD?=
 =?utf-8?B?akdVclBrZVVTcmhnR2ZGVE80ai81bWQ1U2g1Z1p6ZzMwVVVmSStqZVpKNzZI?=
 =?utf-8?B?V2tZMFZMMWI0c2swUXh0T0hiWmRQM0FyczJqL2NrdkU5UkNCOVM2SWlaKzY2?=
 =?utf-8?B?Qm1PcTRqTEQwZ2hqUVgrUmZNR0dJdjd4bmZDSnpZVE5XdDFQb29wK0cxYW5i?=
 =?utf-8?B?Y0NlTnJkcW9MNzViSHBRUHZZVVM2R1JHc2o0eTNwU1RmYUx4YlFRdmVGV0V2?=
 =?utf-8?B?TWgvei8vSTIwalZtWkxOdzhIZFRvUzIxRi9SUmRpeUlIbVkvSTV5VTJRVE1Z?=
 =?utf-8?B?VGFyOFZQMU1HUXZzbG54TW5ZSE4xbTJOeXN1anVFR0NvbnpESWFCWERlajdB?=
 =?utf-8?B?WGIyRTBuRU5MZUV5amw1eGRmeGEzR3pucGRLbDREdUlnUXRwKy9ydDFpMlJy?=
 =?utf-8?B?MURGQm1DTVJVM3U1UkpoVUJ1R2V2NWNialplWGozRWFXeWxoL3BYaXh4TDlu?=
 =?utf-8?B?OVBlYlJDeXJjSnp6YW4xOVhMMVVrSjJvdlNrMXMxeEo0UVpZR29LRWlRZ2RS?=
 =?utf-8?B?VEtuZlZ4MGczNUx2MG5KWnpvQU5nY1hXTHY2YlRhSERNTWc3UXpKbkd4V1do?=
 =?utf-8?B?a3c9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 95c513e1-3996-41fd-0a79-08dbfc26c7c7
X-MS-Exchange-CrossTenant-AuthSource: SJ2PR11MB7573.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 13 Dec 2023 21:59:29.5394
 (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: WA0EwAJzknR3cKdRD3vq44C+v6Pao5/zPJCHRW6cJo6UKswPHKyZyCuKRRR4TBMtaYlBWwZla2RzgAteMKAKWYl5Bik9Hvi9BDLU4jHZoE8=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH0PR11MB5925
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-0.9 required=5.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED,
	DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,
	MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE
	autolearn=unavailable autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on groat.vger.email
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (groat.vger.email [0.0.0.0]); Wed, 13 Dec 2023 13:59:49 -0800 (PST)

Hi Ilpo,

On 12/11/2023 4:18 AM, Ilpo Järvinen wrote:
> CAT test does not reset the CPU affinity after the benchmark.
> This is relatively harmless as is because CAT test is the last
> benchmark to run, however, more tests may be added later.
> 
> Store the CPU affinity the first time taskset_benchmark() is run and
> add taskset_restore() which the test can call to reset the CPU mask to
> its original value.
> 
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> ---
> 
> v3:
> - New patch
> ---
>  tools/testing/selftests/resctrl/cat_test.c    | 13 +++++---
>  tools/testing/selftests/resctrl/resctrl.h     |  3 +-
>  tools/testing/selftests/resctrl/resctrl_val.c |  2 +-
>  tools/testing/selftests/resctrl/resctrlfs.c   | 33 +++++++++++++++++--
>  4 files changed, 42 insertions(+), 9 deletions(-)
> 
> diff --git a/tools/testing/selftests/resctrl/cat_test.c b/tools/testing/selftests/resctrl/cat_test.c
> index b79916069788..fa95433297c9 100644
> --- a/tools/testing/selftests/resctrl/cat_test.c
> +++ b/tools/testing/selftests/resctrl/cat_test.c
> @@ -156,6 +156,7 @@ static int cat_test(struct resctrl_val_param *param, size_t span, unsigned long
>  	char *resctrl_val = param->resctrl_val;
>  	struct perf_event_read pe_read;
>  	struct perf_event_attr pea;
> +	cpu_set_t old_affinity;
>  	unsigned char *buf;
>  	char schemata[64];
>  	int ret, i, pe_fd;
> @@ -167,7 +168,7 @@ static int cat_test(struct resctrl_val_param *param, size_t span, unsigned long
>  	bm_pid = getpid();
>  
>  	/* Taskset benchmark to specified cpu */
> -	ret = taskset_benchmark(bm_pid, param->cpu_no);
> +	ret = taskset_benchmark(bm_pid, param->cpu_no, &old_affinity);
>  	if (ret)
>  		return ret;
>  
> @@ -175,13 +176,15 @@ static int cat_test(struct resctrl_val_param *param, size_t span, unsigned long
>  	ret = write_bm_pid_to_resctrl(bm_pid, param->ctrlgrp, param->mongrp,
>  				      resctrl_val);
>  	if (ret)
> -		return ret;
> +		goto reset_affinity;
>  
>  	perf_event_attr_initialize(&pea, PERF_COUNT_HW_CACHE_MISSES);
>  	perf_event_initialize_read_format(&pe_read);
>  	pe_fd = perf_open(&pea, bm_pid, param->cpu_no);
> -	if (pe_fd < 0)
> -		return pe_fd;
> +	if (pe_fd < 0) {
> +		ret = -1;
> +		goto reset_affinity;
> +	}
>  
>  	buf = alloc_buffer(span, 1);
>  	if (!buf) {
> @@ -220,6 +223,8 @@ static int cat_test(struct resctrl_val_param *param, size_t span, unsigned long
>  	free(buf);
>  pe_close:
>  	close(pe_fd);
> +reset_affinity:
> +	taskset_restore(bm_pid, &old_affinity);
>  
>  	return ret;
>  }
> diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h
> index da1f1b508aee..da62f4cd5add 100644
> --- a/tools/testing/selftests/resctrl/resctrl.h
> +++ b/tools/testing/selftests/resctrl/resctrl.h
> @@ -98,7 +98,8 @@ int umount_resctrlfs(void);
>  int validate_bw_report_request(char *bw_report);
>  bool validate_resctrl_feature_request(const char *resource, const char *feature);
>  char *fgrep(FILE *inf, const char *str);
> -int taskset_benchmark(pid_t bm_pid, int cpu_no);
> +int taskset_benchmark(pid_t bm_pid, int cpu_no, cpu_set_t *old_affinity);
> +int taskset_restore(pid_t bm_pid, cpu_set_t *old_affinity);
>  int write_schemata(char *ctrlgrp, char *schemata, int cpu_no,
>  		   char *resctrl_val);
>  int write_bm_pid_to_resctrl(pid_t bm_pid, char *ctrlgrp, char *mongrp,
> diff --git a/tools/testing/selftests/resctrl/resctrl_val.c b/tools/testing/selftests/resctrl/resctrl_val.c
> index d515850cc174..4aed974efa0f 100644
> --- a/tools/testing/selftests/resctrl/resctrl_val.c
> +++ b/tools/testing/selftests/resctrl/resctrl_val.c
> @@ -777,7 +777,7 @@ int resctrl_val(const char * const *benchmark_cmd, struct resctrl_val_param *par
>  	value.sival_ptr = (void *)benchmark_cmd;
>  
>  	/* Taskset benchmark to specified cpu */
> -	ret = taskset_benchmark(bm_pid, param->cpu_no);
> +	ret = taskset_benchmark(bm_pid, param->cpu_no, NULL);
>  	if (ret)
>  		goto out;
>  
> diff --git a/tools/testing/selftests/resctrl/resctrlfs.c b/tools/testing/selftests/resctrl/resctrlfs.c
> index dffe42e11c6c..97760fadcddf 100644
> --- a/tools/testing/selftests/resctrl/resctrlfs.c
> +++ b/tools/testing/selftests/resctrl/resctrlfs.c
> @@ -345,15 +345,25 @@ int get_mask_no_shareable(const char *cache_type, unsigned long *mask)
>  
>  /*
>   * taskset_benchmark - Taskset PID (i.e. benchmark) to a specified cpu
> - * @bm_pid:	PID that should be binded
> - * @cpu_no:	CPU number at which the PID would be binded
> + * @bm_pid:		PID that should be binded
> + * @cpu_no:		CPU number at which the PID would be binded
> + * @old_affinity:	When not NULL, set to old CPU affinity
>   *
>   * Return: 0 on success, < 0 on error.
>   */
> -int taskset_benchmark(pid_t bm_pid, int cpu_no)
> +int taskset_benchmark(pid_t bm_pid, int cpu_no, cpu_set_t *old_affinity)
>  {
>  	cpu_set_t my_set;
>  
> +	if (old_affinity) {
> +		CPU_ZERO(old_affinity);
> +		if (sched_getaffinity(bm_pid, sizeof(*old_affinity),
> +				      old_affinity)) {
> +			ksft_perror("Unable to read previous CPU affinity");

"previous" can be confusing here (it is not trying to determine something
from the past but instead the current state). It can just be "Unable to read
CPU affinity"

> +			return -1;
> +		}
> +	}
> +
>  	CPU_ZERO(&my_set);
>  	CPU_SET(cpu_no, &my_set);
>  
> @@ -366,6 +376,23 @@ int taskset_benchmark(pid_t bm_pid, int cpu_no)
>  	return 0;
>  }
>  
> +/*
> + * taskset_restore - Taskset PID to the earlier CPU affinity
> + * @bm_pid:		PID that should be reset
> + * @old_affinity:	The old CPU affinity to restore
> + *
> + * Return: 0 on success, < 0 on error.
> + */
> +int taskset_restore(pid_t bm_pid, cpu_set_t *old_affinity)
> +{
> +	if (sched_setaffinity(bm_pid, sizeof(*old_affinity), old_affinity)) {
> +		ksft_perror("Unable to restore taskset");

This message is not clear to me. How about "Unable to restore CPU affinity"?

> +		return -1;
> +	}
> +
> +	return 0;
> +}
> +
>  /*
>   * create_grp - Create a group only if one doesn't exist
>   * @grp_name:	Name of the group

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

Reinette