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: <2536d041-c361-5fb7-84d9-3b158e7fc52e@intel.com>
Date:   Fri, 14 Apr 2023 08:24:31 -0700
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Firefox/102.0 Thunderbird/102.9.1
Subject: Re: [PATCH v9 1/6] selftests/resctrl: Fix set up schemata with 100%
 allocation on first run in MBM test
Content-Language: en-US
To:     =?UTF-8?Q?Ilpo_J=c3=a4rvinen?= <ilpo.jarvinen@linux.intel.com>,
        "Shaopeng Tan" <tan.shaopeng@jp.fujitsu.com>
CC:     Fenghua Yu <fenghua.yu@intel.com>, Shuah Khan <shuah@kernel.org>,
        <linux-kernel@vger.kernel.org>, <linux-kselftest@vger.kernel.org>
References: <20230413072259.2089348-1-tan.shaopeng@jp.fujitsu.com>
 <20230413072259.2089348-2-tan.shaopeng@jp.fujitsu.com>
 <bb65cce8-54d7-68c5-ef19-3364ec95392a@linux.intel.com>
From:   Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <bb65cce8-54d7-68c5-ef19-3364ec95392a@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?RTFrVEVMS1RJZ2pTRzVCUGV2TExBZHlJSEZQNXlNVjI1eDJXM1doMDJGSHdz?=
 =?utf-8?B?YzZ4K3Jpa3ZweXFLYWJENkdrU2pRL0IvZ3BjSnBMVmI0Q0N6a1ZzWlhoNDhz?=
 =?utf-8?B?T1dMQzFHUitiZi90L3J6VHByaURVRU5Gc3NkcXExTEt3OGp3Y0ZXMU50S0Nk?=
 =?utf-8?B?dnRsa0JmMjJ3VVFTaG9XVk9wdTNOUTVROXJzemZBMmZXdUhTbzkvLzVVY1N6?=
 =?utf-8?B?SGU2eW1vTlRCQ25WbVB5bkh6VGtWRU4yaWJJMEZjY2hGaWxTT2NGTmgwM2R1?=
 =?utf-8?B?aDE2dzVSNnNWckhTTWlKaDJMNW5pdjYvNnIydWQyTXA5dURQVGFiMkxCQXVl?=
 =?utf-8?B?RGU3RzN4c0ZIT1czaEZJOGZieDBUa1NIYUxKTk5FUm5BSzl3andyYmMwbFNY?=
 =?utf-8?B?cEhQWHRXaURZaFJXSVBpcG5DamphNGdVUGVFWTYwUnEyUnZMZFltRVpvYnFq?=
 =?utf-8?B?YnBXL2RQczVtb1VVNERYTHJEaVFHVi9JT2FjMEdEbGM3VmNNV3hNVzhaTW84?=
 =?utf-8?B?SVZDM3JxOEF6QXpRM2JWZHBNL1dPS0ZUUlE5ZCthdk1Bak41K0dnTjFNRkhh?=
 =?utf-8?B?aTB6UlZUUkNHSWdnVHpmeDQ4UXFDeWRERFZkL0IwelJZVWlVYm9PN0FSb1hH?=
 =?utf-8?B?U0pUUm5PUlNYc0xlNU82MEJJcjR2dTI3MHVPdFNjZjJUK3BMRVAxdE9yQUFt?=
 =?utf-8?B?UFZ4a3F2QWE3cnlLcWxINzVkQnRBRHBMTnN6dzcyMjVLbk9tVFhoOS9rOWJL?=
 =?utf-8?B?WjRWSmlqS2JUTEY1b3NGWDZMK1dkcFRHbDAwb0UzZ2o2czRTcUwzUWNLYWQ3?=
 =?utf-8?B?c2NEQ2lNKzYwQ0w0VHpIN01LcHpBV0V1T01pampKL3R2dEJQUHM1SkEyYUV2?=
 =?utf-8?B?RUJxWGx3ejljV1VpcktzWThIb0NyRFkxcW40VXBaZUZTdmFmT2g0ZS9zQzNu?=
 =?utf-8?B?UHE1TG1kc2NRWHBNNklBeTRuMlR6RVJBWVVVbFhjTWQxaTQwdkViR2dmOWRk?=
 =?utf-8?B?YXdLcldWeVFZWUgrSkwrb0dpUG9BQnlldUYzQzBBeFRsYkFIeE0wdnNySE5Y?=
 =?utf-8?B?MW96bk5DRDFBSzJuN1FHei8vMTN0ekZSUEtzMFJ4SG9lTllpZTFodko1c0Zx?=
 =?utf-8?B?eEF5NGdKUWtCR1RPbzlHbHJJZFJzSnFyMVhKbEZCRmIyajJ0dnZQSXpmajZD?=
 =?utf-8?B?TkRBUTRkR3Rkc2NzVzdXYnZTSCtUM3czbXVaNHNnL3NCVi81c1hqV2ljTXln?=
 =?utf-8?B?K2RlNlc5VGFlSW1ESDdNUzFVWHhBNENnVTNTVkZ2ODB1TXF5NnFOcTA0R3ZO?=
 =?utf-8?B?M2VqdTJ3eGw5dFFTWFFGUzRJT2hiR2d6SkludU5zd1pMT056eUsxbWJidXpC?=
 =?utf-8?B?VEhLTlZLa2M1czRITGZwZHpBMGgrYStLL25teU1zRG0waG45TVpOdjRXNEVP?=
 =?utf-8?B?YzVLUkNGcHpBVThCWUZYYWp2MWFwcWMraS9RNURrdXpLWkZqclFzZEozdStl?=
 =?utf-8?B?c2lzN1dCT0tISjQzNlVDY2p4YXN3T1FVT05GSkRobm03dEJINENMWi96NW1W?=
 =?utf-8?B?dzE3c1RwLzYxTUpaWGowdHRlNXZrT0RSaFk0RTZPLzVYTVI3VSthbkZncXdk?=
 =?utf-8?B?WUVlVHRQSGtTVms5UjRyc1h0a1FxOXFoRk9zVlB1ZXRWMncySHRvb0VucytI?=
 =?utf-8?B?ZzMrSndEc29TTWNFdUxUVGpiaC9YTDlneEppZEp4R0RCeU50T3Q0QXhicU1s?=
 =?utf-8?B?MUVrUnhqNzM1ZGtJa3dhaGE3M3ZzM29tY2xSZ0piSGh0cjlWUGZUU0svTEFn?=
 =?utf-8?B?b2hIQzJsbkFJOWhuTCs0V2RoSXE2bUhRRFoweE1sQ1ExZk5zK1VSQzhzc2Y2?=
 =?utf-8?B?UzNBSjZFWk1tV2xXdVhFQ2RmOTJWSW01NFZITHQzUUlES254T1dYN2dOMWgv?=
 =?utf-8?B?c3pqTTZOY25TZHVLdGxtWTRyTkh6bG0yT0NhQVpWTWxxS2hrRitFblAyWUdR?=
 =?utf-8?B?SWhPL2ZIUk85ZjFwZG5pUHBDRVRRYmVreWhBOGE0VVYzb25PNHc5RFcrTVhZ?=
 =?utf-8?B?eUJmNnQ5c2RQdVRYa0cyZm1FdFlPbmQ0WU1WaWpVL1BJNW5pd1J2SEVGcmh5?=
 =?utf-8?B?dFQ4djg0aktBeUlUd0hja0tZMDZKTGZzaXNJS0lBblhyY01ZRVU0T2ZmVFJy?=
 =?utf-8?B?R1E9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 4731273c-2bfb-4fe8-39b3-08db3cfc59eb
X-MS-Exchange-CrossTenant-AuthSource: CY4PR11MB1862.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 14 Apr 2023 15:24:34.4512
 (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: Pn+MhwLhMsWtpYdWj+aVqAN4EcnOZLtjt2kE3i7YcPUh6p2F6GqMEmL4knpU9Wd2K6wlYLWkhPNwy8pE4OaPzH/ITGVzHA9clfR86A0SVyk=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: IA1PR11MB7812
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-4.4 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,
        SPF_HELO_NONE,SPF_NONE,T_SCC_BODY_TEXT_LINE 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


On 4/14/2023 5:31 AM, Ilpo Järvinen wrote:
> On Thu, 13 Apr 2023, Shaopeng Tan wrote:
> 
>> There is a comment "Set up shemata with 100% allocation on the first run"
> 
> s/shemata/schemata/

Having the typo may be ok since this is a verbatim (typo and all) copy of
an existing comment.

>> 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%. Even if write_schemata() is called in MBM
>> test, since it is not supported for MBM test it does not set the schemata.
>> This is currently fine because resctrl_val_parm->mum_resctrlfs is always 1
>> and umount/mount will be run in each test to set the schemata to 100%.
>>
>> To support the usage when MBM test does not unmount/remount resctrl
>> filesystem before the test starts, fix to call write_schemata() and
>> set schemata properly when the function is called for the first time.
>>
>> Also, remove static local variable 'num_of_runs' because this is not
>> needed as there is resctrl_val_param->num_of_runs which should be used
>> instead like in cat_setup().
>>
>> Reviewed-by: Reinette Chatre <reinette.chatre@intel.com>
>> Signed-off-by: Shaopeng Tan <tan.shaopeng@jp.fujitsu.com>
>> ---
>>  tools/testing/selftests/resctrl/mbm_test.c  | 13 +++++++------
>>  tools/testing/selftests/resctrl/resctrlfs.c |  4 +++-
>>  2 files changed, 10 insertions(+), 7 deletions(-)
>>
>> diff --git a/tools/testing/selftests/resctrl/mbm_test.c b/tools/testing/selftests/resctrl/mbm_test.c
>> index c9dfa54af42f..9b591c35310f 100644
>> --- a/tools/testing/selftests/resctrl/mbm_test.c
>> +++ b/tools/testing/selftests/resctrl/mbm_test.c
>> @@ -89,23 +89,24 @@ 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 END_OF_TESTS;
>> -
>>  	va_start(param, num);
>>  	p = va_arg(param, struct resctrl_val_param *);
>>  	va_end(param);
>>  
>> +	/* Run NUM_OF_RUNS times */
>> +	if (p->num_of_runs >= NUM_OF_RUNS)
>> +		return -1;
> 
> This is not rebased correctly, negative return values mean now errors.
> To complete tests "normally", return END_OF_TESTS. Please see commit 
> fa10366cc6f4 ("selftests/resctrl: Allow ->setup() to return errors") for 
> future information.
> 
> With the forementioned problems fixed:
> 
> Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> 

Thanks for catching this. Unfortunately this was just merged so a separate
fix will be needed.

Reinette