| 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
| ||
|
Message-ID: <43c11982-36e9-4472-9edf-a249b442ca4b@gmail.com>
Date: Sun, 28 Jul 2024 22:12:58 +0800
From: Yunsheng Lin <yunshenglin0825@...il.com>
To: Alexander Duyck <alexander.duyck@...il.com>,
Yunsheng Lin <linyunsheng@...wei.com>
Cc: davem@...emloft.net, kuba@...nel.org, pabeni@...hat.com,
netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
Andrew Morton <akpm@...ux-foundation.org>, linux-mm@...ck.org
Subject: Re: [RFC v11 03/14] mm: page_frag: use initial zero offset for
page_frag_alloc_align()
On 7/22/2024 2:34 AM, Alexander Duyck wrote:
> On Fri, Jul 19, 2024 at 2:37 AM Yunsheng Lin <linyunsheng@...wei.com> wrote:
...
>> diff --git a/mm/page_frag_cache.c b/mm/page_frag_cache.c
>> index 609a485cd02a..2958fe006fe7 100644
>> --- a/mm/page_frag_cache.c
>> +++ b/mm/page_frag_cache.c
>> @@ -22,6 +22,7 @@
>> static struct page *__page_frag_cache_refill(struct page_frag_cache *nc,
>> gfp_t gfp_mask)
>> {
>> + unsigned int page_size = PAGE_FRAG_CACHE_MAX_SIZE;
>> struct page *page = NULL;
>> gfp_t gfp = gfp_mask;
>>
>> @@ -30,12 +31,21 @@ static struct page *__page_frag_cache_refill(struct page_frag_cache *nc,
>> __GFP_NOWARN | __GFP_NORETRY | __GFP_NOMEMALLOC;
>> page = alloc_pages_node(NUMA_NO_NODE, gfp_mask,
>> PAGE_FRAG_CACHE_MAX_ORDER);
>> - nc->size = page ? PAGE_FRAG_CACHE_MAX_SIZE : PAGE_SIZE;
>> #endif
>> - if (unlikely(!page))
>> + if (unlikely(!page)) {
>> page = alloc_pages_node(NUMA_NO_NODE, gfp, 0);
>> + if (unlikely(!page)) {
>> + nc->va = NULL;
>> + return NULL;
>> + }
>>
>> - nc->va = page ? page_address(page) : NULL;
>> + page_size = PAGE_SIZE;
>> + }
>> +
>> +#if (PAGE_SIZE < PAGE_FRAG_CACHE_MAX_SIZE)
>> + nc->size = page_size;
>> +#endif
>> + nc->va = page_address(page);
>>
>> return page;
>> }
>
> Not a huge fan of the changes here. If we are changing the direction
> then just do that. I don't see the point of these changes. As far as I
> can tell it is just adding noise to the diff and has no effect on the
> final code as the outcome is mostly the same except for you don't
> update size in the event that you overwrite nc->va to NULL.
While I am agreed the above changing is not really related to this
patch, but it does have some effect on the final code, as it seems
to avoid one extra '!page' checking:
./scripts/bloat-o-meter vmlinux.org vmlinux
add/remove: 0/0 grow/shrink: 1/0 up/down: 11/0 (11)
Function old new delta
__page_frag_alloc_align 594 605 +11
Total: Before=22083357, After=22083368, chg +0.00%
Let me see if I can move it to more related patch when refactoring.
>
>> @@ -64,8 +74,8 @@ void *__page_frag_alloc_align(struct page_frag_cache *nc,
>> unsigned int align_mask)
>> {
>> unsigned int size = PAGE_SIZE;
>> + unsigned int remaining;
>> struct page *page;
>> - int offset;
>>
>> if (unlikely(!nc->va)) {
>> refill:
>> @@ -82,35 +92,20 @@ void *__page_frag_alloc_align(struct page_frag_cache *nc,
>> */
>> page_ref_add(page, PAGE_FRAG_CACHE_MAX_SIZE);
>>
>> - /* reset page count bias and offset to start of new frag */
>> + /* reset page count bias and remaining to start of new frag */
>> nc->pfmemalloc = page_is_pfmemalloc(page);
>> nc->pagecnt_bias = PAGE_FRAG_CACHE_MAX_SIZE + 1;
>> - nc->offset = size;
>> + nc->remaining = size;
>> }
>>
>> - offset = nc->offset - fragsz;
>> - if (unlikely(offset < 0)) {
>> - page = virt_to_page(nc->va);
>> -
>> - if (!page_ref_sub_and_test(page, nc->pagecnt_bias))
>> - goto refill;
>> -
>> - if (unlikely(nc->pfmemalloc)) {
>> - free_unref_page(page, compound_order(page));
>> - goto refill;
>> - }
>> -
>> #if (PAGE_SIZE < PAGE_FRAG_CACHE_MAX_SIZE)
>> - /* if size can vary use size else just use PAGE_SIZE */
>> - size = nc->size;
>> + /* if size can vary use size else just use PAGE_SIZE */
>> + size = nc->size;
>> #endif
>
> Rather than pulling this out and placing it here it might make more
> sense at the start of the function. Basically just overwrite size w/
> either PAGE_SIZE or nc->size right at the start. Then if we have to
> reallocate we overwrite it. That way we can avoid some redundancy and
> this will be easier to read.
You meant something like below at the start of the function, it does
make more sense.
#if (PAGE_SIZE < PAGE_FRAG_CACHE_MAX_SIZE)
unsigned int size = nc->size;
#else
unsigned int size = PAGE_SIZE;
#endif
>
>> - /* OK, page count is 0, we can safely set it */
>> - set_page_count(page, PAGE_FRAG_CACHE_MAX_SIZE + 1);
>>
>> - /* reset page count bias and offset to start of new frag */
>> - nc->pagecnt_bias = PAGE_FRAG_CACHE_MAX_SIZE + 1;
>> - offset = size - fragsz;
>> - if (unlikely(offset < 0)) {
>> + remaining = nc->remaining & align_mask;
>> + if (unlikely(remaining < fragsz)) {
>> + if (unlikely(fragsz > PAGE_SIZE)) {
>> /*
>> * The caller is trying to allocate a fragment
>> * with fragsz > PAGE_SIZE but the cache isn't big
>> @@ -122,13 +117,31 @@ void *__page_frag_alloc_align(struct page_frag_cache *nc,
>> */
>> return NULL;
>> }
>> +
>> + page = virt_to_page(nc->va);
>> +
>> + if (!page_ref_sub_and_test(page, nc->pagecnt_bias))
>> + goto refill;
>> +
>> + if (unlikely(nc->pfmemalloc)) {
>> + free_unref_page(page, compound_order(page));
>> + goto refill;
>> + }
>> +
>> + /* OK, page count is 0, we can safely set it */
>> + set_page_count(page, PAGE_FRAG_CACHE_MAX_SIZE + 1);
>> +
>> + /* reset page count bias and remaining to start of new frag */
>> + nc->pagecnt_bias = PAGE_FRAG_CACHE_MAX_SIZE + 1;
>> + nc->remaining = size;
>
> Why are you setting nc->remaining here? You set it a few lines below.
> This is redundant.
Yes, it is not needed after '(fragsz > PAGE_SIZE)' after checking is
moved upward.
>
>> +
>> + remaining = size;
>> }
>>
>> nc->pagecnt_bias--;
>> - offset &= align_mask;
>> - nc->offset = offset;
>> + nc->remaining = remaining - fragsz;
>>
>> - return nc->va + offset;
>> + return nc->va + (size - remaining);
>> }
>> EXPORT_SYMBOL(__page_frag_alloc_align);
>
Powered by blists - more mailing lists