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: <bbc21b48-58b5-6356-0248-656e22d95281@intel.com>
Date:   Mon, 24 Oct 2022 09:45:14 -0700
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Firefox/102.0 Thunderbird/102.4.0
Subject: Re: [PATCH v2] x86/resctrl: Clear the stale staged config after the
 configuration is completed
To:     Shawn Wang <shawnwang@linux.alibaba.com>, <fenghua.yu@intel.com>
CC:     <tglx@linutronix.de>, <mingo@redhat.com>, <bp@alien8.de>,
        <dave.hansen@linux.intel.com>, <x86@kernel.org>, <hpa@zytor.com>,
        James Morse <james.morse@arm.com>, <jamie@nuviainc.com>,
        <linux-kernel@vger.kernel.org>
References: <1665304608-120466-1-git-send-email-shawnwang@linux.alibaba.com>
 <7fa6ed4e-abae-85fb-4e95-8c73755a4263@intel.com>
 <ad08eee6-cfea-3858-0def-e2e3fef315fb@linux.alibaba.com>
 <ff44b0ff-6adb-3bae-d17e-4c341c09df5d@intel.com>
 <86fc22a2-e779-b7ab-67d6-a3aff975ae56@linux.alibaba.com>
 <30637459-7419-6497-6230-b13c73a947de@intel.com>
 <2cdfbe28-01cc-926d-2f6d-2a974a4c5a74@linux.alibaba.com>
Content-Language: en-US
From:   Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <2cdfbe28-01cc-926d-2f6d-2a974a4c5a74@linux.alibaba.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?Uyt6d0oyVnQzMm9pTit5TkdSZDdKTzVSWnE1ZU9jNDREZWtGRVk4RmJWeVVX?=
 =?utf-8?B?TzZ3NTFJSjRqcmQ1UjNxTlVLL0FYcFFvSE5teThoaDUreW5rYVNuak9ZK2tO?=
 =?utf-8?B?cjZqK0ZneXdlREdvTHpXVnhOZ1Faa2RWZmVzN3RSZVpFZ09jeEhXZHZpU0Vh?=
 =?utf-8?B?Y0poVk1IdEE4TXhCcVVoMlN1a2k1TXpPYmk4aG15T2Y3TTBjNHdHWVB0cDQ4?=
 =?utf-8?B?eHdZME93NEUxNU9aNi9EQWZRdllMUW5ReG14bEdKZ2F5QXU2MmJGcjRyaXhx?=
 =?utf-8?B?TjRRWit5TklUMUcvczZyUmgzaGw3R2Q4TVVRYVg1Z2Q1Z09kbHI4RjYrcFdU?=
 =?utf-8?B?MWpmUmJEbTh6dVNqK3NURFBnUEtEaFM4UjNHK2RjTHNEVlQ5QjFPdDByQnB1?=
 =?utf-8?B?MlpFYlZ1ZWZISXgyd1pCVExWMzFZTUpodFRRMEhTaEN2a2dQR05vdU9xVDNB?=
 =?utf-8?B?U1FMWHVwNHFGY3h0WXZLejJoN3BNM0VZdFZyZ0xhNU5mUW8xVTNFYkN1OFBJ?=
 =?utf-8?B?RTd5eGREa0J0bHd6Z3l3VG9TZVhlcFpDSmI3ZXBlNkRsanE3REZlYU5ZaW1h?=
 =?utf-8?B?cENCdHROL1FoSVV0MExweWlvSi8wTWVEQXBQaFhpVUp4K1l4QVZTN0hIenky?=
 =?utf-8?B?Q0ZmeEUwZTJwRWJEcDFRM3Z2b2R0VVd5MTBFWmN2WkxuTC82NGIyVDBkaE9j?=
 =?utf-8?B?b0hvRHFDeDdXdDczTWxvZWp5c2QwODZjTWhmK1F5eHZtSXBmYmhVUXlQcmR1?=
 =?utf-8?B?clU4QmJWZ2h6VmkzeFN2YzRMS1lJYit0bjU0eG9PUkcya1pHRjhqSk8zUnhj?=
 =?utf-8?B?NENYYytsNlcwNDJZNXV3ZTVXaDFubmZ4a29xVUZJYlZycWJ6cCtMNmRKS3dG?=
 =?utf-8?B?RTcxQ3Z6TThVQ29kQ0lZN3R1OUtaQi9xUmRjNGwyajg1L2F3YlgyUGNaUGpH?=
 =?utf-8?B?dS9FK2dpUC9nRjQ0MERDajdjNWl3K0JBYnhUTStCSTZFV05pUkRVSVdhNFMw?=
 =?utf-8?B?Vi9jUnVxcG1XOUhTN2NZOFd2bWl0ZkN5V1JrNndRMEZVaWM1QWZKY2pKRjR5?=
 =?utf-8?B?MThEWFprWE1iSjdoVEQxVmlZaXVRMmd1eDNkTStzKzBlUE1TdG5Mc3BFYVA0?=
 =?utf-8?B?VlkyczBBSmhzK0JZR0t6Ly9ITnMxS053VFl1U2JIbUdqcHVWNFVkRDg5ZUZz?=
 =?utf-8?B?TmNZakhQUUVzUHplNFVGSzJJMzhvc1FWdVRpcDFnOUJzRDFFOTZyUFpwQ0JQ?=
 =?utf-8?B?dGYvVkNxNWZwQXVvSnFJMkM0VjZVVnZUc3JMcWNtcHpZTHlYMFN3WGNFdC81?=
 =?utf-8?B?S0xzVS9ZU29FZlhQeFpvVUNXSDdTa2RJT2JHRmZPbFJCdDBHZ1MzL1pKRTlq?=
 =?utf-8?B?UDg5ZGNWanh6OVdZT2huZTJjek51MEZiRFI3bnFlMzdlZFBlMTlnOTducEdD?=
 =?utf-8?B?ZmwyRkxVREo0YnZiSGNUai94Y0NlVHowbzFsZjc2eGtOVFRuckVscHg3YkR5?=
 =?utf-8?B?bW9rWEhzZ3VKUmRKaWllbmVSV01CL0h5RzlNZ2hYNG0yVTFYQjlXbXloUTFX?=
 =?utf-8?B?RWJsbm8zR2FHelNpQitnR3JUNlNnUUx6NjUyazBOQWg3aUhpbkM0aW55Z21F?=
 =?utf-8?B?TmpPbFNic292SHoyclQrUUxzUXFQWGlUTW1icWtWY0dmbFAzanpGRUhiSHd5?=
 =?utf-8?B?dDFFYmV0dzNLWmVoeVRxcTluTzQ5SjB6VktjK25zOVJDZzhEZnhtUHdxTklu?=
 =?utf-8?B?NUlNQVQxS2tnS25XRkFrcHBVQUEvOTMrR2YzQU5DWWIwaGJlTXVXMmRjVk5t?=
 =?utf-8?B?b2llajJqbWM1K1czbnBYZjVvWEg2THkvOXpQMzlEWVdvZVcvUUFrVThxMm9H?=
 =?utf-8?B?bnpWeSsvMm12OU1sVlU0M01FMjdIYVltN1AraHdSaVJpSGpTZHJ2TThoSE5Z?=
 =?utf-8?B?dHJuWXg2NW80QjMzYm5DQVNCZnVZS09RTllxc1kvQkhzQzFOR05hd2g5QU41?=
 =?utf-8?B?VmtGcmsvWkFpOGZXcG5Dc3NRMWNXS2l6b2FOMTc3T1ZEQ1JzbTR3cDZGY09h?=
 =?utf-8?B?TDlmRm5zc0lGa29BNWl1MkZjMFhoT0x3b1FoQlRwdm5nWG43YVlDUVpiODdQ?=
 =?utf-8?B?UUhyNUNRSW9QK21aTUM1Y1JPYmlaVCthakhaRE91WkRIUGNMQWlSRjErVE5i?=
 =?utf-8?B?bVE9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 3f67b7c9-d717-4578-09f6-08dab5df21cf
X-MS-Exchange-CrossTenant-AuthSource: CY4PR11MB1862.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 24 Oct 2022 16:45:17.7616
 (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: FJi6SznpfoZMYtI427rCI8w3GN6GHKBqllpPiQw6pc5NkSyQQ6qLWQNLVektkNf18qvQrg1hwwb8rNGkOCkfFbCcHA6P9wvEYR5lJ5pIE3c=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS7PR11MB6245
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-4.9 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 Shawn,

On 10/23/2022 7:31 PM, Shawn Wang wrote:
> On 10/22/2022 2:05 AM, Reinette Chatre wrote:
> 
> ...
> 
>>> It may not be enough to just clear staged_config[] when
>>> resctrl_arch_update_domains() exits. I think the fix needs to make
>>> sure staged_config[] can be cleared where it is set.
>>>
>>> The modification of staged_config[] comes from two paths:
>>>
>>> Path 1:
>>> rdtgroup_schemata_write() {
>>>      ...
>>>      rdtgroup_parse_resource()     // set staged_config[]
>>>      ...
>>>      resctrl_arch_update_domains()     // clear staged_config[]
>>>      ...
>>> }
>>>
>>> Path 2:
>>> rdtgroup_init_alloc() {
>>>      ...
>>>      rdtgroup_init_mba()/rdtgroup_init_cat()    // set staged_config[]
>>>      ...
>>>      resctrl_arch_update_domains()        // clear staged_config[]
>>>      ...
>>> }
>>>
>>> If we clear staged_config[] in resctrl_arch_update_domains(), goto
>>> statement for error handling between setting staged_config[] and
>>> calling resctrl_arch_update_domains() will be ignored. This can still
>>> remain the stale staged_config[].
>> ah - indeed. Thank you for catching that.
>>
>>>
>>> I think maybe it is better to put the clearing work where
>>> rdtgroup_schemata_write() and rdtgroup_init_alloc() exit.
>>>
>>
>> It may be more robust to let rdtgroup_init_alloc() follow
>> how rdtgroup_schemata_write() already ensures that it is
>> working with a clean state by clearing staged_config[] before
>> placing its staged config within.
>>
> 
> I want to make sure, do you mean just ignore the stale value and
> place the clearing work before staged_config[] is used? If so, maybe
> the only thing the fix should do is to add memset() to
> rdtgroup_init_alloc().> 

No, let us not leave stale data lying around.

The idea is that the function calling resctrl_arch_update_domains() is
responsible for initializing staged_config[] correctly and completely.
To confirm, yes, the idea is to clear the staged_config[] in
rdtgroup_init_alloc() before resctrl_arch_update_domains() is called
to follow how it is currently done in rdtgroup_schemata_write().

But, as you indicate, by itself this would leave stale data lying around.

The solution that you suggested earlier, to put the clearing work where
rdtgroup_schemata_write() and rdtgroup_init_alloc() exit, is most logical.
That makes the code symmetrical in that staged_config[] is cleared
where it is initialized and no stale data is left lying around. What was
not clear to me is how this would look in the end. Were you planning to
keep the staged_config[] clearing within rdtgroup_schemata_write() but
not do so in rdtgroup_init_alloc()? rdtgroup_schemata_write() and
rdtgroup_init_alloc() has to follow the same pattern to reduce confusion.

So, to be more robust, how about:

/* Clear staged_config[] to make sure working from a clean slate */
resctrl_arch_update_domains()
/* Clear staged_config[] to not leave stale data lying around */

Reinette