Thanks Michal for the comments!!
On 7/18/2022 5:20 PM, Michal Hocko wrote:
>> The above mentioned race is just one example __but the problem persists
>> in the other paths too involving page_ext->flags access(eg:
>> page_is_idle())__. Since offline waits till the last reference on the
>> page goes down i.e. any path that took the refcount on the page can make
>> the memory offline operation to wait. Eg: In the migrate_pages()
>> operation, we do take the extra refcount on the pages that are under
>> migration and then we do copy page_owner by accessing page_ext. For
>>
>> Fix those paths where offline races with page_ext access by maintaining
>> synchronization with rcu lock.
> Please be much more specific about the synchronization. How does RCU
> actually synchronize the offlining and access? Higher level description
> of all the actors would be very helpful not only for the review but also
> for future readers.
I will improve the commit message about this synchronization change
using RCU's.
>
> Also, more specifically
> [...]
>> diff --git a/mm/page_ext.c b/mm/page_ext.c
>> index 3dc715d..5ccd3ee 100644
>> --- a/mm/page_ext.c
>> +++ b/mm/page_ext.c
>> @@ -299,8 +299,9 @@ static void __free_page_ext(unsigned long pfn)
>> if (!ms || !ms->page_ext)
>> return;
>> base = get_entry(ms->page_ext, pfn);
>> - free_page_ext(base);
>> ms->page_ext = NULL;
>> + synchronize_rcu();
>> + free_page_ext(base);
>> }
> So you are imposing the RCU grace period for each page_ext! This can get
> really expensive. Have you tried to measure the effect?
>
I didn't really measure the effect. Let me measure it and post these in V2.
> Is there any reason why page_ext is freed during offlining rather when
> it is hotremoved?
This is something I am struggling to get the answer. IMO, this is even
wrong design where I don't have page_ext but page. Moving the freeing of
page_ext to hotremove path actually solves the problem but somehow this
idea didn't liked[1]. copying the excerpt here:
">
> 3) Change the design where the page_ext is valid as long as the struct
> page is alive.
:/ Doesn't spark joy."
@Joonsoo : We see that you did commit eefa864b701d ("mm/page_ext:
resurrect struct page extending code for debugging"). Any reason why
the page_ext is chosen to free at offline operation rather than the
remove operation of a memory block?
[1]
https://lore.kernel.org/linux-mm/8fefe59d-c893-39f4-3225-65343086c867@xxxxxxxxxx/
>
> Thanks!
