Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1525772 > unrolled thread

Re: [mm PATCH v3 21/23] mm: Add support for releasing multiple instances of a page

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [mm PATCH v3 21/23] mm: Add support for releasing multiple  instances of a page Andrew Morton <akpm@linux-foundation.org> - 2016-11-19 00:30 +0100
    Re: [mm PATCH v3 21/23] mm: Add support for releasing multiple  instances of a page Alexander Duyck <alexander.duyck@gmail.com> - 2016-11-21 17:30 +0100
      Re: [mm PATCH v3 21/23] mm: Add support for releasing multiple  instances of a page Andrew Morton <akpm@linux-foundation.org> - 2016-11-22 07:30 +0100

#1525772 — Re: [mm PATCH v3 21/23] mm: Add support for releasing multiple instances of a page

On Thu, 10 Nov 2016 06:36:06 -0500 Alexander Duyck <alexander.h.duyck@intel.com> wrote:

> This patch adds a function that allows us to batch free a page that has
> multiple references outstanding.  Specifically this function can be used to
> drop a page being used in the page frag alloc cache.  With this drivers can
> make use of functionality similar to the page frag alloc cache without
> having to do any workarounds for the fact that there is no function that
> frees multiple references.
> 
> ...
>
> --- a/include/linux/gfp.h
> +++ b/include/linux/gfp.h
> @@ -506,6 +506,8 @@ extern void free_hot_cold_page(struct page *page, bool cold);
>  extern void free_hot_cold_page_list(struct list_head *list, bool cold);
>  
>  struct page_frag_cache;
> +extern void __page_frag_drain(struct page *page, unsigned int order,
> +			      unsigned int count);
>  extern void *__alloc_page_frag(struct page_frag_cache *nc,
>  			       unsigned int fragsz, gfp_t gfp_mask);
>  extern void __free_page_frag(void *addr);
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 0fbfead..54fea40 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -3912,6 +3912,20 @@ static struct page *__page_frag_refill(struct page_frag_cache *nc,
>  	return page;
>  }
>  
> +void __page_frag_drain(struct page *page, unsigned int order,
> +		       unsigned int count)
> +{
> +	VM_BUG_ON_PAGE(page_ref_count(page) == 0, page);
> +
> +	if (page_ref_sub_and_test(page, count)) {
> +		if (order == 0)
> +			free_hot_cold_page(page, false);
> +		else
> +			__free_pages_ok(page, order);
> +	}
> +}
> +EXPORT_SYMBOL(__page_frag_drain);

It's an exported-to-modules library function.  It should be documented,
please?  The page-frag API is only partially documented, but that's no
excuse.

And perhaps documentation will help explain the naming choice.  Why
"drain"?  I'd have expected "put"?

And why the leading underscores.  The page-frag API is pretty weird :(

And inconsistent.  __alloc_page_frag -> page_frag_alloc,
__free_page_frag -> page_frag_free(), etc.  I must have been asleep
when I let that lot through.

[toc] | [next] | [standalone]

#1526874

On Fri, Nov 18, 2016 at 3:27 PM, Andrew Morton
<akpm@linux-foundation.org> wrote:
> On Thu, 10 Nov 2016 06:36:06 -0500 Alexander Duyck <alexander.h.duyck@intel.com> wrote:
>
>> This patch adds a function that allows us to batch free a page that has
>> multiple references outstanding.  Specifically this function can be used to
>> drop a page being used in the page frag alloc cache.  With this drivers can
>> make use of functionality similar to the page frag alloc cache without
>> having to do any workarounds for the fact that there is no function that
>> frees multiple references.
>>
>> ...
>>
>> --- a/include/linux/gfp.h
>> +++ b/include/linux/gfp.h
>> @@ -506,6 +506,8 @@ extern void free_hot_cold_page(struct page *page, bool cold);
>>  extern void free_hot_cold_page_list(struct list_head *list, bool cold);
>>
>>  struct page_frag_cache;
>> +extern void __page_frag_drain(struct page *page, unsigned int order,
>> +                           unsigned int count);
>>  extern void *__alloc_page_frag(struct page_frag_cache *nc,
>>                              unsigned int fragsz, gfp_t gfp_mask);
>>  extern void __free_page_frag(void *addr);
>> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
>> index 0fbfead..54fea40 100644
>> --- a/mm/page_alloc.c
>> +++ b/mm/page_alloc.c
>> @@ -3912,6 +3912,20 @@ static struct page *__page_frag_refill(struct page_frag_cache *nc,
>>       return page;
>>  }
>>
>> +void __page_frag_drain(struct page *page, unsigned int order,
>> +                    unsigned int count)
>> +{
>> +     VM_BUG_ON_PAGE(page_ref_count(page) == 0, page);
>> +
>> +     if (page_ref_sub_and_test(page, count)) {
>> +             if (order == 0)
>> +                     free_hot_cold_page(page, false);
>> +             else
>> +                     __free_pages_ok(page, order);
>> +     }
>> +}
>> +EXPORT_SYMBOL(__page_frag_drain);
>
> It's an exported-to-modules library function.  It should be documented,
> please?  The page-frag API is only partially documented, but that's no
> excuse.

Okay.  I assume you want the documentation as a follow-up patch since
I received a notice that the patch was added to -mm?

> And perhaps documentation will help explain the naming choice.  Why
> "drain"?  I'd have expected "put"?

The idea was that this is supposed to be a counterpart to
__page_frag_refill.  Basically it is a function we can use if we need
to tear down the page frag cache and free the backing page.  If you
want I could update the names for these functions to make that
clarification that this is meant to drain a frag cache versus just
freeing a page frag.  I had originally thought about coming up with an
mput or something like that since we are dropping multiple references,
but then I figured since we already had __page_frag_refill I would go
for __page_frag_drain.

> And why the leading underscores.  The page-frag API is pretty weird :(
>
> And inconsistent.  __alloc_page_frag -> page_frag_alloc,
> __free_page_frag -> page_frag_free(), etc.  I must have been asleep
> when I let that lot through.

The leading underscores are inherited.  Most of it has to do with the
fact that this is a backing API for the netdev sk_buff allocator.
When this stuff existed in net it was already named this way and I
just moved it over.  I'm not sure if you approved it or not as I don't
see an Ack-by or Signed-off-by from you on the patch.  The timing of
it was such that I think Linus approved it and it was then pulled in
through Dave's tree.

If you would like I could look at doing a couple of renaming patches
so that we make the API a bit more consistent.  I could move the
__alloc and __free to what you have suggested, and then take a look at
trying to rename the refill/drain to be a bit more consistent in terms
of what they are supposed to work on and how they are supposed to be
used.

- Alex

[toc] | [prev] | [next] | [standalone]

#1527253

On Mon, 21 Nov 2016 08:21:39 -0800 Alexander Duyck <alexander.duyck@gmail.com> wrote:

> >> +                     __free_pages_ok(page, order);
> >> +     }
> >> +}
> >> +EXPORT_SYMBOL(__page_frag_drain);
> >
> > It's an exported-to-modules library function.  It should be documented,
> > please?  The page-frag API is only partially documented, but that's no
> > excuse.
> 
> Okay.  I assume you want the documentation as a follow-up patch since
> I received a notice that the patch was added to -mm?

Yes please.  Or a replacement patch which I'll temporarily turn into a
delta, either is fine.

> If you would like I could look at doing a couple of renaming patches
> so that we make the API a bit more consistent.  I could move the
> __alloc and __free to what you have suggested, and then take a look at
> trying to rename the refill/drain to be a bit more consistent in terms
> of what they are supposed to work on and how they are supposed to be
> used.

I think that would be better - it's hardly high-priority but a bit of
attention to the documentation and naming conventions would help tidy
things up.  When you can't find anything else to do ;)

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web