Re: [PATCH 1/4] mm: page_ext: add an iteration API for page extensions

[David Hildenbrand <david@redhat.com>]

>>> +    for (__iter.index = 0;                                 \
>>> +        __page_ext && __iter.index < __pgcount;        \
>>> +        __page_ext = page_ext_iter_next(&__iter),      \
>>> +        __iter.index++)
>>
>> Hm, if we now have an index, why not turn iter.pfn -> iter.start_pfn, and only adjust the index in page_ext_iter_next?
>>
>> Then you can set the index to 0 in page_ext_iter_begin() and have here
>>
>> for (__page_ext = page_ext_iter_begin(&__iter, __page),
>>        __page_ext && __iter.index < __pgcount,
>>        __page_ext = page_ext_iter_next(&__iter);)
> 
> I can do this if you feel strong about it, but I prefer explicitly over
> implicitly. I moved the index into the iter object just to avoid having
> to define it in the macro's body. Also, the way I did it allows for
> using page_ext_iter_begin()/page_ext_iter_next() own their if the need
> arises.

Ah, I see what you mean.

for (__page_ext = page_ext_iter_begin(&__iter, __page, __pgcount);
      __page_ext;
      __page_ext = page_ext_iter_next(&__iter))

Could do that I guess by moving the count in there as well and 
performing the check+increment in page_ext_iter_next.

That looks very clean to me, but no strong opinion. Having the index in 
there just to make a macro happy is rather weird.

> 
>> A page_ext_iter_reset() could then simply reset the index=0 and
>> lookup the page_ext(start_pfn + index) == page_ext(start_pfn)
> 
> Just note we don't have page_ext_iter_reset() today (and I guess it's
> not needed).

Right, was writing this before reviewing the other patch.

> 
>>> +
>>> +/**
>>> + * for_each_page_ext_order(): iterate through page_ext objects
>>> + *                            for a given page order
>>> + * @__page: the page we're interested in
>>> + * @__order: page order to iterate through
>>> + * @__page_ext: struct page_ext pointer where the current page_ext
>>> + *              object is returned
>>> + * @__iter: struct page_ext_iter object (defined in the stack)
>>> + *
>>> + * IMPORTANT: must be called with RCU read lock taken.
>>> + */
>>> +#define for_each_page_ext_order(__page, __order, __page_ext, __iter) \
>>> +    for_each_page_ext(__page, (1UL << __order), __page_ext, __iter)
>>> +
>>>    #else /* !CONFIG_PAGE_EXTENSION */
>>>    struct page_ext;
>>> diff --git a/mm/page_ext.c b/mm/page_ext.c
>>> index 641d93f6af4c1..508deb04d5ead 100644
>>> --- a/mm/page_ext.c
>>> +++ b/mm/page_ext.c
>>> @@ -549,3 +549,44 @@ void page_ext_put(struct page_ext *page_ext)
>>>        rcu_read_unlock();
>>>    }
>>> +
>>> +/**
>>> + * page_ext_iter_begin() - Prepare for iterating through page extensions.
>>> + * @iter: page extension iterator.
>>> + * @page: The page we're interested in.
>>> + *
>>> + * Must be called with RCU read lock taken.
>>> + *
>>> + * Return: NULL if no page_ext exists for this page.
>>> + */
>>> +struct page_ext *page_ext_iter_begin(struct page_ext_iter *iter, struct page *page)
>>> +{
>>> +    iter->pfn = page_to_pfn(page);
>>> +    iter->page_ext = lookup_page_ext(page);
>>> +
>>> +    return iter->page_ext;
>>> +}
>>> +
>>> +/**
>>> + * page_ext_iter_next() - Get next page extension
>>> + * @iter: page extension iterator.
>>> + *
>>> + * Must be called with RCU read lock taken.
>>> + *
>>> + * Return: NULL if no next page_ext exists.
>>> + */
>>> +struct page_ext *page_ext_iter_next(struct page_ext_iter *iter)
>>> +{
>>> +    if (WARN_ON_ONCE(!iter->page_ext))
>>> +        return NULL;
>>> +
>>> +    iter->pfn++;
>>   > +> +    if (page_ext_iter_next_fast_possible(iter->pfn)) {
>>> +        iter->page_ext = page_ext_next(iter->page_ext);
>>> +    } else {
>>> +        iter->page_ext = lookup_page_ext(pfn_to_page(iter->pfn));
>>> +    }
>>> +
>>> +    return iter->page_ext;
>>> +}
>>
>> We now always have a function call when calling into page_ext_iter_next(). Could we move that to the header and rather expose lookup_page_ext() ?
> 
> I personally don't like over-using inline functions, also I don't think this
> code needs optimization since the current clients make the affected code paths
> slow anyways (and this also applies to the likely/unlikely use in page_owner
> and page_table_check, I'd drop all of them if you ask me). But again, I can
> change if this would prevent you from giving your ACK :)

Well, 512^512 function calls for a 1 GiB page just to traverse the page 
ext? :)

-- 
Cheers,

David / dhildenb