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: <10ca9b73-b3e1-4139-6cbf-c362467c7943@intel.com>
Date:   Wed, 5 Oct 2022 13:56:21 -0700
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Firefox/102.0 Thunderbird/102.3.1
Subject: Re: [PATCH v2 1/4] selftests/resctrl: Fix set up shemata with 100%
 allocation on first run in MBM test.
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: <20221005013933.1486054-1-tan.shaopeng@jp.fujitsu.com>
 <20221005013933.1486054-2-tan.shaopeng@jp.fujitsu.com>
From:   Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <20221005013933.1486054-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?NWlVZ1dpYlBuSlVmQy95U1RqQzFSbU1iVVdYekxHeFRtRXRUZXR0S3FKWGdZ?=
 =?utf-8?B?U1dtWmo0TFNJcDExNFQ2Z2pLZk5qczJ4bjBpQ0F6VktvNGdmSVZKYmxuc1lH?=
 =?utf-8?B?Sk5zdGhIZC81dElSVnJRUFE4KzNudEx5bnpGemgyaTFmamRKMU1VN2NMbnpE?=
 =?utf-8?B?UkVoYnZkOThPZDZsM0UrTy8rQjVxWXduOFc3a0k4ZlIzOVdoVGRTTEFkVDVl?=
 =?utf-8?B?OFlOazNyL2paTGJGNEZubG5HLy9aUy8zd1lJa1BXN2xwMllKUnVCZzMyNzJJ?=
 =?utf-8?B?SFN5dzcyT2h2Y09lNjhsZDhITEMxdXVzNkpOZHVJR1l0dk9EUVo2dmNaMWlM?=
 =?utf-8?B?bGVDVXZtSHJmODRPYUFRZEdXMHVSaVFCeldyWEpnbmkzYTlBK2IyOVJlZjJk?=
 =?utf-8?B?VUNRcml2SE0yVGdjR1RPVmI5Zk8vUGhYZm12cnh4TGxGMG0yajhxNkZVY2JQ?=
 =?utf-8?B?eGo1aHQvSmFIYjdPWFJjcm1kZDNjbkpKdkwxczFlaStTdGRhUEMrM1NjY0dy?=
 =?utf-8?B?TEZoVWZrb0Vrd0NYZUpqZXBtclllSEkrMjhQdzhQTnE5RWhwaXF6VlVjNExY?=
 =?utf-8?B?ZE5BY3VpT05LYnBJV3RmNUVYaGNyVGdDbmZSTEs4NHRjenhMUGhzS3JyYjdV?=
 =?utf-8?B?N3dHRTdNdFJkdysxL0RaMnQrS2FBa3lFWmhXRlh0YjhPWExKdmJvN0NBQVh5?=
 =?utf-8?B?WHlWeTZnMjk4RG02MnE1dVNKQlo3b3VOaUdIdStnWWw1NlhYVkVDM2s4ZVpL?=
 =?utf-8?B?L2l4cHI0UTBOQ21QdXF6bzg2L09aaXQ0cEh5N0pPMVM4RzFId3JXQVI4Wkh1?=
 =?utf-8?B?V3Nzbzl5eUhsbWdOOFNVYmVERFlqbTIvbTdOUy80U0ZlbjJTZzU0amtvcEtt?=
 =?utf-8?B?b25WZ3dPd3FuS0MvKzVGV0hOOXRYUXJNc1FSUld4TGdFTG9uN2RvVEJqZU9C?=
 =?utf-8?B?WUd5QVhQVEplUFNLb1EyMUI2RU1QNGorWUQ0YWkzdXRjVmpuUzBSVW9kMFVP?=
 =?utf-8?B?bXB3S0lhclhKZk1kenlvSGdiY0RSR2VIUnhTdG9oMnduVkZ1L2ZCdzJVdm9u?=
 =?utf-8?B?cllJWmZEWE9vcHA2Nm50alhTOWd6R0FHWHl6TXNwVS9DU2JnQThTTTc5ZlJS?=
 =?utf-8?B?UE1BbjhuU0F5LzVtczhVVWNCais2T3RxUVBhb1M5U3hXMEcxaWlITVRtSEFa?=
 =?utf-8?B?TkwvazVrNjFFbWlyRTZmWWY2Nm5sNnRMeFFTRmtIYjlkdXJia2s2T0NoSE5T?=
 =?utf-8?B?cWg4WjlpNUpoSDBrUlMvdXNUOGxFQUEyUDlWMGxnanltOWY1ZE9HdXFUUSt0?=
 =?utf-8?B?clJLOHlqTUxCVFRmOUhzdkJNUGowZjdNVWJ3bktOc09iSE0xNTNSQXdZQlA5?=
 =?utf-8?B?S2hMTzh5OXErUy92MCswTmNwQnZqS096bUdxNkx6Tk1UcWJGNGpERkhpVlRP?=
 =?utf-8?B?U0ZtQjMyUjJNNmRyMnQ2VUhmRlYyVGYvL1ZMRGhTdlN4Mi8yejdnRDlueGlP?=
 =?utf-8?B?UGdNM0Q5Ry9Oa2o0TStGMDNKYjFqQW5iUTNBWm5kckJHLzE5c21ZMmF6SVBW?=
 =?utf-8?B?eGpQZVBrVkhSU3Q0THI5NS9SZFZUd1V4anpyQk9CeERySlptblhQaWovUGhh?=
 =?utf-8?B?REJUdU5QZFllOS90UFdGU0xmTGY1RE4zZUs2RW9LdzFrbmlMWGRIb0R4KzFQ?=
 =?utf-8?B?OURTZEF3ZEUzSzJFNTNPUGlscHYyNGZqak5ON2NRL1dDSXc0Umlzc1RCUmtS?=
 =?utf-8?B?WFNJT0pEcTh4L3dhQ0g0VUNnYVpVM09FenlRampBcnlTbGJ6MGdzc2drUkZ3?=
 =?utf-8?B?d1U3YnBWekMxVmhpeGxrdVFMbGhOeUxwR1FMbFZEZWRibnUxVFRkQkE5N0J4?=
 =?utf-8?B?Z3l5RGRwYThOdVF5NDFOWkFkV2pFb1VBRVA1OTRPMmJnVlhYRnphMlh2QWhh?=
 =?utf-8?B?SytIdC9YQXJTaW1ZTnVVTEtmSDZnQjBuc29lZklpVTBNc0p4K2h2YWlIL2dU?=
 =?utf-8?B?MXRwZVJFOStKU1VjaDI3L2RpTE9jY2Q3K0lkWURMMXlSMnMxTUtxVjVIL3JL?=
 =?utf-8?B?dW9uL0RJcUI3eTV3U0hqQUNidFVLZ0U4YzFZYno1bmxwRmovSE40bmhmVU5L?=
 =?utf-8?B?cmxhaGQzSmhEVjlpM3FiZjNZcUhxTGFSVjA5eHdueFF3bGpLaE9QL3NOeC9w?=
 =?utf-8?B?K0E9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 9cdf47eb-2461-4233-b066-08daa7140fba
X-MS-Exchange-CrossTenant-AuthSource: CY4PR11MB1862.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 05 Oct 2022 20:56:23.3298
 (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: Uye5YFayBJE/xvQgjemD9ovBXtX0Hqxp4kr24J/w5YlOPUt4kc8PCXL22TUshzcwDdD57mhzpb/xBng4QIHPwjjST8gFDjf4aZULUFJOSC0=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MN0PR11MB5988
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-5.8 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_PASS,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,

I understand there is a typo in the code you are modifying but I do not think
that justifies the same typo in the subject: shemata -> schemata

On 10/4/2022 6:39 PM, Shaopeng Tan wrote:
> There is a comment "Set up shemata with 100% allocation on the first run"
> in function mbm_setup(), but there is an increment bug and the condition
> "num_of_runs == 0" will never be met and write_schemata() will never be
> called to set schemata to 100%. This is currently fine because
> resctl_val_parm->num_resctrlfs is always 1 and umount/mount will be run

resctl_val_parm -> resctrl_val_param
num_resctrlfs -> mum_resctrlfs

> in each test to set the schemata to 100%.
> 
> To make mbm_setup() future code-change proof, fix to call

You could be more specific by indicating that this change will 
support the usage when the test does not unmount/remount resctrl before
the test.

> write-schemata() properly when the function is called for the first time.

write-schemata() -> write_schemata()

> 
> Also, remove static local variable 'num_of_runs' because this is not
> needed as there is resctl_val_param->num_of_runs which should be used

resctl_val_param -> resctrl_val_param

> instead like in cat_setup().

With the move to using a member from the struct I think it would make the
code easier to understand if num_of_runs is explicitly initialized. That is
indeed the norm when looking at the other call sites.

> 
> Signed-off-by: Shaopeng Tan <tan.shaopeng@jp.fujitsu.com>
> ---
>  tools/testing/selftests/resctrl/mbm_test.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/testing/selftests/resctrl/mbm_test.c b/tools/testing/selftests/resctrl/mbm_test.c
> index 8392e5c55ed0..4a54be314402 100644
> --- a/tools/testing/selftests/resctrl/mbm_test.c
> +++ b/tools/testing/selftests/resctrl/mbm_test.c
> @@ -89,12 +89,11 @@ static int check_results(int span)
>  static int mbm_setup(int num, ...)
>  {
>  	struct resctrl_val_param *p;

p is defined here ...

> -	static int num_of_runs;
>  	va_list param;
>  	int ret = 0;
>  
>  	/* Run NUM_OF_RUNS times */
> -	if (num_of_runs++ >= NUM_OF_RUNS)
> +	if (p->num_of_runs >= NUM_OF_RUNS)

p is used here _before_ it is initialized in the code that follows. 

>  		return -1;
>  
>  	va_start(param, num);
> @@ -102,9 +101,10 @@ static int mbm_setup(int num, ...)
>  	va_end(param);
>  
>  	/* Set up shemata with 100% allocation on the first run. */
> -	if (num_of_runs == 0)
> +	if (p->num_of_runs == 0)
>  		ret = write_schemata(p->ctrlgrp, "100", p->cpu_no,
>  				     p->resctrl_val);
> +	p->num_of_runs++;
>  
>  	return ret;
>  }

Reinette