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: <de3d3e98-ef52-e290-f47c-717f531a0659@intel.com>
Date:   Tue, 25 Oct 2022 12:34:27 -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
Content-Language: en-US
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>
 <bbc21b48-58b5-6356-0248-656e22d95281@intel.com>
 <140177ee-da8f-c6eb-caf6-af0775a3de0e@linux.alibaba.com>
From:   Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <140177ee-da8f-c6eb-caf6-af0775a3de0e@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?bXRmUHVJaVY1VkpmRzdiTXJZQXROWlZlamczc1FNdTVrb2lMRCtaYkg3SDhN?=
 =?utf-8?B?SGhCZzBpWDdCYUNZcVdIc0J2SllCLythR1pIK3VjVnV0b25GWjZZTHg0NjRp?=
 =?utf-8?B?TEZEajdnN2R5UThtLzN3N2VlU1BWbGdHa2lJLzIzZVQweGFhbVNTRkQ4U1cw?=
 =?utf-8?B?cUFxMEJseEhXYy9VZmYzai9sY1FrTE1NckNrZUNnOCtYSGpjUG11LzB3M0Zx?=
 =?utf-8?B?SE1GZ1pvTjdId1RGNjJXSCtrdmdvcUhLcjJHa1duVmpyaHlZVXEyRmsrQTRK?=
 =?utf-8?B?czA5OCtsNVZKT1NTYk1QNGpUVmUrZmh1MjNiN1ZNb25vQlJzUTRkZHFISys0?=
 =?utf-8?B?TE11TmNOeEh3ZzZjSEFTTHF2NWNGcnh6OWFveG45SEVKOEhtRDg3eXJ1WnFH?=
 =?utf-8?B?bEt5YkNjRjlVQlh1NmNLTndMM1Y4Qmoxa25NVGFCVUNFMld5TlRUam9zM0Rs?=
 =?utf-8?B?ZHpzOWdWSVhIbnFKb3lqbU1kbjBCeUlveVVPazZyRExtQ2h3LzcwWDZYZGM4?=
 =?utf-8?B?cXFLOUpnVktwelllbFdpTXdlYWZwaG80aHZVVDhJdXlwZENaeWVGcGJlenlj?=
 =?utf-8?B?dVZSdk9iOWQzLzJwYm1mY0M5Kzl3aWhSQUFueTdQd0UyVTRMSlZuQ082VVNv?=
 =?utf-8?B?UVlJc21wMGoyTzRvTlVEVGcvbzVyVS9BMWFnS1BweTJLOXZYUEhKZnp4QUNa?=
 =?utf-8?B?cTFqNDFOSTl3RGFZaHh6UTlyZmEzN0EwcUFjbVpBTXlyVHgycXlPUVlHQUxa?=
 =?utf-8?B?T0NPSm92UkVQdkpYTUhzelBwN0F4OXQzeENMUDJkZDZubkkvT2RxOTc4YzhQ?=
 =?utf-8?B?V3loQXFDMzgxSDkwYmk5NVRPM1pOZTFOeG1YSzhaclZoUm9yekdxUmhSQjNp?=
 =?utf-8?B?WWh6b0U3V0xlYTNKdGlwNjJhMFJPRkpVNFowRHpxeHV5bkVPbEFJOWxDREsx?=
 =?utf-8?B?SVIwbEtDdldzU0lLQ0ptUmZuaTUzYUN0MXJrdVNLdUlGbnI3QUhWbWlQMUMx?=
 =?utf-8?B?TWJ1eW1NN1Y2ZzFPMm0vLzVSKzRIbGZtKzNNZTV4MXVpQ2U1K2dzeWk2TkJy?=
 =?utf-8?B?RHNIdzRvVzFaTmZ1VFNxWGV6aDdMNnhLSE5PcTVCVnd2dFVwSTF6TTcxL2d0?=
 =?utf-8?B?STYrdzF1NDVJeFo3MG5PU25xdXphTHBBc0hHRHB5eEVqdW0rSUZZREZwQUJI?=
 =?utf-8?B?dmNZV2xna0xhbDNXZHFvWUI2YWM3WHBLclo1OU9sQWRXL3JxUnBmWXJ5Sk0r?=
 =?utf-8?B?Y0NKeWxwdkVmSUU5VkM3RllzWmx1K1pzY3hVVk9aNm1TenRKQllPcmQrYlAz?=
 =?utf-8?B?TjhyZDlYS20xRGhDU2RJY3pGUVMva2JIajFRRFNIejFhQzlPZ3VpVnRPVWFK?=
 =?utf-8?B?dGVqYUt2WWtnSkFDeGN4T1VaZE5hQkQ2aE1yelA2U29UVi9lTndUelczRnJX?=
 =?utf-8?B?dlRUeXFsK0JzUjJ3Mm8wd1ZZM3ZiNmVIQ0p2Yk40SFJzcUd5ZXVDTmxiRFVz?=
 =?utf-8?B?TkNNNUpvUVp4YUR6U1F1TEVpZERnZmFxZzQrMFFib09sVk8xVndzNHZRY2ht?=
 =?utf-8?B?dHFPek9adXh4SlBtSkRsU0tOWXhnWlU5bFE5RUJQMGtrWm9pZVVncFl4bUcx?=
 =?utf-8?B?WVpaRVhVUnZrc3dDR1MvaE9GYlMxZHp0dHpWNm9MNXRrcXh2QzZXM1RTT3Nq?=
 =?utf-8?B?OG5tRzhSdTRoSkdOL1RkMzM3QjhCelFCdHV6clFNZ2VkQ0dQK1FDQlhnVHl4?=
 =?utf-8?B?VklBa0ZMSVpzWm41YWVpRDVXK1ZEbXoxNUlnZnE4NCtJK0ZyQmI3cnBIeUpT?=
 =?utf-8?B?MHhUOVFqUlBNbTdHdlBuR2t1d3o5L0t4UmlGbDR0bFM0L2xjUnZmTHZOaktG?=
 =?utf-8?B?RlNZbVZ1Zk5HcjZMWUZuTzd2SnIwYW5SNVlnTUpDMXhneUplaFRGbi9HQzNx?=
 =?utf-8?B?Ni9NUW51c29hcmowWjlEU1BKQkpjWTI3OTdmVms0ZllBQzBoTjVWdVFYUjVH?=
 =?utf-8?B?TituYTZxdGJaQjdjY2MzeDZEOWhVNUU2ekRETkFGNjBSNWR3SDY1MXYwZFQy?=
 =?utf-8?B?cERxMFd5VmhEYnBBYUVZWTV0cE5KTHRxYkxrWDNscU94ektNREZ5RVVUVm93?=
 =?utf-8?B?dFRZd2g5SGpIVWlEVCt0S2hqTElWYVRsV3VJamVoR2N3ell4YklFVWVsMUpl?=
 =?utf-8?B?cUE9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 906117f3-19e2-4ee4-69cd-08dab6bfef91
X-MS-Exchange-CrossTenant-AuthSource: CY4PR11MB1862.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 25 Oct 2022 19:34:30.3608
 (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: 74YWNfVTebO3Wtzdq3a7aACZRtSLts5jSkzmKvooNa+bl1SR1Ih/6G6eRWnTSgURSJ0rHRt0G1EGnZdlcnxa1K/WL9kECih4fkcGs5EV4zo=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR11MB5917
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,URIBL_BLOCKED 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/25/2022 8:30 AM, Shawn Wang wrote:
> Hi Reinette,
> 
> On 10/25/2022 12:45 AM, Reinette Chatre wrote:
>> 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 */
>>
> 
> Thank you for your explanation, and it makes sense to me. But this will
> require 4 memset() loops, how about putting the clearing work in
> a separate function in rdtgroup.c, like rdt_last_cmd_clear():

Yes, thanks for avoiding duplicating code.

> 
> void staged_configs_clear(void) {
>     struct resctrl_schema *s;
>     struct rdt_domain *dom;
> 
>     lockdep_assert_held(&rdtgroup_mutex);
> 
>     list_for_each_entry(s, &resctrl_schema_all, list) {
>         list_for_each_entry(dom, &s->res->domains, list)
>             memset(dom->staged_config, 0, sizeof(dom->staged_config));
>     }
> }
> 

I understand that you are just copying what is currently done in
rdtgroup_schemata_write() but for a separate function I think something
like below would be more efficient:


	for_each_alloc_capable_rdt_resource(r) {
		list_for_each_entry(dom, &r->domains, list)
			memset(dom->staged_config, 0, sizeof(dom->staged_config));
	}

This would be more efficient since it would not clean the same memory area
twice when CDP is enabled.

Reinette