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: <24cbb6e4-d853-6c7e-cffc-daa7412d1eab@intel.com>
Date:   Thu, 22 Sep 2022 10:44:05 -0700
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Firefox/102.0 Thunderbird/102.2.2
Subject: Re: [PATCH 1/5] selftests/resctrl: Clear unused initalization code in
 MBM tests
Content-Language: en-US
To:     Shaopeng Tan <tan.shaopeng@jp.fujitsu.com>,
        Fenghua Yu <fenghua.yu@intel.com>,
        Shuah Khan <shuah@kernel.org>
CC:     <linux-kernel@vger.kernel.org>, <linux-kselftest@vger.kernel.org>
References: <20220914015147.3071025-1-tan.shaopeng@jp.fujitsu.com>
 <20220914015147.3071025-2-tan.shaopeng@jp.fujitsu.com>
From:   Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <20220914015147.3071025-2-tan.shaopeng@jp.fujitsu.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?T1RQdWNTcDVXUGNjQUtGUTVHVFFJSEphRjE0d0tYUDhncVNsWmF6T0N5cUtI?=
 =?utf-8?B?d3RyTUFRbGJPU1RkUjd1MUplYXovLzNwQkkyY1BrelNvWDJmYUx1dVRIaVNT?=
 =?utf-8?B?SEJ3Rk5HNW9kN3Q4VDdZZ3Q1Zy83YmJzQ0ZKd0svK3ZNOEhmNFlGak1OVExn?=
 =?utf-8?B?aEo5M3hZR3ZxN0h3d1hqVDdNeFgzdDdvdXFLcHBuSi9LV20xREVJNE1yZ1JS?=
 =?utf-8?B?MFFLRk8weVZRZHNBMzljWkJBNlFKTHVTOGE5dTc1djhOUzZMRjVnd0hWL3g2?=
 =?utf-8?B?RlFHemE4SlgrZVpydGtITXZRVVpFUVN1OVN6TlNKdEFSc3lid2Nic0hDVk5y?=
 =?utf-8?B?NEZpK2U0L25QRnJQTWZVOHVwcDhCdmJzc2g2eWNDMzQ2WFBzTUdVYUNWTEUx?=
 =?utf-8?B?T3hMa09aZ0VZdElIaDMvVEVvd05RNEpEdXFEQno5bS84OEJFQjRtMlVRM1BS?=
 =?utf-8?B?QjVjUS8yY3dPSDNsRGp5dHhqU1Jyd1ZvZGVEa05oYkpNbDh6TTROaFFUakVB?=
 =?utf-8?B?RTFkamlXNEZsVkNyWHZicFVsK3JtcjZ2cWYwK3Z3VWxQS0FOMW5oY09PeGd1?=
 =?utf-8?B?dEZDS2Y5NEdXMFVEbTdlQW5vQzRjdlBKOWZWVXNqYWxtWGE0QTNhNXFWSlhY?=
 =?utf-8?B?NHhJc2JKQm1UWGVlVCtnWmZlK2duOEFQRVM2RVYvMHRYK0w5M3IvTjgrRndD?=
 =?utf-8?B?S2ZzMEliOFhtdkdFbStTdzlSTXJjZUVLUlhGV3dMRWtiUnNXN0RGMWlWZHBz?=
 =?utf-8?B?N2pOaEFFek51SSsvcnJvR085aUxrek1Uei9HcHRNUjQ5MUhNS2tCOGZnc3dl?=
 =?utf-8?B?Z3I1MStxOTdqOEU0K28zVmJCc0E4WDBKSGU1NU1MZUtCMEcwd296amFFMkNG?=
 =?utf-8?B?S0lrVWFrRnp0MlJJVy9xS1J5ak9EMTdGZGZpQVl5M0R0bTJ6Q2kvRmVGazZn?=
 =?utf-8?B?L1RSREFkOGVYQzlUTlRQazE1czIwNUxnOS91OHBVSFh1RGVWQXFnM0EyanQx?=
 =?utf-8?B?bEFSK0UwNU9vZndqTG55UW5oYlZIam8yZEFsTDJVWGFFdjc0LzlHY29HMVJV?=
 =?utf-8?B?SmtiTTRYZ3FwR0YvV2Vjbk9JZjhDYms0ejh2ZmNvRzBLa2NHMWlhbEovY0xu?=
 =?utf-8?B?ckRuVlRNWkJCcFVHeHVRYnZkOGRWdk0vR3JkUUlwZ2JBdEozSUtSRVFvZVBL?=
 =?utf-8?B?WlFxdm5raXU0b2pXNnFwbElrUUhxaHVZSFVEMkxUTjZMUEEzeFpINk1kTVM2?=
 =?utf-8?B?Rm1Pa1hIZlVETGQ2ZENqT0VhdDhFR3pPWlZNNFBGQlB2eG13SUlGNjFjMytX?=
 =?utf-8?B?NmJKS2VxeEgvYlN2U0k1ZlZRWGhpMWczY3ZUNVg5UU4wUEhCSkhzMzVwdEFR?=
 =?utf-8?B?TG9NRng5QmxKRCswc2JMNVp2UW1oc0NWYVZ2UDArUS9DcTJPL05MQUdTMFVq?=
 =?utf-8?B?K1lGTExFOWIvakNhS05NVVl4YXpjdkZ6cE5mOG1tdEVEUjZaZlYzRkwzSHJP?=
 =?utf-8?B?WUx3UjdlT1BrUEFLTjlvZndtOG5yWWJTZ05iTm51bFlCYVJBMnVyMTlYZFVy?=
 =?utf-8?B?ZDlJRFFXRVdkR2ZXRVcvcUFVcTI4Tkw5b3dBMDhBT1lOOXVZY3FYdjlYejBx?=
 =?utf-8?B?d21JTFVVVWdnRVIxWlR6STNaVHFtMTc2NnpZRkdvalpLOGlaTExKZHZDVExr?=
 =?utf-8?B?TUtMNUJNZFVKaHFaSG56djM2azZOZDdndmdMOXY2N2k2dklwUk9kTUhMK0NK?=
 =?utf-8?B?VGVGdjhEY3lsMDNMTkFab2lKdHdqV3J6ZjFrUEc4dWhVM0J1SGdnQkJ1bmlV?=
 =?utf-8?B?OEovZDI1aVBjTUhXeDVOeUJ2WWdCd01Pd1pJK0VNcmN4c0FLS3MrcklzWUFZ?=
 =?utf-8?B?NVQrY1M3aFNIU1BEbTdmVmV1L29uREI0UlhPM0FoSW1reGtHVldPWTBLM0o1?=
 =?utf-8?B?eTVIZndGNjVnRDRkT3JWYTJkeFYvSi84cHRSRU94MlpIam93U2IwcW9PUXAv?=
 =?utf-8?B?NXJjK1FtREVkUmFkNklWMDlNVVNHMERCSmxydkxqT1dyOWlxNlFYdGtCTnND?=
 =?utf-8?B?M29MY0IzKyttOFJZQWxGMFRqUXY0bmN0bEM2S2NiU2habmNDb2czUU1QcmRO?=
 =?utf-8?B?ZnNkVmd0NTVvMURMdVdTVmlNVXNSYUNQWmtvWEJHN2FFTFRoclJWZmNTc3Vx?=
 =?utf-8?B?aEE9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: f7a5fff0-d7d9-44d4-7530-08da9cc20cb9
X-MS-Exchange-CrossTenant-AuthSource: CY4PR11MB1862.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 22 Sep 2022 17:44:07.9134
 (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: ZVULAEWgaTtp+uGsUYOiFRnzNtU412LDQ3OO0b9RTvYGs7gL5m1IwEAYPtk9XYw6ZkASsh7WSF92B9M1fpfBKdtloIlq6ms9mSckO6aRbAw=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS7PR11MB6064
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-6.3 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 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 Shaopeng,

(typo in Subject: initalization -> initialization)

On 9/13/2022 6:51 PM, Shaopeng Tan wrote:
> There is a comment "Set up shemata with 100% allocation on the first run"
> in function mbm_setup(), but the condition "num_of_runs == 0" will
> never be met and write_schemata() will never be called to set schemata
> to 100%.

Thanks for catching this.

> 
> Since umount/mount resctrl file system is run on each resctrl test,
> at the same time the default schemata will also be set to 100%.

This is the case when a test is run with struct
resctrl_val_param->mum_resctrlfs == 1, but if the test is run with
struct
resctrl_val_param->mum_resctrlfs == 0 then resctrl filesystem will
not be remounted.

I do think that this setup function should support both cases.

> 
> Clear unused initialization code in MBM test, such as CMT test.

Could the initialization code be fixed instead to increment
the number of runs later, similar to cat_setup()?

> 
> Signed-off-by: Shaopeng Tan <tan.shaopeng@jp.fujitsu.com>
> ---
>  tools/testing/selftests/resctrl/mbm_test.c | 17 ++++++-----------
>  1 file changed, 6 insertions(+), 11 deletions(-)
> 
> diff --git a/tools/testing/selftests/resctrl/mbm_test.c b/tools/testing/selftests/resctrl/mbm_test.c
> index 8392e5c55ed0..38a3b3ad1c76 100644
> --- a/tools/testing/selftests/resctrl/mbm_test.c
> +++ b/tools/testing/selftests/resctrl/mbm_test.c
> @@ -89,24 +89,19 @@ static int check_results(int span)
>  static int mbm_setup(int num, ...)
>  {
>  	struct resctrl_val_param *p;
> -	static int num_of_runs;
>  	va_list param;
> -	int ret = 0;
> -
> -	/* Run NUM_OF_RUNS times */
> -	if (num_of_runs++ >= NUM_OF_RUNS)
> -		return -1;
>  
>  	va_start(param, num);
>  	p = va_arg(param, struct resctrl_val_param *);
>  	va_end(param);
>  
> -	/* Set up shemata with 100% allocation on the first run. */
> -	if (num_of_runs == 0)
> -		ret = write_schemata(p->ctrlgrp, "100", p->cpu_no,
> -				     p->resctrl_val);
> +	/* Run NUM_OF_RUNS times */
> +	if (p->num_of_runs >= NUM_OF_RUNS)
> +		return -1;

You seem to be fixing two bugs in this patch, the first is described in the
commit message and the second is to use p->num_of_runs instead of the
local num_of_runs. Although, after a quick look I cannot see if 
struct resctrl_val_param->num_of_runs is used anywhere. Could you
please add description of these changes to the changelog?

> +
> +	p->num_of_runs++;
>  
> -	return ret;
> +	return 0;
>  }
>  
>  void mbm_test_cleanup(void)

Thank you

Reinette