netdev - Re: [net-next PATCH 1/6] net: Add skb_free_frag to replace use of put

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <554A793F.3070001@redhat.com>
Date:	Wed, 06 May 2015 13:27:43 -0700
From:	Alexander Duyck <alexander.h.duyck@...hat.com>
To:	Andrew Morton <akpm@...ux-foundation.org>
CC:	linux-mm@...ck.org, netdev@...r.kernel.org, davem@...emloft.net,
	Eric Dumazet <eric.dumazet@...il.com>
Subject: Re: [net-next PATCH 1/6] net: Add skb_free_frag to replace use of
 put_page in freeing skb->head



On 05/06/2015 12:38 PM, Andrew Morton wrote:
> On Mon, 04 May 2015 16:14:48 -0700 Alexander Duyck <alexander.h.duyck@...hat.com> wrote:
>
>> +/**
>> + * skb_free_frag - free a page fragment
>> + * @head: virtual address of page fragment
>> + *
>> + * Frees a page fragment allocated out of either a compound or order 0 page.
>> + * The function itself is a hybrid between free_pages and free_compound_page
>> + * which can be found in mm/page_alloc.c
>> + */
>> +void skb_free_frag(void *head)
>> +{
>> +	struct page *page = virt_to_head_page(head);
>> +
>> +	if (unlikely(put_page_testzero(page))) {
>> +		if (likely(PageHead(page)))
>> +			__free_pages_ok(page, compound_order(page));
>> +		else
>> +			free_hot_cold_page(page, false);
>> +	}
>> +}
> Why are we testing for PageHead in here?  If the code were to simply do
>
> 	if (unlikely(put_page_testzero(page)))
> 		__free_pages_ok(page, compound_order(page));
>
> that would still work?

My assumption was that there was a performance difference between 
__free_pages_ok and free_hot_cold_page for order 0 pages.  From what I 
can tell free_hot_cold_page will do bulk cleanup via free_pcppages_bulk 
while __free_pages_ok just calls free_one_page.

> There's nothing networking-specific in here.  I suggest the function be
> renamed and moved to page_alloc.c.  Add an inlined skb_free_frag() in a
> net header which calls it.  This way the mm developers know about it
> and will hopefully maintain it.  It would need a comment explaining
> when and why people should and shouldn't use it.

That's true.  While I am at it I should probably pull the allocation out 
as well just so it is all in one location.

> The term "page fragment" is a net thing and isn't something we know
> about.  What is it?  From context I'm thinking a definition would look
> something like
>
>    An arbitrary-length arbitrary-offset area of memory which resides
>    within a 0 or higher order page.  Multiple fragments within that page
>    are individually refcounted, in the page's reference counter.
>
> Is that correct and complete?

Yeah that is pretty complete.  I've added Eric who originally authored 
this to the Cc in case there is something he wants to add. I'll see 
about updating this and will likely have a v2 ready in the next couple 
of hours.

- Alex
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html