Thanks Michal for the reviews!!
On 7/28/2022 8:07 PM, Michal Hocko wrote:
>> FAQ's:
>> Q) Should page_ext_[get|put]() needs to be used for every page_ext
>> access?
>> A) NO, the synchronization is really not needed in all the paths of
>> accessing page_ext. One case is where extra refcount is taken on a
>> page for which memory block, this pages falls into, offline operation is
>> being performed. This extra refcount makes the offline operation not to
>> succeed hence the freeing of page_ext. Another case is where the page
>> is already being freed and we do reset its page_owner.
> This is just subtlety and something that can get misunderstood over
> time. Moreover there is no documentation explaining the difference.
> What is the reason to have these two different APIs in the first place.
> RCU read side is almost zero cost. So what is the point?
Currently not all the places where page_ext is being used is put under
the rcu_lock. I just used rcu lock in the places where it is possible to
have the use-after-free of page_ext. You recommend to use rcu lock while
using with page_ext in all the places?
My only point here is since there may be a non-atomic context exist
across page_ext_get/put() and If users are sure that this page's
page_ext will not be freed by parallel offline operation, they need not
get the rcu lock.
I agree that this can be misunderstood over time, let me check if I can
use page_ext_get/put in all the places.
>> @@ -57,6 +60,11 @@ static inline void page_ext_init(void)
>>
>> struct page_ext *lookup_page_ext(const struct page *page);
>>
>> +static inline bool page_ext_invalid(struct page_ext *page_ext)
>> +{
>> + return !page_ext || (((unsigned long)page_ext & PAGE_EXT_INVALID) == 1);
>> +}
>> +
> No real reason to expose this into a header file. Nothing but page_ext.c
> should know and care about this.
Agree. Will move it accordingly.
>
>> +static inline struct page_ext *page_ext_get(struct page *page)
>> +{
>> + struct page_ext *page_ext;
>> +
>> + rcu_read_lock();
>> + page_ext = lookup_page_ext(page);
>> + if (!page_ext) {
>> + rcu_read_unlock();
>> + return NULL;
>> + }
>> +
>> + return page_ext;
> If you make this an extern you can actually hide lookup_page_ext and
> prevent from future bugs where people are using non serialized API
> without realizing that.
This design looks good. Let me check the feasibility in its implementation.
>> diff --git a/mm/page_ext.c b/mm/page_ext.c
>> index 3dc715d..404a2eb 100644
>> --- a/mm/page_ext.c
>> +++ b/mm/page_ext.c
>> @@ -211,15 +211,17 @@ struct page_ext *lookup_page_ext(const struct page *page)
>> {
>> unsigned long pfn = page_to_pfn(page);
>> struct mem_section *section = __pfn_to_section(pfn);
>> + struct page_ext *page_ext = READ_ONCE(section->page_ext);
>> +
> WARN_ON_ONCE(!rcu_read_lock_held());
Again this requires page_ext usage should be under the rcu lock always
by the user.
>
>> static void *__meminit alloc_page_ext(size_t size, int nid)
>> @@ -298,9 +300,26 @@ static void __free_page_ext(unsigned long pfn)
>> ms = __pfn_to_section(pfn);
>> if (!ms || !ms->page_ext)
>> return;
>> - base = get_entry(ms->page_ext, pfn);
>> +
>> + base = READ_ONCE(ms->page_ext);
>> + if (page_ext_invalid(base))
>> + base = (void *)base - PAGE_EXT_INVALID;
> All page_ext accesses should use the same fetched pointer including the
> ms->page_ext check. Also page_ext_invalid _must_ be true here otherwise
> something bad is going on so I would go with
> if (WARN_ON_ONCE(!page_ext_invalid(base)))
> return;
> base = (void *)base - PAGE_EXT_INVALID;
The roll back operation in the online_page_ext(), where we free the
allocated page_ext's, will not have the PAGE_EXT_INVALID flag thus
WARN() may not work here. no?
>
Thanks,
Charan
