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: <eac72c7b-bd01-084e-cf23-ec5e8e0e3cf3@intel.com>
Date:   Fri, 21 Apr 2023 17:32:20 -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 24/24] selftests/resctrl: Rewrite Cache Allocation
 Technology (CAT) test
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-25-ilpo.jarvinen@linux.intel.com>
From:   Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <20230418114506.46788-25-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?SXNhNkdmVWc1dDBDN3BmUWl5T0plNDFrRzVySk9TL3RyQlJ3UERackpwR2FI?=
 =?utf-8?B?cytvbFZrZjdjWTJscVBlelRMeTVZYjlCNFlIdlM5RFhNVy8vNTA1aWFSYzZX?=
 =?utf-8?B?QWlnQ3VMdnZBaEdZZDhyZmVlT05DTDdKamlUcFBvdy9IM1lkSnNBcnFsNzB4?=
 =?utf-8?B?dWl5MG1aUW4wY0g0QlB2NVRpamdZSlhyRWRMQ0h6NWkxQVhrdWdlaDllMHIr?=
 =?utf-8?B?ZDdVU1VxeTVkUWpDZW8wNWd4ME00M2hLN1NyUS9ZWmYzVlJkc3EzeDJjODZE?=
 =?utf-8?B?d3dPYzlWVlBnVGhTY2JNMmVTOUkrZllENWo0TGp1a0l2dy9ObE9ZWDVMbnJk?=
 =?utf-8?B?dDVqaEVUTm8yWTJMd05qd3B2U0d0VWpwNEh6M0lDODFXby8zYklYaXdseWZZ?=
 =?utf-8?B?YzhxeW5qNDhvaXdVSEo4TzNkcjFHUDNPaTVyWE1FQkxiVXl3QkhBaHRDMCtM?=
 =?utf-8?B?UytUNkw1K0dvRjJJUmtpVzZWWG93L09LbVFxQ3lvNWdaVnJmaWlpOWltU0Yr?=
 =?utf-8?B?T2FjZTZPU2Y5eXdCVXBIZEplVUNTdUh6RlFJSmQ3SzMzbFVzOTZwZm5HVUp3?=
 =?utf-8?B?NzNubGdHejVuVHliYW1sNTlTdVE3dldsQmJOeXJOMm9qVmk0Q0xWUTFaODJ4?=
 =?utf-8?B?UVpmNWk4MzJSaVNDcmE4ZmVHVDBpMUhpR2N1cUh4MXZpTXIwdTZ5c2V6Sjh5?=
 =?utf-8?B?d3lNQXFGT2JsZjVnVVZ6NGYrdG9GZmZnM3FNanJ0R21qSGlIN2NDME1neVNG?=
 =?utf-8?B?bkxWUXN5YTVLYTBNc0hQQzhJL2hPQWJBdW5Qck1ndVNSc3JaTEhlN0ZSY3hy?=
 =?utf-8?B?QzhrdVlhU05nSThBM1pZaytQcmhZMklaalRRRmp3THMxUVhCY1lUMFlaZDU2?=
 =?utf-8?B?SG1JYUdNZVhaVWlyeEZqWVpOQWNWR3RrTmdjc2xvZ2w1Zm1hSnZnZWRHVU12?=
 =?utf-8?B?L0JNWXJkNkRFaXovTHAwalNBNXg1U0ExWWM3ck1wenBTU3BxSUdJYktEelBF?=
 =?utf-8?B?Z0lMNkMrS1hPbGFyRVBoaWx4eVpsRzhDekpoWWNpNnJHRXhZeE0vc2Z2d3RG?=
 =?utf-8?B?V01aQXE0dmgzdGFCVmRnZmNPSFQ4MklUeFBKWUh6Wk1pN1dSQUdzcjlnVXBM?=
 =?utf-8?B?ZXYxeHQyZGRTMys4Yi9ySDIwaDlHWnVrSkpXTnVyT1ZNTVRnT2o1VkFJbjZz?=
 =?utf-8?B?dnBuVWVIVU9lM1cwSFlnS1FHc0p4RjJFMGxDZ1JKRXRaMzdDdXVuN2V1R3BD?=
 =?utf-8?B?Y1d0UFMyYVRIcGZxVjRWYVBPTy9PWGZFS0JrTEFMR045c3U3TGUwYk40WktQ?=
 =?utf-8?B?TExpVEZ5ckx3UjJYekNudnhyZkJqMEpKQS9wR3pXRHhTUm5IYWt2aUMxcHFD?=
 =?utf-8?B?K0xMcFhKc0ZCWjEzckxrY0RqdXFPT2N0Zm5RQXFQcGhHMmVpaDd6clR2aHRp?=
 =?utf-8?B?eEZ5WG1SVzhSb0tpTnREdXE5QWNXL0F3azBsZnFYY2hZRXJieHZLOVRlZGd4?=
 =?utf-8?B?R1F4OXliSTk4cXlZUFdmc1lkRlFGdG00dDM2a3hBOHI2Y0g2aHJMUG5lL093?=
 =?utf-8?B?OEVnUzVxckRuR1hMZlA5M1J0V1kzOU02WDJrSlA3ZHdCaVFRZTdlM29WTFNa?=
 =?utf-8?B?NUh5VDdWVHA3aG96QU5JajB5azdHSzlaYTlNeFJOWDFEWEtyTW9vZEQreTN6?=
 =?utf-8?B?T0dLMWVERHAzdTRQWU1BUjFkQnJoM3JwWEpaOGxxamdZRHpjVXRISUlmMVhy?=
 =?utf-8?B?ODl6VlZ3cmRQajVMM2NFR0E3RjUxWkxCVTBQMWl2cVVFL3JKY0ZOVC9pQisv?=
 =?utf-8?B?RStJM25DR29aOGR2M043ci9mL2d6OThGdTdHWkJqdGdMdE9DS3U3dUZQWm5M?=
 =?utf-8?B?RVBNV0V3SWZnZmM0R2o3R2Z3UGtjY1N3TWpaSTZqdTl4Z0p3bkphaXpsTXBR?=
 =?utf-8?B?NXVPWE5XeUtvRVp4dTRmWEExMHFTUkNNRGdiQi96QWFZcEZpUGkvSWxtRDZo?=
 =?utf-8?B?aVNjZHd5T21RbVFJUWdnSHFJMnRIV3NsYkpOWmpRK1liK2xkcjlnU21wQUxC?=
 =?utf-8?B?TGZubmVXaFg2V0hPeko2WWo3Qm92enlDK24zUWo5SFFqYzhPUG96RmtUeUhs?=
 =?utf-8?B?UDlGU2w3Q2pqNS9Da0dvUGxoUzVlZzZScFl6RDJWaUExd3JnNUtueThZeGlv?=
 =?utf-8?B?b0E9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 5a7f7681-8e92-4e9c-d0ba-08db42c90a52
X-MS-Exchange-CrossTenant-AuthSource: DM5PR11MB1867.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 22 Apr 2023 00:32:23.6605
 (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: JPiBdE2S7DpWamXAUAstIra9dYcshZgviZmAMAcgm8PCwhVwEtfNGnHZswp/ZvSktzcsJOlMmIrWr8ZZyuX7NTUOfae+mBk6Cqxv4Moxq9w=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS0PR11MB7558
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,RCVD_IN_MSPIKE_H2,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:45 AM, Ilpo Järvinen wrote:
> CAT test spawns two processes into two different control groups with
> exclusive schemata. Both the processes alloc a buffer from memory
> matching their allocated LLC block size and flush the entire buffer out
> of caches. Since the processes are reading through the buffer only once
> during the measurement and initially all the buffer was flushed, the
> test isn't testing CAT.
> 
> Rewrite the CAT test to allocated a buffer sized to half of LLC. Then

"allocated a buffer" -> "allocate a buffer" ?

> perform a sequence of tests with different LLC alloc sizes starting
> from half of the CBM bits down to 1-bit CBM. Flush the buffer before
> each test and read the buffer twice. Observe the LLC misses on the
> second read through the buffer. As the allocated LLC block gets smaller
> and smaller, the LLC misses will become larger and larger giving a
> strong signal on CAT working properly.

Since the changelog starts by describing the CAT test needing two
processes I think it would help to highlight that this test uses a
single process. I think it would also help to describing how the cache
is used by the rest while this test is running.

> 
> Suggested-by: Reinette Chatre <reinette.chatre@intel.com>
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> ---
>  tools/testing/selftests/resctrl/cache.c    |  20 +-
>  tools/testing/selftests/resctrl/cat_test.c | 204 +++++++++------------
>  2 files changed, 97 insertions(+), 127 deletions(-)
> 
> diff --git a/tools/testing/selftests/resctrl/cache.c b/tools/testing/selftests/resctrl/cache.c
> index 7970239413da..64f08ba5edc2 100644
> --- a/tools/testing/selftests/resctrl/cache.c
> +++ b/tools/testing/selftests/resctrl/cache.c
> @@ -224,10 +224,10 @@ int measure_llc_resctrl(struct resctrl_val_param *param, int bm_pid)
>   */
>  int cat_val(struct resctrl_val_param *param)
>  {
> -	int memflush = 1, operation = 0, ret = 0;
>  	char *resctrl_val = param->resctrl_val;
>  	unsigned long llc_perf_miss = 0;
>  	pid_t bm_pid;
> +	int ret;
>  
>  	if (strcmp(param->filename, "") == 0)
>  		sprintf(param->filename, "stdio");
> @@ -245,6 +245,10 @@ int cat_val(struct resctrl_val_param *param)
>  	if (ret)
>  		return ret;
>  
> +	ret = alloc_buffer(param->span, 1);
> +	if (ret)
> +		return ret;
> +
>  	initialize_llc_perf();
>  
>  	/* Test runs until the callback setup() tells the test to stop. */
> @@ -256,17 +260,15 @@ int cat_val(struct resctrl_val_param *param)
>  		}
>  		if (ret < 0)
>  			break;
> +
> +		flush_buffer(param->span);
> +		use_buffer(param->span, 0, true);
> +
>  		ret = reset_enable_llc_perf(bm_pid, param->cpu_no);
>  		if (ret)
>  			break;
>  
> -		if (run_fill_buf(param->span, memflush, operation, true)) {
> -			fprintf(stderr, "Error-running fill buffer\n");
> -			ret = -1;
> -			break;
> -		}
> -
> -		sleep(1);
> +		use_buffer(param->span, 0, true);
>  
>  		/* Measure cache miss from perf */
>  		ret = get_llc_perf(&llc_perf_miss);
> @@ -279,6 +281,8 @@ int cat_val(struct resctrl_val_param *param)
>  			break;
>  	}
>  
> +	free_buffer();
> +
>  	return ret;
>  }
>  
> diff --git a/tools/testing/selftests/resctrl/cat_test.c b/tools/testing/selftests/resctrl/cat_test.c
> index 4b505fdb35d7..85053829b9c5 100644
> --- a/tools/testing/selftests/resctrl/cat_test.c
> +++ b/tools/testing/selftests/resctrl/cat_test.c
> @@ -11,11 +11,12 @@
>  #include "resctrl.h"
>  #include <unistd.h>
>  
> -#define RESULT_FILE_NAME1	"result_cat1"
> -#define RESULT_FILE_NAME2	"result_cat2"
> -#define NUM_OF_RUNS		5
> -#define MAX_DIFF_PERCENT	4
> -#define MAX_DIFF		1000000
> +#define RESULT_FILE_NAME		"result_cat"
> +#define NUM_OF_RUNS			5
> +#define MIN_DIFF_PERCENT_PER_BIT	2

Could you please start a new trend that adds documentation
that explains what this constant means and how it was chosen?

> +
> +static unsigned long current_mask;
> +static long prev_avg_llc_val;
>  
>  /*
>   * Change schemata. Write schemata to specified
> @@ -28,13 +29,24 @@ static int cat_setup(struct resctrl_val_param *p)
>  	int ret = 0;
>  
>  	/* Run NUM_OF_RUNS times */
> -	if (p->num_of_runs >= NUM_OF_RUNS)
> -		return END_OF_TESTS;
> +	if (p->num_of_runs >= NUM_OF_RUNS) {
> +		/* Remove one bit from the consecutive block */
> +		current_mask &= current_mask >> 1;
> +		if (!current_mask)
> +			return END_OF_TESTS;
> +
> +		p->num_of_runs = 0;

This seems like a workaround to get the schemata to be written. It is
problematic since now p->num_of_runs no longer accurately reflects the
number of test runs. I was expecting this mask manipulation to be
in cat_val() so that it is clear how test works instead of part
of the logic handled here.

> +	}
>  
>  	if (p->num_of_runs == 0) {
> -		sprintf(schemata, "%lx", p->mask);
> -		ret = write_schemata(p->ctrlgrp, schemata, p->cpu_no,
> -				     p->resctrl_val);
> +		snprintf(schemata, sizeof(schemata), "%lx", p->mask & ~current_mask);
> +		ret = write_schemata("", schemata, p->cpu_no, p->resctrl_val);
> +		if (ret)
> +			return ret;
> +		snprintf(schemata, sizeof(schemata), "%lx", current_mask);
> +		ret = write_schemata(p->ctrlgrp, schemata, p->cpu_no, p->resctrl_val);
> +		if (ret)
> +			return ret;
>  	}
>  	p->num_of_runs++;
>  

...

> @@ -126,7 +162,7 @@ int cat_perf_miss_val(int cpu_no, int n, char *cache_type)
>  	ret = get_mask_no_shareable(cache_type, &long_mask);
>  	if (ret)
>  		return ret;
> -	count_of_bits = count_consecutive_bits(long_mask, NULL);
> +	count_of_bits = count_consecutive_bits(long_mask, &start);
>  
>  	/* Get L3/L2 cache size */
>  	ret = get_cache_size(cpu_no, cache_type, &cache_size);
> @@ -143,99 +179,29 @@ int cat_perf_miss_val(int cpu_no, int n, char *cache_type)
>  			       count_of_bits - 1);
>  		return -1;
>  	}
> -
> -	/* Get core id from same socket for running another thread */
> -	sibling_cpu_no = get_core_sibling(cpu_no);

Do any users of get_core_sibling() remain after this?


Reinette