Thanks David for the inputs.
On 8/23/2022 6:39 PM, David Hildenbrand wrote:
>> static ssize_t
>> @@ -508,6 +527,14 @@ read_page_owner(struct file *file, char __user *buf, size_t count, loff_t *ppos)
>> /* Find an allocated page */
>> for (; pfn < max_pfn; pfn++) {
>> /*
>> + * This temporary page_owner is required so
>> + * that we can avoid the context switches while holding
>> + * the rcu lock and copying the page owner information to
>> + * user through copy_to_user() or GFP_KERNEL allocations.
>> + */
>> + struct page_owner page_owner_tmp;
>> +
>> + /*
>> * If the new page is in a new MAX_ORDER_NR_PAGES area,
>> * validate the area as existing, skip it if not
>> */
>> @@ -525,7 +552,7 @@ read_page_owner(struct file *file, char __user *buf, size_t count, loff_t *ppos)
>> continue;
>> }
>>
>> - page_ext = lookup_page_ext(page);
>> + page_ext = page_ext_get(page);
>> if (unlikely(!page_ext))
>> continue;
>>
>> @@ -534,14 +561,14 @@ read_page_owner(struct file *file, char __user *buf, size_t count, loff_t *ppos)
>> * because we don't hold the zone lock.
>> */
>> if (!test_bit(PAGE_EXT_OWNER, &page_ext->flags))
>> - continue;
>> + goto loop;
>>
>> /*
>> * Although we do have the info about past allocation of free
>> * pages, it's not relevant for current memory usage.
>> */
>> if (!test_bit(PAGE_EXT_OWNER_ALLOCATED, &page_ext->flags))
>> - continue;
>> + goto loop;
>>
>> page_owner = get_page_owner(page_ext);
>>
>> @@ -550,7 +577,7 @@ read_page_owner(struct file *file, char __user *buf, size_t count, loff_t *ppos)
>> * would inflate the stats.
>> */
>> if (!IS_ALIGNED(pfn, 1 << page_owner->order))
>> - continue;
>> + goto loop;
>>
>> /*
>> * Access to page_ext->handle isn't synchronous so we should
>> @@ -558,13 +585,17 @@ read_page_owner(struct file *file, char __user *buf, size_t count, loff_t *ppos)
>> */
>> handle = READ_ONCE(page_owner->handle);
>> if (!handle)
>> - continue;
>> + goto loop;
>>
>> /* Record the next PFN to read in the file offset */
>> *ppos = (pfn - min_low_pfn) + 1;
>>
>> + page_owner_tmp = *page_owner;
>> + page_ext_put(page_ext);
>> return print_page_owner(buf, count, pfn, page,
>> - page_owner, handle);
>> + &page_owner_tmp, handle);
>> +loop:
>> + page_ext_put(page_ext);
>> }
>>
>> return 0;
>> @@ -617,18 +648,20 @@ static void init_pages_in_zone(pg_data_t *pgdat, struct zone *zone)
>> if (PageReserved(page))
>> continue;
>>
>> - page_ext = lookup_page_ext(page);
>> + page_ext = page_ext_get(page);
>> if (unlikely(!page_ext))
>> continue;
>>
>> /* Maybe overlapping zone */
>> if (test_bit(PAGE_EXT_OWNER, &page_ext->flags))
>> - continue;
>> + goto loop;
>>
>> /* Found early allocated page */
>> __set_page_owner_handle(page_ext, early_handle,
>> 0, 0);
>> count++;
>> +loop:
>> + page_ext_put(page_ext);
>> }
> I kind-of dislike the "loop" labels. Can we come up with a more
> expressive name?
>
> "put_continue"
>
> or something?
>
>
> One alternative would be to add to the beginning of the loop, and after
> the loop sth like
>
> if (page_ext) {
> page_ext_put(page_ext);
> page_ext = NULL;
> }
I think, moving this to beginning of the loop looks cleaner than the
goto statement. Will spin V5.
>
> One could wrap that in a function, but not sure if that improves the
> situation.
