Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:4 as permitted sender) client-ip=2620:137:e000::3:4;
Message-ID: <cddbbbae-599b-42c0-abe1-4ca74d5ce36c@intel.com>
Date:   Wed, 13 Dec 2023 15:27:54 -0800
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v7 02/24] x86/resctrl: kfree() rmid_ptrs from
 rdtgroup_exit()
Content-Language: en-US
To:     James Morse <james.morse@arm.com>, <x86@kernel.org>,
        <linux-kernel@vger.kernel.org>
CC:     Fenghua Yu <fenghua.yu@intel.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
        H Peter Anvin <hpa@zytor.com>,
        Babu Moger <Babu.Moger@amd.com>,
        <shameerali.kolothum.thodi@huawei.com>,
        D Scott Phillips OS <scott@os.amperecomputing.com>,
        <carl@os.amperecomputing.com>, <lcherian@marvell.com>,
        <bobo.shaobowang@huawei.com>, <tan.shaopeng@fujitsu.com>,
        <baolin.wang@linux.alibaba.com>,
        Jamie Iles <quic_jiles@quicinc.com>,
        Xin Hao <xhao@linux.alibaba.com>, <peternewman@google.com>,
        <dfustini@baylibre.com>, <amitsinght@marvell.com>
References: <20231025180345.28061-1-james.morse@arm.com>
 <20231025180345.28061-3-james.morse@arm.com>
 <208c3ade-a8c3-41cc-b136-4ab9b7e938e5@intel.com>
 <bd3afbfd-3372-cac9-600e-ace19ddd3199@arm.com>
From:   Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <bd3afbfd-3372-cac9-600e-ace19ddd3199@arm.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?Z0NuT1ZLckhqbXoxeFNQdnNkRGtIL0FESU5mS0FnVDdqOEFnNnFJdHd3TmpF?=
 =?utf-8?B?eThIN3hzRm8zSkhoZFp2aXJPM0lYQXlUSFFYRGdnb3IyVU94NjRQU05DbmUx?=
 =?utf-8?B?YUgxNTZORmxta2J3T1g5NDJKWHB1UkNTdy8vaDRzRnRjMFJPYzV0ckgyK3Ax?=
 =?utf-8?B?Zlh1eTVuYnV2YTBxaWJDaTA3c0x6eTdqNTlsWFZWbHh3RUFrTVludDNzTGxs?=
 =?utf-8?B?dnp6ZlROM1ZrWG5jSHRpdVhuVEVvcXB6YU4rdkw2Z2J4bFBhVHRLblB1UnhF?=
 =?utf-8?B?SjRvTWNOUktnZFg0bFVMWjVtK2RHcHNhTVl0V0VFcmt2TmwycW0yN1l0bmNP?=
 =?utf-8?B?eXd6SHVNU29adDhQQ2NRcXI4dTJZUUUraW5RRXNCalN3Vm1OL3B2YXhNRGV3?=
 =?utf-8?B?STUwWllMVnhDaVF2Mlc0d0x0MTRWRTlGZ1RocHp4WU4rSTlZRU1UQitRTnlu?=
 =?utf-8?B?bFZzU3hKNlFMOWpNclg5dythVXptQm8vMmFBb0dUbzZ3Qis5ZUF1UVdrWHQw?=
 =?utf-8?B?Z25XZjBxRS9uVXVVVzYwKzIvRnRuRVpYa3h6Zk5ES1ZxMk9yWmFMV2pxOWNO?=
 =?utf-8?B?SW51R0RKeEJwb1MxUXRnajRnOXl1NDZPdG1qTFZSQWlFL3Q1UHN0dzFXYzUz?=
 =?utf-8?B?S1RiSlVXNmZ3bENqcUptMkxhYWdIZnJLb1dlQmdzN2VFaUxwYkR1M1lOem93?=
 =?utf-8?B?QlVmenQ3Nmw4dUdUOUVuOHdkK3FvSUREMkRQOThFVW5JSm1hY014NkhRLzdw?=
 =?utf-8?B?QUZXd1RrOFJXZVJBdkVnVlE5RXdLQWZyY2o4bWdCMzJEMmxoUjlrbVA0TmpB?=
 =?utf-8?B?ZEFUUHgzRmc5ZW54SktERDc3OWJ6OEYrOTVnbHloSjJwa3liemU3UWkxVG5P?=
 =?utf-8?B?K3RzRHQ1MzdPaXY2NTZQQTRoQjRxelNqcFlDYUpaYjU5WURZYkhqMTZ2RnFr?=
 =?utf-8?B?ZWFjdXBRSTgwTEVnbFNCdEl3QUpwZTRKbllXY3hpZHExYUtlK2xrcmljNUY5?=
 =?utf-8?B?TnllWFUwTDI4ZG9kanVaRUt2VUlDdWM1ckdqeFRPQkFEMEZvakdRS1pCWDV6?=
 =?utf-8?B?TDczYnRFeTdIZjNCTDNReStVMzVDTWx5M0gzWWxhbkNiSlRyWm96V3V4d1g0?=
 =?utf-8?B?ZlYzdmNWVDNtZnA0T0Z0VDlJbXl0dXVOQ2VhdEI3YnowSzV6eUJHc3o5dzA4?=
 =?utf-8?B?Sjh5cld6M21NbW1kbmR5b2FWQmVzTlN5ZW1nRHNZMFpqNG52bFArSnRIOU5Z?=
 =?utf-8?B?N0JZRkkwbGZDaDluZGhobGJhRzAvRnV5N21LYzJhNGVmczZiMktlb0xxMnpm?=
 =?utf-8?B?bEw1N3EzWXVhMGhuQ1Q5MEtWZ1N3ZEZkd3A2SGRsZ1RQaTNBWjhTMVptUW9G?=
 =?utf-8?B?cU15NnpreGY4N3l4R0NuL0NHU2hHZFBtUXEwenBNOUxpVUErS09jNVNnK1Zx?=
 =?utf-8?B?Znk0bnEwUWVQZ016RVBEMkE4VU9yRElZQzZVd3l4Z0V4OGVrSXoxYkt0U3VG?=
 =?utf-8?B?MGhkclNOZDV5My9tL1NGR1BPNzlIRkRJcW5qODJiT3JHQlhhRmZ2aG5JaUJO?=
 =?utf-8?B?U29SSC9vVithcDZIQlY0aXdzVGxnNDhrQWVlYXNUa2R6aUlCTk01UlhqQ2oy?=
 =?utf-8?B?NHdjV1Q1dThZaTd5dUV2bkJ0dXJYWDdPaW9YRWtRRFFIU05ENGtyR3M0RWFT?=
 =?utf-8?B?RDBLVG1rYkR1ZzVMWmVoV01SSlRoOGV4SXltbHdnVTF5WWJGSytOWDdLbmVH?=
 =?utf-8?B?ZVhWTDN3aHMvQ21xZFZIeU8yT3F3dUxLcnZmYXE4cjBpK1B2dE5XaGg0Wk50?=
 =?utf-8?B?M1Q0NjIwRzM5dndsaUhMQnlXVTFTemFaQ2FkQkF0R3BEVEVlZnNVbVprNnZQ?=
 =?utf-8?B?eWM3a1ZxZmg4NlNzT0NKVjRiTUd2THdxb1lLeTg5WllidEx4eDlydThqSnhW?=
 =?utf-8?B?eDZaakVxcjR2dHdXa2tsczRTSDBEVEJIOC9nT20wcHN0a3lJbjNHRGZIQ0pi?=
 =?utf-8?B?MVBIb2hnQ1JORWMzV2hxZjB1aGRTaDZJdC9uWDByOVZWc2dBZ2JISWxJQUxj?=
 =?utf-8?B?QmhlTEtvaUY2anVjVjNzRHlZY2swYm1qQWx1S0lUWXIwZGlvRGdVb0VZSzRq?=
 =?utf-8?B?ZjkxbjRocTI4dlI5djNvL2VRcXUxV3VGWWIrYmIxRFZOZG5Bb2hyanAySkNG?=
 =?utf-8?B?U0E9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: c38d0aeb-6360-439b-9fae-08dbfc332352
X-MS-Exchange-CrossTenant-AuthSource: SJ2PR11MB7573.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 13 Dec 2023 23:27:57.0481
 (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: OjHkdq9JVXB86BPM8xzAgR1oMREqSr50UIOuA3XC5xnbcxCVmo3UrEmNZrNZ6zepcuxni4Yn2Mx0pIoLnB732ymG6RCK+9W4JZloEK9umvs=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: IA1PR11MB6097
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-0.9 required=5.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED,
	DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,
	MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE
	autolearn=unavailable autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on howler.vger.email
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (howler.vger.email [0.0.0.0]); Wed, 13 Dec 2023 15:28:34 -0800 (PST)

Hi James,

On 12/13/2023 10:03 AM, James Morse wrote:
> On 09/11/2023 17:39, Reinette Chatre wrote:
>> On 10/25/2023 11:03 AM, James Morse wrote:

...

>>> diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
>>> index 19e0681f0435..0056c9962a44 100644
>>> --- a/arch/x86/kernel/cpu/resctrl/core.c
>>> +++ b/arch/x86/kernel/cpu/resctrl/core.c
>>> @@ -992,7 +992,13 @@ late_initcall(resctrl_late_init);
>>>  
>>>  static void __exit resctrl_exit(void)
>>>  {
>>> +	struct rdt_resource *r = &rdt_resources_all[RDT_RESOURCE_L3].r_resctrl;
>>> +
>>>  	cpuhp_remove_state(rdt_online);
>>> +
>>> +	if (r->mon_capable)
>>> +		rdt_put_mon_l3_config(r);
>>> +
>>>  	rdtgroup_exit();
>>>  }
>>
>> I expect cleanup to do the inverse of init. I do not know what was the
>> motivation for the rdtgroup_exit() to follow cpuhp_remove_state()
> 
> This will invoke the hotplug callbacks, making it look to resctrl like all CPUs are
> offline. This means it is then impossible for rdtgroup_exit() to race with the hotplug
> notifiers. (if you could run this code...)
> 

hmmm ... if there is a risk of such a race would the init code not also be
vulnerable to that with the notifiers up before rdtgroup_init()? The races you mention
are not obvious to me. I see the filesystem and hotplug code protected against races via
the mutex and static keys. Could you please elaborate on the flows of concern?

I am not advocating for cpuhp_remove_state() to be called later. I understand that
it simplifies the flows to consider.

>> but I
>> was expecting this new cleanup to be done after rdtgroup_exit() to be inverse
>> of init. This cleanup is inserted in middle of two existing cleanup - could
>> you please elaborate how this location was chosen?
> 
> rdtgroup_exit() does nothing with the resctrl structures, it removes sysfs and debugfs
> entries, and unregisters the filesystem.
> 
> Hypothetically, you can't observe any effect of the rmid_ptrs array being freed as
> all the CPUs are offline and the overflow/limbo threads should have been cancelled.
> Once cpuhp_remove_state() has been called, this really doesn't matter.

Sounds like nothing prevents this code from following the custom of cleanup to be
inverse of init (yet keep cpuhp_remove_state() first).

Reinette