Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:7 as permitted sender) client-ip=2620:137:e000::3:7;
Message-ID: <5d504c29-896d-451a-bf85-eaef315bb822@intel.com>
Date:   Thu, 2 Nov 2023 10:48:43 -0700
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 08/24] selftests/resctrl: Split show_cache_info() to test
 specific and generic parts
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-9-ilpo.jarvinen@linux.intel.com>
From:   Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <20231024092634.7122-9-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?ODdXd0J5eFN6MkliUUV1SE5sdUpPV2ZoQzBhZFdWN0wvNkRKSDdXc2w5Wkpz?=
 =?utf-8?B?VStGYmQ2YXVjVGQvUzNZb1kwV3ZwUTdnS001WUxXMHpUMXB0b1RkYmxQSFFN?=
 =?utf-8?B?WjU0bVVZaW1Gd1VLcFRLakFXR09hM0JlekFtQVdVbnNyYjhzRU1iUnRjNFlk?=
 =?utf-8?B?NXBvTUcyc2xTM2UvTXZOOEJOUGZ0OVJuRHExV0ZKUzQ0S1o4d2NRd3JxNkJz?=
 =?utf-8?B?WExoS0RCQTdzUkFsNkZzRkVTRFVFYTV6aVY1NnZva201clRLT0lvaDE4SFBM?=
 =?utf-8?B?MHFMaUYrN2NCR014Zlc3czNCZU9OdEZJelFpMHJ1SkE3YXZIcDB2QVRzYjBK?=
 =?utf-8?B?WUtRLzcxRCtNN0dsb0syUmJocE5XempRUFkrZFJEQXE1cDdNQVZyayt5a2VS?=
 =?utf-8?B?Z3ZsTE5xaGswd1N6NEdzSTdEVWE3TXdlK3pvTEJmcEM5MDdpWTlDdFJHUzNs?=
 =?utf-8?B?cWRmSCt6L0czN2huYkg0dHdCR0lkcHFuRml0dmJZcnB5QTZXWndWT3NGYlYr?=
 =?utf-8?B?TnQrMU96MXZWYjhqUlY3ZGRvb3VjbStnN1ZzdlMwbzZJVUYvQ2VUbnpRYjQv?=
 =?utf-8?B?RTRNcG53dlV5Y0xCc0xPSEJDL0lUSUFtT1RsNnBhOWoxWWFGVUd5V3hDK0pP?=
 =?utf-8?B?OHZveVZuODZYZDFiV0tRVFo0UVk1RHcxUWlIaE1ZMWhxdVNzeit4Zms3dnBD?=
 =?utf-8?B?c2QvanNMNHZiNDJNOUxxWm1RRjE3b0VRVFhpSjZWZHdqOEErQXBjSGt3amVO?=
 =?utf-8?B?YStZWDFIaVZGRlVPRmJCdk9qMWNjVmlOWmxJUjk3eXZVYnFGZnRtSTdjYTcz?=
 =?utf-8?B?Zkw2d0h4ZitKaWpWSGtwcEVHMWJZU1lhMmxVSm9Jc1hmbXlEZkVKUUY0ckpW?=
 =?utf-8?B?UnZDMUsvZUNoWkpsdDZTV0xqQXJlUGk3Z2dpZmFrWlJCZW52bmZhbTlCSVRE?=
 =?utf-8?B?ZytqYWk4aWZNcEgyT3ZudkFab3NaNDVzMDl5b1hGYjVNR1VNWlZrd3Jrb0ZI?=
 =?utf-8?B?aGdYTTdHdy9TOTJubHVnT05uZllaTFhtaGJjRWU4b2d2Ky9OOVNRK0lzYjJv?=
 =?utf-8?B?S0tXZkFpTGFzOGR2eVBheUN3M1grYkl2dkRRaElYejNWVW8yR2E2eHpVdWh3?=
 =?utf-8?B?SmdvL0VWSWFJZVVDdER4Q3hSdXlzbjI1Zk5KZHk0VTN6S2xnSFZ0Z20vKzlQ?=
 =?utf-8?B?QXBmYjFPNU9TbXpsdzd4L0RVOHVNSGVtNXYvUmM1eTR0WjlVQ1RoajdXRGhr?=
 =?utf-8?B?VG05aVdxR1VCY1BGL203UHc4aWtrdTMzeXh0T3htUms3SGlUWEwyNUlBL1B3?=
 =?utf-8?B?dlIvZmhrcjhBQ2E1SU9naWR4ZWFmc1czaTQ1MlRtaUZUd0tZKytkSTl0TzY2?=
 =?utf-8?B?YVY4R1luc3FheUpYZm9mL1ZhWFYrd3BVN0dPSWoyNzMwMDhwUElONmFKMzBS?=
 =?utf-8?B?cTJYVytEcXJ4VFB6TG1ROUp4SHBveDVrY0ZuTmJPL2ZtSDd3Z1l2ZnBDcm4r?=
 =?utf-8?B?U3NST1I2SlNwSUl3Y1hlbGRVQlFSNDNGNkhzVDlKMkRieXdxQlpPeVplOWtN?=
 =?utf-8?B?UnJNeFI4NWhza3ZvZUE2ZktSbDE4TDVjQVdvaFlrOFl5bzN2QXZtcml5U1FH?=
 =?utf-8?B?ajl1c0NDY3RRUnltblljckRqTkl3dXl1V1RENHpYemNrMUZ0eGZsYnd6ZzlX?=
 =?utf-8?B?VVJUL0xCV1RFaGw0UERMTWt6ckZGK0tWUHBZbmQ1eUZGWk5oVWgyckxtV1Rt?=
 =?utf-8?B?dXlWRnFhRGdrZ1ZicWdIcHdqYXJwbkpqY0tORWV0Zy9hbDJvY1k0Vlc3L05Z?=
 =?utf-8?B?dW1BTGhTaFRRL1RvaThROHorc0JVakRaMXBqM2ttS1RTaDBlTU93VlE2OXZy?=
 =?utf-8?B?RnEvYWplVDFuRmVHWndKVW5nK1hJeUt1N2dUUyt2Z3BKV3lwZHYvd09TckRx?=
 =?utf-8?B?N0pHUTBVaFhnUytSU2xEcFErcWFGSFpxbnBlYmo2WjBYRUZ1WHBFOHNKblM5?=
 =?utf-8?B?ejdGU2FHTWgwajd2cUtLT2swWFQxeXVFUnNGV0lBSHJzOG1KbC9EWjFUMFlQ?=
 =?utf-8?B?L0lJNFRZb0YxVnFKTmkvczVBWGJObjJBaytyK09wamVNK1RlM2JlL096OXNY?=
 =?utf-8?B?MndaaTRtbXdHKzdBS0JWelM5U1JWU1FVdzBkdFd2eGVDeVkxeU5tV2dBSkxL?=
 =?utf-8?B?Z0E9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 93a36755-0bd0-437a-2c02-08dbdbcbf507
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:43.9864
 (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: Ms3AIwaypjnHZUDKILKoWP0d5gCpCWb5UEIhyC73IecZ0Z07nAIY451TqPKCmNEDO+uhVPwV0IyuEXg+43EnrNV9PtfZMp0UTYsTCeE2qKE=
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:59 -0700 (PDT)

Hi Ilpo,

On 10/24/2023 2:26 AM, Ilpo Järvinen wrote:
> show_cache_info() calculates results and provides generic cache
> information. This makes it hard to alter pass/fail conditions.
> 
> Separate the test specific checks into CAT and CMT test files and
> leave only the generic information part into show_cache_info().
> 
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> ---
>  tools/testing/selftests/resctrl/cache.c    | 40 ++++------------------
>  tools/testing/selftests/resctrl/cat_test.c | 30 ++++++++++++++--
>  tools/testing/selftests/resctrl/cmt_test.c | 32 +++++++++++++++--
>  tools/testing/selftests/resctrl/resctrl.h  |  6 ++--
>  4 files changed, 65 insertions(+), 43 deletions(-)
> 
> diff --git a/tools/testing/selftests/resctrl/cache.c b/tools/testing/selftests/resctrl/cache.c
> index 299d9508221f..95489d4b42b7 100644
> --- a/tools/testing/selftests/resctrl/cache.c
> +++ b/tools/testing/selftests/resctrl/cache.c
> @@ -267,43 +267,17 @@ int cat_val(struct resctrl_val_param *param, size_t span)
>  }
>  
>  /*
> - * show_cache_info:	show cache test result information
> - * @sum_llc_val:	sum of LLC cache result data
> + * show_cache_info:	show generic cache test information
>   * @no_of_bits:		number of bits
> - * @cache_span:		cache span in bytes for CMT or in lines for CAT
> - * @max_diff:		max difference
> - * @max_diff_percent:	max difference percentage
> - * @num_of_runs:	number of runs
> - * @platform:		show test information on this platform
> - * @cmt:		CMT test or CAT test
> - *
> - * Return:		0 on success. non-zero on failure.
> + * @avg_llc_val:	avg of LLC cache result data
> + * @cache_span:		cache span
> + * @lines:		cache span in lines or bytes

It may be helpful to directly connect it to the other parameter:
	@cache_span in lines or bytes

>   */
> -int show_cache_info(unsigned long sum_llc_val, int no_of_bits,
> -		    size_t cache_span, unsigned long max_diff,
> -		    unsigned long max_diff_percent, unsigned long num_of_runs,
> -		    bool platform, bool cmt)
> +void show_cache_info(int no_of_bits, unsigned long avg_llc_val,
> +		     size_t cache_span, bool lines)
>  {
> -	unsigned long avg_llc_val = 0;
> -	float diff_percent;
> -	long avg_diff = 0;
> -	int ret;
> -
> -	avg_llc_val = sum_llc_val / num_of_runs;
> -	avg_diff = (long)abs(cache_span - avg_llc_val);
> -	diff_percent = ((float)cache_span - avg_llc_val) / cache_span * 100;
> -
> -	ret = platform && abs((int)diff_percent) > max_diff_percent &&
> -	      (cmt ? (abs(avg_diff) > max_diff) : true);
> -
> -	ksft_print_msg("%s Check cache miss rate within %lu%%\n",
> -		       ret ? "Fail:" : "Pass:", max_diff_percent);
> -
> -	ksft_print_msg("Percent diff=%d\n", abs((int)diff_percent));
>  	ksft_print_msg("Number of bits: %d\n", no_of_bits);
>  	ksft_print_msg("Average LLC val: %lu\n", avg_llc_val);
> -	ksft_print_msg("Cache span (%s): %zu\n", cmt ? "bytes" : "lines",
> +	ksft_print_msg("Cache span (%s): %zu\n", !lines ? "bytes" : "lines",
>  		       cache_span);

I think it would be easier to read as: lines ? "lines" : "bytes"

Reinette