Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.37 as permitted sender) client-ip=23.128.96.37;
Message-ID: <12945b6d-0cc5-4dda-97b3-ec2b597f6ad5@intel.com>
Date:   Thu, 2 Nov 2023 10:48:22 -0700
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 07/24] selftests/resctrl: Split measure_cache_vals()
 function
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: <20231024092634.7122-1-ilpo.jarvinen@linux.intel.com>
 <20231024092634.7122-8-ilpo.jarvinen@linux.intel.com>
From:   Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <20231024092634.7122-8-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?OXhYcXZPbmovMWRwWXZPQWRqV01vWS9lcFZGTG5MaThYaEl3S1pzelA5U0lw?=
 =?utf-8?B?dlUyeHZFUVNNUFJkVkdTM21wd2hnTFdWbUV2bnBoRm0wUXFaLzdBdnJSdC9l?=
 =?utf-8?B?dU9RWnZSeUdITGY1UllSMG03NC9aR2Qyb09pZG5GblpTdytxQUxCQlBYelJh?=
 =?utf-8?B?YkkwZ3BiTWgvSE40dEl1ZGNvZk1mOWw5S0pIeEJxYmdjT0JMZG1WZ1hmVkVu?=
 =?utf-8?B?dGdyMTl3T0JYVUNiN0VRMlJ4KzlzNkdEbE5kMkRIb0Q3Q0JMdXczamdkM0RW?=
 =?utf-8?B?R2pZMm5qM0FKbVFWL0tLdlZTNjBnYlhrdUUwdC9NbXNmaHFlNVovdE5JUGZ4?=
 =?utf-8?B?TlhWc0JzZWJTZmE2V0pTa0REcnNIaGIzcUlNellzaUk0em54TXN3TWVWbDd0?=
 =?utf-8?B?b3YxZWJmdjhTdGZuS3N6SnQ0a1hINUN1cUEvY2dWajNlZ3dHazJ3UE9VQy9F?=
 =?utf-8?B?cHpnZ3lZYUNzRHk0UVJQNzE3Z1NMeDF6OGlQOFBhOUIvSStSL1R0bFhLN29k?=
 =?utf-8?B?VVFLS0xERGZoUG9UWnV5NXdZbWd0aU02Sk95aVp5SVg0T1VjTmtzNUNZU2dT?=
 =?utf-8?B?R0RTT01ZUXdtNlZMUm1PL1JaNjZWWFVmS3NKZmtvb2xwSjR6Z1hXN0ZQUFkv?=
 =?utf-8?B?M2FGVmtFdjRreGlIbktjVXpMYjBwSnJpWWpYZFRpUHlyUzRMdkd1RG90UjJ2?=
 =?utf-8?B?dnNubmhPUjhZd2E3UlFQOW11NlBSS216Zy9KUTl3cXJDcGNCRnJLRlJ1YktD?=
 =?utf-8?B?YitSZ2s0eU1JL0RRNUY0VVl0TjVRSnd3a3ArazdWc1dQTmJNdHhFc3NRckFH?=
 =?utf-8?B?S2llQTBsYU1oWGJXK1ZuSGcwRjMxTU5xeFRmQmkyaEVITUpEeTY5M1ZvWEMr?=
 =?utf-8?B?MTA1VWM3MHBNcGZ5b3JpWlUvcklXc3N1K0VLUllrRm5mekpPWHhJemRrUngz?=
 =?utf-8?B?R0hvaytZdDE2aWdHd3U4UUVyMWZ3LzZFWVJOTkFlNG0rK0NPRWZFRXB5eXJo?=
 =?utf-8?B?ZlFkYWRHNFU0WHIrZlNCRmxTdnVmS1FGQ1Q1ZEdIWmZsOS9xdjFhRWFYc1Jq?=
 =?utf-8?B?ZmVtU2lPL25zZURoSXRoWlgvL3VxaFpiR21WaGIvQlJGZ0MrMmVEYnUyakQ0?=
 =?utf-8?B?bExYMFlycUNPeW1KN3BmVW1sRjBJazRUZVZRbU9VZVJVdnV6aXJyeGRnNFhk?=
 =?utf-8?B?alNJUWFlUHNsakVjOTBGYmVUbURFM29EMXZ1TkhaYllNK3I2RS8zTTlsZnYx?=
 =?utf-8?B?WnZsRUdpSVdjcjcxamJxVWh1TUpzTUZMZDBmNkRyQ3A3ZWc2cDRJYnp0SEkz?=
 =?utf-8?B?TFZCRlROVTRUNTFuY0NHSUtXRmpQVjZxTmVtZFkrSUg1QUtJTTAyTk1YQzJ3?=
 =?utf-8?B?eUtFV3QvZVgzWHhoRFVHOGFuZzZOdVk5Q0hyZDRGeS9oMnFkOXZadVEvcE5j?=
 =?utf-8?B?R0VtZ0Nqc1FrVXJPODVoWFJ5VDJpb1pkajNGUThwVW5lZEdZNGljR0xEWGRU?=
 =?utf-8?B?OU43MjVHNGxNckdHVGdaNXhUbjFwZXJuS3lKRmZBNjY5SnVWUDByRjBTWDRH?=
 =?utf-8?B?aHFqSGZUSVpCT3J1WXBDcjZrNCsybCtBcjc3ZStEU2NxOFBOQ3l5Rm9jditJ?=
 =?utf-8?B?aGtKdEprbUJhbnJnM0U4Z1d1VjIzZGVzOFlQTmF5d0lZV09pTi8wU1lhaEVt?=
 =?utf-8?B?eHZiNzQzYUtiUGIrZnFKazZWWTNpT1JmNHhCYVcxTHNvTDd1a1NlWFo1NGp3?=
 =?utf-8?B?aWN1SDV4YWxxbmNSNC9hbE1KeTYwTGNYUlgzaCtFeit6d2VJK05FMEhmdngy?=
 =?utf-8?B?KzVrN2J0RnZnOEo3dDFGZDNqd21aSVBEL1lCaVFiaWdXYUM2elZYbjRmRDJB?=
 =?utf-8?B?aDFRR2Zabllsd1UxWlFjV05lSjhFQnplRk53Y2FVY3ZJcGtwVmJEK1NwUFRI?=
 =?utf-8?B?Um5nMk5NWnJWVzdxbkR0TlpVbmZiTDljN0JOMkhXYVVveUtUY2t5ZGxISzdm?=
 =?utf-8?B?OU02MDRPNzV4VmZSNDZwcmRuUVUzNWVJT1ZrdFUrS1ZsNk9DeHpxMVVrc2lU?=
 =?utf-8?B?ZXBSMGZ2eVhyYWxvVmxuUkVFd3lYZ1NlU28zazgwSlhDSFJ6YmRZcmJhemdC?=
 =?utf-8?B?TDRBbkpaMXZxQ2thUWpkZk81UFFyYUpic3VrNlk4cXBWaGYxWFpXdG5uTzV1?=
 =?utf-8?B?VlE9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: f0c1de1b-218a-4144-f329-08dbdbcbe86a
X-MS-Exchange-CrossTenant-AuthSource: SJ2PR11MB7573.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 02 Nov 2023 17:48:22.8266
 (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: ieANnHMLJJl5efElTG8rS3IG6esoS6nTgVxso5h5zaIwK8t3ErZKHSIyMjuDBug4/0qNINqsmHQxbVM9cp1kpiUmOImbkjUChwExidDCG70=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS7PR11MB7949
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-2.5 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,
        RCVD_IN_DNSWL_BLOCKED,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
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (snail.vger.email [0.0.0.0]); Thu, 02 Nov 2023 10:48:40 -0700 (PDT)

Hi Ilpo,

On 10/24/2023 2:26 AM, Ilpo Järvinen wrote:
> The measure_cache_vals() function does a different thing depending on

No need to say "function" when using "()". The above can just be:
"measure_cache_vals() does a different ..."

> the test case that called it:
>   - For CAT, it measures LLC perf misses.
>   - For CMT, it measures LLC occupancy through resctrl.
> 
> Split these two functionalities into own functions the CAT and CMT
> tests can call directly.
> 
> 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/cache.c       | 37 ++++++++++---------
>  tools/testing/selftests/resctrl/resctrl.h     |  2 +-
>  tools/testing/selftests/resctrl/resctrl_val.c |  2 +-
>  3 files changed, 21 insertions(+), 20 deletions(-)
> 
> diff --git a/tools/testing/selftests/resctrl/cache.c b/tools/testing/selftests/resctrl/cache.c
> index bcbca356d56a..299d9508221f 100644
> --- a/tools/testing/selftests/resctrl/cache.c
> +++ b/tools/testing/selftests/resctrl/cache.c
> @@ -170,35 +170,36 @@ static int print_results_cache(char *filename, int bm_pid,
>  	return 0;
>  }
>  
> -int measure_cache_vals(struct resctrl_val_param *param, int bm_pid)
> +static int measure_llc_perf(struct resctrl_val_param *param, int bm_pid)
>  {
> -	unsigned long llc_perf_miss = 0, llc_occu_resc = 0, llc_value = 0;
> +	unsigned long llc_perf_miss = 0;
>  	int ret;
>  
>  	/*
>  	 * Measure cache miss from perf.
>  	 */

I'd expect these comments to move as part of a change like this,
but seems like this is merged with other changes in patch 10. Does not
seem like an issue done in this way.

> -	if (!strncmp(param->resctrl_val, CAT_STR, sizeof(CAT_STR))) {
> -		ret = get_llc_perf(&llc_perf_miss);
> -		if (ret < 0)
> -			return ret;
> -		llc_value = llc_perf_miss;
> -	}
> +	ret = get_llc_perf(&llc_perf_miss);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = print_results_cache(param->filename, bm_pid, llc_perf_miss);
> +	return ret;
> +}
> +
> +int measure_llc_resctrl(struct resctrl_val_param *param, int bm_pid)
> +{
> +	unsigned long llc_occu_resc = 0;
> +	int ret;
>  
>  	/*
>  	 * Measure llc occupancy from resctrl.
>  	 */
> -	if (!strncmp(param->resctrl_val, CMT_STR, sizeof(CMT_STR))) {
> -		ret = get_llc_occu_resctrl(&llc_occu_resc);
> -		if (ret < 0)
> -			return ret;
> -		llc_value = llc_occu_resc;
> -	}
> -	ret = print_results_cache(param->filename, bm_pid, llc_value);
> -	if (ret)
> +	ret = get_llc_occu_resctrl(&llc_occu_resc);
> +	if (ret < 0)
>  		return ret;
>  
> -	return 0;
> +	ret = print_results_cache(param->filename, bm_pid, llc_occu_resc);
> +	return ret;
>  }
>  
>  /*
> @@ -253,7 +254,7 @@ int cat_val(struct resctrl_val_param *param, size_t span)
>  		}
>  
>  		sleep(1);
> -		ret = measure_cache_vals(param, bm_pid);
> +		ret = measure_llc_perf(param, bm_pid);
>  		if (ret)
>  			goto pe_close;
>  	}


Reinette