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: <d28c2b1c-a974-1622-e817-942ff28ade24@intel.com>
Date:   Thu, 29 Sep 2022 08:27:38 -0700
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Firefox/102.0 Thunderbird/102.3.0
Subject: Re: [PATCH 1/5] selftests/resctrl: Clear unused initalization code in
 MBM tests
Content-Language: en-US
To:     "tan.shaopeng@fujitsu.com" <tan.shaopeng@fujitsu.com>,
        Fenghua Yu <fenghua.yu@intel.com>,
        Shuah Khan <shuah@kernel.org>
CC:     "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "linux-kselftest@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>
 <24cbb6e4-d853-6c7e-cffc-daa7412d1eab@intel.com>
 <TYAPR01MB63300E751096E8F613CC7F398B559@TYAPR01MB6330.jpnprd01.prod.outlook.com>
 <580b7e0f-1303-bb27-0f9c-f05f793a30d3@intel.com>
 <TYAPR01MB63302FA7919E8C95B9C56FC08B579@TYAPR01MB6330.jpnprd01.prod.outlook.com>
From:   Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <TYAPR01MB63302FA7919E8C95B9C56FC08B579@TYAPR01MB6330.jpnprd01.prod.outlook.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?NUdCTitPUExyU25BdDFSdGhuQVVYTCtWMTE0R1Y1L1AxSUMraE5TYkV1dVFU?=
 =?utf-8?B?N0h1VXhZNmV0TUl6dVZIeUpjWjMyUmdGUTJUamJydjlSTm1EdUsvaUpqQVpO?=
 =?utf-8?B?MGk4Qm1OL01OZzFFUUpDeHJvYjllbWU5RFdJcTVPVUIzT0dVYzJwb0pad0dG?=
 =?utf-8?B?ZmttUm80Nk1WcWlBVzRLVy83NjZyT0pHLzdyN1BVd2Z5ekdHbUJYV1JndVV6?=
 =?utf-8?B?VEZ1Nk5LYU9OMWd6VlI3OENjZWd5NzBqbTdOT2hldVFvSTNmYXA0RmxVSXVq?=
 =?utf-8?B?SWZpcTBlMGozQnM2TjJxK3huUGFzeVpNdVdJNUpSOTZyMmpOMmUzdEhOZWdY?=
 =?utf-8?B?M25pZkxFZXRmWHFsMFhNL0ZkM2pVNDZ3MjdxSEEzVGtoeFN5cXlZMitxVTF3?=
 =?utf-8?B?REdkYjJyWm16RzVFcUJGMC9aNUFlaytDaU9DcVpJV1g4L0pVVTcyanlTT1Qv?=
 =?utf-8?B?SmVzeVhiaU1kMExJd1JFUG9VWm1HZWZvU1EvdkpvdTBHUTM4cjB5dnFtY1ZZ?=
 =?utf-8?B?dWNHalFLL1lDVGRsRnpDd0g4b21QekJUTWZnV1hEVEJIN2ZISEhUZzlBRE5s?=
 =?utf-8?B?U2ZWaUxaZTRzUm5BS01FS1R1bDdZMi9FZU0rUEZnd3pTcGdZaktYN0xxVk1o?=
 =?utf-8?B?KzZZaC95L3VDVHJPZTNuSzZYb01EVWFFQjcvTmRNalBLQWgrbmdnQ3JKZjBo?=
 =?utf-8?B?TjNFZTN1UWN3SW43SkdITWhWOGRjZFNtNVlFTjdOdFRMaHE1Uyt3RVg5SFFr?=
 =?utf-8?B?Mkx5dHc3elpBazJudUtIM3JxSkxySHBrSmJFRkNhM0Y1cEU4NXBlaFlTWEdn?=
 =?utf-8?B?bTA2TDZGSTV4eG9ZaWtFNGREVm1GT2ltYkdFS2pZV0l5SVhNbC9uTGVCQXd3?=
 =?utf-8?B?YmpJQ252VXA5SUNtMVhwVjhrYnlLYlVwcEQyclNTN3JBdGNkL1ptV2F0YUVU?=
 =?utf-8?B?c0VnYlJObmVuQTNWSnRWZllQNlVSdHpNWFhrTVRPMlFITGxRZHJVcWpYK29l?=
 =?utf-8?B?aWxRbElFZnQ4WUZYYStJWnIyaFpRWUxqYVNDNGdYaWtscDZaZmdtRHUzdW9M?=
 =?utf-8?B?SUJpTGZEQnZuMnowY3Nob3hGTXV6SG51RzlldHFKZ0paOHkweEUxZGt6dXNq?=
 =?utf-8?B?ZXZWbndRZnJXbFVrY0dqWVBtMmRWYk9YN1BFSG1HSG1JYUJrMzVJRDRpWDFM?=
 =?utf-8?B?TU1pMDhHeW95M0FhbVQzR0R4YnQ2a3A3ZkRPalB4UHdNWm96SGI4YSthM2dI?=
 =?utf-8?B?M09YNnBBSmhtb0duSGFYOWxKaHJidTFHLy9vcHZqNXRBVWx6eWxSbWl0WW5k?=
 =?utf-8?B?V2YxK0RIUnRIUHZTbWh0REcvUXdlWmxVK0ZFakhnQnJySzBLZW4xd2xxT2py?=
 =?utf-8?B?UTBiZVVlSllyWkNBMU1waENZZTdwaDJLQTRZZ2tnNi9tN0xMTTBPZFd1UVAx?=
 =?utf-8?B?TndaSDhEYTlya0R4Y2JTelVBUDRwdXBPS3V4VEI0ZDdpMzJMK012WDVycVIw?=
 =?utf-8?B?ZWxaeTZnT05CMnJwRUxGNk1meEI2Nnp4dmhObmRmV0JwQVp0MkhhazdtWUJR?=
 =?utf-8?B?Z0JtS2VKMjRxaWFyd0VhS3Y1UDdIQ0hDWWQvKzZaM3hVcGhOc2FnUkhlMjJW?=
 =?utf-8?B?cFJnZHZPOXVyWHVEQjBOK3BSOUdzWExSVm9KemorZEZlZkxBWDI5K2g1aklO?=
 =?utf-8?B?VE85RFhydkVYVHEyQjlaVEgwZkZ1VmFLbk83VUNIckFsTnZlZkZrazJTYlVz?=
 =?utf-8?B?NTcxMCtIeXo0ZzlxejVYaEhLUkM1K3BCUjA0V2JOQnd6ODlOLzVMR1B4QktZ?=
 =?utf-8?B?SkdmNllJaVR0WHlPQzdGR2Y0U0hvK29XZlRYZ2laMU1ITUgra1pzUXk5bXFj?=
 =?utf-8?B?SmlNdW9pMTNtaGVmN2JKMk5aNkVMSG1LdEVFVEt5U3RxQjFXVUw4bmVUN05r?=
 =?utf-8?B?bENRMmpwcllpU0tIN0FTVDFvVGZ2MTh3R0swQWdJK09HUS9JQ0k5QXNRVk8v?=
 =?utf-8?B?bWVlby9LWmJuVTFxSDRaK3c5SThiZFJxa1FWbW56MEsrTWlMdEYwaG1pZFJF?=
 =?utf-8?B?NGVZQitPbkNPaG81MGNsc2N6a2ZpM0xxbWUraEN1ek8rREdMTGNIWWgxM0t0?=
 =?utf-8?B?MndISXRWQlJQak1rTzYweTRFVkh4bzFqYzBBNFZ6ZFIrSlNEWWVjRkVFQWYy?=
 =?utf-8?B?YkE9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 5c0d52cd-5749-4ad9-0594-08daa22f25a5
X-MS-Exchange-CrossTenant-AuthSource: CY4PR11MB1862.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 29 Sep 2022 15:27:40.6955
 (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: iODnX0pey6lk95Wjk+5Xvgak6ojcj7bf2x5Q+1tkqIbBD1qB9ReMc1oryuvXhQhYwxbeoOjgPgpFlCvMK9jd4ZQJ/21emX0RkNYDLmDKhUQ=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR11MB4877
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-8.6 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,

On 9/28/2022 10:28 PM, tan.shaopeng@fujitsu.com wrote:
>> On 9/27/2022 2:01 AM, tan.shaopeng@fujitsu.com wrote:
>>>> 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.
>>>
>>> In mbm test(mbm_test.c), resctrl_val_param.mum_resctrlfs is set to 1
>>> and never be changed, and umount/mount resctrl file system is always
>> executed.
>>> So it is not necessary to run "if (num_of_runs == 0)".
>>
>> This is true for the current usage. You could also add a warning here ("running
>> test with stale config") if a future test sets mum_resctrlfs - but with all the
>> current output of the selftests a warning may be lost in the noise.
>>
>> I think it would just be simpler to support both cases. Having the tests be more
>> robust is good.
> 
> I understand that mum_resctrlfs should support both cases(0&1).
> 
> However, "num_of_runs++" is executed before "if (num_of_runs == 0)",
> So write_schemata() is never executed regardless of mum_rectrlfs is 0 or 1.
> 
> 97         if (num_of_runs++ >= NUM_OF_RUNS)
> 105         if (num_of_runs == 0)
> 106                 ret = write_schemata(p->ctrlgrp, "100", p->cpu_no,
> 
> I will fix this in the next version
> 

Thank you very much.

Reinette