Thanks Pavan for the comments!!
On 7/18/2022 11:41 AM, Pavan Kondeti wrote:
>> diff --git a/include/linux/page_ext.h b/include/linux/page_ext.h
>> index fabb2e1..df5d353 100644
>> --- a/include/linux/page_ext.h
>> +++ b/include/linux/page_ext.h
>> @@ -64,6 +64,25 @@ static inline struct page_ext *page_ext_next(struct page_ext *curr)
>> return next;
>> }
>>
>> +static inline struct page_ext *get_page_ext(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;
>> +}
>> +
>> +static inline void put_page_ext(void)
>> +{
>> + rcu_read_unlock();
>> +}
>> +
> Would it be a harm if we make lookup_page_ext() completely a private function?
> Or is there any codepath that have the benefit of calling lookup_page_ext()
> without going through get_page_ext()? If that is the case, we should add
> RCU lockdep check inside lookup_page_ext() to make sure that this function is
> called with RCUs.
IIUC, the synchronization is really not needed in all the paths of
accessing the page_ext thus hiding the lookup_page_ext and forcing the
users to always rely on get and put functions doesn't seem correct to me.
Some example code paths where you don't need the synchronization while
accessing the page_ext are:
1) In migration (where we also migrate the page_owner information), we
take the extra refcount on the source and destination pages and then
start the migration. This extra refcount makes the test_pages_isolated()
function to fail thus retry the offline operation.
2) In free_pages_prepare(), we do reset the page_owner(through page_ext)
which again doesn't need the protection to access because the page is
already freeing (through only one path).
Thus I don't find the need of rcu lockdep check in the lookup_page_ext.
Any other paths that I am missing to add protection while page_ext
access, please let me know.
Thanks,
Charan
