netdev - Re: [RFC v11 02/14] mm: move the page fragment allocator from page

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: <ed24943d-5c3e-4f60-9e53-3c294c4237b5@gmail.com>
Date: Sat, 27 Jul 2024 23:04:55 +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,
 David Howells <dhowells@...hat.com>,
 Andrew Morton <akpm@...ux-foundation.org>, linux-mm@...ck.org
Subject: Re: [RFC v11 02/14] mm: move the page fragment allocator from
 page_alloc into its own file

On 7/22/2024 1:58 AM, Alexander Duyck wrote:
> On Fri, Jul 19, 2024 at 2:37 AM Yunsheng Lin <linyunsheng@...wei.com> wrote:

...

>> --- /dev/null
>> +++ b/include/linux/page_frag_cache.h
>> @@ -0,0 +1,32 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +
>> +#ifndef _LINUX_PAGE_FRAG_CACHE_H
>> +#define _LINUX_PAGE_FRAG_CACHE_H
>> +
>> +#include <linux/log2.h>
>> +#include <linux/types.h>
>> +#include <linux/mm_types_task.h>
> 
> You don't need to include mm_types_task.h here. You can just use
> declare "struct page_frag_cache;" as we did before in gfp.h.
> Technically this should be included in mm_types.h so any callers
> making use of these functions would need to make sure to include that
> like we did for gfp.h before anyway.

The probe API is added as an inline helper in patch 11 according to
discussion in [1], so the definition of "struct page_frag_cache" is
needed, so I am not sure what is the point of using
"struct page_frag_cache;" here and then remove it and include
mm_types_task.h in patch 11.

1. 
https://lore.kernel.org/all/cb541985-a06d-7a71-9e6d-38827ccdf875@huawei.com/

> 
>> +#include <asm/page.h>
>> +
> 
> Not sure why this is included here either. From what I can tell there
> isn't anything here using the contents of page.h. I suspect you should
> only need it for the get_order call which would be used in other
> files.

It seems unnecessay, will remove that.

> 
>> +void page_frag_cache_drain(struct page_frag_cache *nc);
>> +void __page_frag_cache_drain(struct page *page, unsigned int count);
>> +void *__page_frag_alloc_align(struct page_frag_cache *nc, unsigned int fragsz,
>> +                             gfp_t gfp_mask, unsigned int align_mask);
>> +
>> +static inline void *page_frag_alloc_align(struct page_frag_cache *nc,
>> +                                         unsigned int fragsz, gfp_t gfp_mask,
>> +                                         unsigned int align)
>> +{
>> +       WARN_ON_ONCE(!is_power_of_2(align));
>> +       return __page_frag_alloc_align(nc, fragsz, gfp_mask, -align);
>> +}
>> +
>> +static inline void *page_frag_alloc(struct page_frag_cache *nc,
>> +                                   unsigned int fragsz, gfp_t gfp_mask)
>> +{
>> +       return __page_frag_alloc_align(nc, fragsz, gfp_mask, ~0u);
>> +}
>> +
>> +void page_frag_free(void *addr);
>> +
>> +#endif
> 
> ...
> 
>> diff --git a/mm/page_frag_test.c b/mm/page_frag_test.c
>> index cf2691f60b67..b7a5affb92f2 100644
>> --- a/mm/page_frag_test.c
>> +++ b/mm/page_frag_test.c
>> @@ -6,7 +6,6 @@
>>    * Copyright: linyunsheng@...wei.com
>>    */
>>
>> -#include <linux/mm.h>
>>   #include <linux/module.h>
>>   #include <linux/slab.h>
>>   #include <linux/vmalloc.h>
>> @@ -16,6 +15,7 @@
>>   #include <linux/log2.h>
>>   #include <linux/completion.h>
>>   #include <linux/kthread.h>
>> +#include <linux/page_frag_cache.h>
>>
>>   #define OBJPOOL_NR_OBJECT_MAX  BIT(24)
> 
> Rather than making users have to include page_frag_cache.h I think it
> would be better for us to just maintain the code as being accessible
> from mm.h. So it might be better to just add page_frag_cache.h to the
> includes there.

It would be better to list out why it is better that way as I am failing
to see it that way yet as I think it is better to use the explicit
header file instead the implicit header file.


>