Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.38 as permitted sender) client-ip=23.128.96.38;
Message-ID: <c53872e2-1d2e-44b3-80f1-e39fb0a7330d@intel.com>
Date:   Thu, 14 Dec 2023 11:06:04 -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>
 <cddbbbae-599b-42c0-abe1-4ca74d5ce36c@intel.com>
 <a5ef6b40-a9b3-5338-a12a-6a4540cda861@arm.com>
From:   Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <a5ef6b40-a9b3-5338-a12a-6a4540cda861@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?V2sraXl3ODlic3cvYUFESEUxWGRoM3F2S3FlNXpDQnZlb1dyUlV1VkdCRERP?=
 =?utf-8?B?M2kwbUlPUVU4NjF3Q2N0ZldHblJaM09LeThHMVlzTjNYMWhFN0RkNmVlYnhr?=
 =?utf-8?B?cHRxYjUyTGtSWW9oZ1d0Y0hJVFloU0ZLQ2JoWUkwLzZMVnVzcUZCVzdQZ3NS?=
 =?utf-8?B?TXg5U3BtbnJZVUgyQ1diemdNd3VadG9xWS9BeVEyc0YvbWovRDVuekpiNmRh?=
 =?utf-8?B?dGNPdDNlemlQNElnaS9IN1krYUVqbGNRVnUxK0RSSWhkRGlLd0kvMVdveTBk?=
 =?utf-8?B?bTRZUGtsSXkzMkFFbWVvblZVRG04TW9rUHdTTVBLK0g1aXZBMVVxZFhUSm1w?=
 =?utf-8?B?RlJ1OW9EQ1FlaUVpK3BBcnZqYm5henpOZVV6YVlXWDQ4MGY4YVdZMCt3alpr?=
 =?utf-8?B?VGdacW82Q0FDeGdKc0ZFakNUbCtGMlN3MHRRNXFhQjc4Umw1SEVUTUQ0TkV2?=
 =?utf-8?B?TzhCSGNUNWdpMzFDTFlYdFU0NmhPRjJTSFp6dG50Y0Uvd21WZGdaMjNEYjBN?=
 =?utf-8?B?dldoblFjdTd1UGI4TUdrOGMwaC9RR2xiQzloMWhzNkFXblFRRzNoR2NBRW13?=
 =?utf-8?B?UDdDVEpMdU9iQ2lLeFVieTNtY1ZMck4razl4WEVobE9HWTJDaHRvVVpHR3ZS?=
 =?utf-8?B?TElnYmtTejNxUnlBV3BYYmZiY1lGY1JUbXVoZFE0a3k0dWJ1dSsxMGVaR2o3?=
 =?utf-8?B?eTExdktQRHI3dGlnMzNQYnNnTEE3QXl0NWNSSC9LZXRleDhCTkFZLy9kOVJO?=
 =?utf-8?B?MTN0dWVCS29FTzNUU3NLUjF5c1FSNThqTDUwa1VMdlIzVElScFdkZEtmcE80?=
 =?utf-8?B?c2R3VEtNazFna0g2R1lUMDA0MEJwZGtNZzdYcXZRUlpvTERtcC9wbTFjaHJo?=
 =?utf-8?B?WTFxb3ZsVy9ITkVYaTFid3RkaldPUkFkK0FOWmFqM0s4RFdUajgzWk14QzE0?=
 =?utf-8?B?RGlUd2R2ak5sM2k5SWoxQ2FRSEg4NVRIOXZYTU1zK2ozNC9DZk9USmNRTVFV?=
 =?utf-8?B?RmN6QlhBKzBNSGpEZmVoRTlCWlJxQW5PSnRpS1dBY1ZudXZRVnZEb0pHUFlm?=
 =?utf-8?B?M1M4Qy9KSjI3UFdQYzJMbkliS3UxdjVhTTBLT09FR3JxV1hRYWw5dU9razlL?=
 =?utf-8?B?Q28ySGRXczcwa2FkbTJJbklDeEZaVDhOQWhYa1ROSlpHVkFaSDkvMnBwTkFI?=
 =?utf-8?B?WjMySzZReFVub2J0UWZuZ3VWV0F5dmFrV2kzMXVuaDhHSDQ2NThvSGx3S2RX?=
 =?utf-8?B?bCtxeVdjSWxCdlVmUHhiMVk3M0pXOG5CVkhROXR3dzZTeHFuSElHVVdEZmlP?=
 =?utf-8?B?L0RPeSt5aC84Z3grTi90UWVsMkNFTWFEQmhsQk40WHUyUk4vR0NYLzBHN2wx?=
 =?utf-8?B?dE9EZ1k0R1ZpUXh4aGlGYndPMHRWdnFZVTZKdWlBMHlSVVIwbHpxckVYUU9x?=
 =?utf-8?B?Tk1aZ05kMUZ1TVFWYTZlcnRvQVVRSDNNcnVmTE9VMTAvMlBQTEZZM2taSzFu?=
 =?utf-8?B?LzdtaTYraDJZb0h5elYxclpPNVM1bllQRE5OTjEzQkNGMko3RCtmM2N3Zkl0?=
 =?utf-8?B?bzNsRXZ5aTliWHRMZnJmSHE4dUdWQTdsTDRsenJKQWtDOUxxY1lFeVVqeHlI?=
 =?utf-8?B?Y3lXZ2FwK1NhQUFWTFk5aWVFYytKOURlY3RmREtCMERRTE5NRzhweHRodEVl?=
 =?utf-8?B?aXNaZmdBeGZXcFd2eDV3WWR4b2dKc3JNclBYaE9RZnFzVVNPZm9wcVpmV2NB?=
 =?utf-8?B?Y3h3RkhTSnczeEU5aFNxYUFzNFpjSzd6VER4SVRCQXN6YmYwUzVENytlRnJV?=
 =?utf-8?B?Wm1seDFmL0pnT3NsZEpHaWVHeXd0bWQreExYcHBQMmtsMUpmS1JOTW44eUNU?=
 =?utf-8?B?QkxMTFNqM3RrRXduM0tSWWZVVWpiRGwwSmVWbWN1TXNUb3ZNeDdrcExpeWY1?=
 =?utf-8?B?dkk4T0RETW5nV3l2NEZMajB3b3N0b0FXcjI0b1pUTmlXNjM5Rm10aXNud3Vo?=
 =?utf-8?B?a0puQVdVZlZxVmdnb01ZWmx2ckUwNVJlUnBzS0I0dms3WG1mdG4wZDhjZjdN?=
 =?utf-8?B?MEt5Z1VybEh6SExob3VPb0F1MkoyV255aU96RmRBamEzUmJMT3ZSRDNJemVI?=
 =?utf-8?B?bVBabnFJMjFyTzNyeVRmSEtaRmpaQXZlV0d2Rk0yUStwVnV4ZjlQNnR6U2Fi?=
 =?utf-8?B?YlE9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 19101992-a2ee-4bac-75a6-08dbfcd7ba64
X-MS-Exchange-CrossTenant-AuthSource: SJ2PR11MB7573.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 14 Dec 2023 19:06:07.9939
 (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: iWxyT2193EDifhAkHRXRwKb3erx3Hh301SvuzqebvRHQkuJ3bme/0xbOyLDaekQiI7GWVSofhvD8fa7rfSeLyoJKqff2wZP4xxIl4caETBY=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN9PR11MB5466
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 fry.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 (fry.vger.email [0.0.0.0]); Thu, 14 Dec 2023 11:06:49 -0800 (PST)

Hi James,

On 12/14/2023 10:28 AM, James Morse wrote:
> Hi Reinette,
> 
> On 13/12/2023 23:27, Reinette Chatre wrote:
>> 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()?
> 
> Nope, because this array is allocated behind rdt_get_mon_l3_config(), which ultimately
> comes from get_rdt_resources() in resctrl_late_init() - which calls cpuhp_setup_state()
> after all this init work has been done.
> 
> (cpu hp always gives me a headache1)

Right. My comment was actually and specifically about rdtgroup_init() and attempting to
understand your view of its races with the hotplug notifiers in response to your comment about
its (the hotplug notifiers) races with rdtgroup_exit().

The current order of state initialization you mention and hotplug notifiers needing the
state is sane and implies to expect an inverse order of teardown.

>> 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?
> 
> Functions like __check_limbo() (calling __rmid_entry()) are called under the
> rdtgroup_mutex, but they don't consider that rmid_ptrs[] may be NULL.
> 
> But this could only happen if the limbo work ran after cpuhp_remove_state() - this can't
> happen because the hotplug callbacks cancel the limbo work, and won't reschedule it if the
> domain is going offline.
> 
> 
> The only other path is via free_rmid(), I've not thought too much about this as
> resctrl_exit() can't actually be invoked - this code is discarded by the linker.
> 
> It could be run on MPAM, but only in response to an 'error interrupt' (which is optional)
> - and all the MPAM error interrupts indicate a software bug.

This still just considers the resctrl state and hotplug notifiers.

I clearly am missing something. It is still not clear to me how this connects to your earlier
comment about races with the rdtgroup_exit() code ... how the hotplug notifiers races with the
filesystem register/unregister code.

> 
> I've only invoked this path once, and rdtgroup_exit()s unregister_filesystem() didn't
> remove all the files. I anticipate digging into this teardown code more once the bulk of
> the MPAM driver is upstream.
> 
> 
>> 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).
> 
> I'll put the the rdt_put_mon_l3_config() call after rdtgroup_exit()...

thank you

Reinette