linux-kernel - Re: [PATCH net-next v9 06/13] mm: page_frag: reuse existing space for 'size' and 'pfmemalloc'

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: <12ff13d9-1f3d-4c1b-a972-2efb6f247e31@gmail.com>
Date: Sun, 14 Jul 2024 12:52:18 +0800
From: Yunsheng Lin <yunshenglin0825@...il.com>
To: Alexander Duyck <alexander.duyck@...il.com>
Cc: Yunsheng Lin <linyunsheng@...wei.com>, 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: [PATCH net-next v9 06/13] mm: page_frag: reuse existing space for
 'size' and 'pfmemalloc'

On 7/14/2024 12:55 AM, Alexander Duyck wrote:

...

>>>>
>>>> Perhaps the 'remaining' changing in this patch does seems to make things
>>>> harder to discuss. Anyway, it would be more helpful if there is some pseudo
>>>> code to show the steps of how the above can be done in your mind.
>>>
>>> Basically what you would really need do for all this is:
>>>     remaining = __ALIGN_KERNEL_MASK(nc->remaining, ~align_mask);
>>>     nc->remaining = remaining + fragsz;
>>>     return encoded_page_address(nc->encoded_va) + size + remaining;
>>
> 
> I might have mixed my explanation up a bit. This is assuming remaining
> is a negative value as I mentioned before.

Let's be more specific about the options here, what you meant is below,
right? Let's say it is option 1 as below:
struct page_frag_cache {
         /* encoded_va consists of the virtual address, pfmemalloc bit 
and order
          * of a page.
          */
         unsigned long encoded_va;

#if (PAGE_SIZE < PAGE_FRAG_CACHE_MAX_SIZE) && (BITS_PER_LONG <= 32)
         __s16 remaining;
         __u16 pagecnt_bias;
#else
         __s32 remaining;
         __u32 pagecnt_bias;
#endif
};

void *__page_frag_alloc_va_align(struct page_frag_cache *nc,
                                  unsigned int fragsz, gfp_t gfp_mask,
                                  unsigned int align_mask)
{
         unsigned int size = page_frag_cache_page_size(nc->encoded_va);
         int remaining;

         remaining = __ALIGN_KERNEL_MASK(nc->remaining, ~align_mask);
         if (unlikely(remaining + (int)fragsz > 0)) {
                 if (!__page_frag_cache_refill(nc, gfp_mask))
                         return NULL;

                 size = page_frag_cache_page_size(nc->encoded_va);

                 remaining = -size;
                 if (unlikely(remaining + (int)fragsz > 0))
                         return NULL;
         }

         nc->pagecnt_bias--;
         nc->remaining = remaining + fragsz;

         return encoded_page_address(nc->encoded_va) + size + remaining;
}


And let's say what I am proposing in v10 is option 2 as below:
struct page_frag_cache {
         /* encoded_va consists of the virtual address, pfmemalloc bit 
and order
          * of a page.
          */
         unsigned long encoded_va;

#if (PAGE_SIZE < PAGE_FRAG_CACHE_MAX_SIZE) && (BITS_PER_LONG <= 32)
         __u16 remaining;
         __u16 pagecnt_bias;
#else
         __u32 remaining;
         __u32 pagecnt_bias;
#endif
};

void *__page_frag_alloc_va_align(struct page_frag_cache *nc,
                                  unsigned int fragsz, gfp_t gfp_mask,
                                  unsigned int align_mask)
{
         unsigned int size = page_frag_cache_page_size(nc->encoded_va);
         int aligned_remaining = nc->remaining & align_mask;
         int remaining = aligned_remaining - fragsz;

         if (unlikely(remaining < 0)) {
                 if (!__page_frag_cache_refill(nc, gfp_mask))
                         return NULL;

                 size = page_frag_cache_page_size(nc->encoded_va);

                 aligned_remaining = size;
                 remaining = aligned_remaining - fragsz;
                 if (unlikely(remaining < 0))
                         return NULL;
         }

         nc->pagecnt_bias--;
         nc->remaining = remaining;

         return encoded_page_address(nc->encoded_va) + (size - 
aligned_remaining);
}

If the option 1 is not what you have in mind, it would be better to be 
more specific about what you have in mind.

If the option 1 is what you have in mind, it seems both option 1 and
option 2 have the same semantics as my understanding, right? The 
question here seems to be what is your perfer option and why?

I implemented both of them, and the option 1 seems to have a
bigger generated asm size as below:
./scripts/bloat-o-meter vmlinux_non_neg vmlinux
add/remove: 0/0 grow/shrink: 1/0 up/down: 37/0 (37)
Function                                     old     new   delta
__page_frag_alloc_va_align                   414     451     +37

> 
> Basically the issue is your current code is using nc->remaining to
> generate the current address and that is bad as it isn't aligned to
> anything as fragsz was added to it and no alignment check had been
> done on that value.