Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754889AbcKUQVn (ORCPT ); Mon, 21 Nov 2016 11:21:43 -0500 Received: from mail-it0-f66.google.com ([209.85.214.66]:33662 "EHLO mail-it0-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753120AbcKUQVl (ORCPT ); Mon, 21 Nov 2016 11:21:41 -0500 MIME-Version: 1.0 In-Reply-To: <20161118152716.3f7acf6e25f142846909b2f6@linux-foundation.org> References: <20161110113027.76501.63030.stgit@ahduyck-blue-test.jf.intel.com> <20161110113606.76501.70752.stgit@ahduyck-blue-test.jf.intel.com> <20161118152716.3f7acf6e25f142846909b2f6@linux-foundation.org> From: Alexander Duyck Date: Mon, 21 Nov 2016 08:21:39 -0800 Message-ID: Subject: Re: [mm PATCH v3 21/23] mm: Add support for releasing multiple instances of a page To: Andrew Morton Cc: Alexander Duyck , linux-mm , Netdev , "linux-kernel@vger.kernel.org" Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3847 Lines: 88 On Fri, Nov 18, 2016 at 3:27 PM, Andrew Morton wrote: > On Thu, 10 Nov 2016 06:36:06 -0500 Alexander Duyck 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