Re: [PATCH net-next v9 10/13] mm: page_frag: introduce prepare/probe/commit API

[Yunsheng Lin <linyunsheng@huawei.com>]

On 2024/6/30 23:05, Yunsheng Lin wrote:
> On 6/30/2024 10:35 PM, Alexander Duyck wrote:
>> On Sun, Jun 30, 2024 at 7:05 AM Yunsheng Lin <yunshenglin0825@gmail.com> wrote:
>>>
>>> On 6/30/2024 1:37 AM, Alexander Duyck wrote:
>>>> On Sat, Jun 29, 2024 at 4:15 AM Yunsheng Lin <linyunsheng@huawei.com> wrote:
>>>
>>> ...
>>>
>>>>>>
>>>>>> Why is this a macro instead of just being an inline? Are you trying to
>>>>>> avoid having to include a header due to the virt_to_page?
>>>>>
>>>>> Yes, you are right.
>>
>> ...
>>
>>>> I am pretty sure you just need to add:
>>>> #include <asm/page.h>
>>>
>>> I am supposing you mean adding the above to page_frag_cache.h, right?
>>>
>>> It seems thing is more complicated for SPARSEMEM_VMEMMAP case, as it
>>> needs the declaration of 'vmemmap'(some arch defines it as a pointer
>>> variable while some arch defines it as a macro) and the definition of
>>> 'struct page' for '(vmemmap + (pfn))' operation.
>>>
>>> Adding below for 'vmemmap' and 'struct page' seems to have some compiler
>>> error caused by interdependence between linux/mm_types.h and asm/pgtable.h:
>>> #include <asm/pgtable.h>
>>> #include <linux/mm_types.h>
>>>
>>
>> Maybe you should just include linux/mm.h as that should have all the
>> necessary includes to handle these cases. In any case though it
> 
> Including linux/mm.h seems to have similar compiler error, just the
> interdependence is between linux/mm_types.h and linux/mm.h now.

How about splitting page_frag_cache.h into page_frag_types.h and
page_frag_cache.h mirroring the above linux/mm_types.h and linux/mm.h
to fix the compiler error?

> 
> As below, linux/mmap_lock.h obviously need the definition of
> 'struct mm_struct' from linux/mm_types.h, and linux/mm_types.h
> has some a long dependency of linux/mm.h starting from
> linux/uprobes.h if we add '#include <linux/mm.h>' in linux/page_frag_cache.h:
> 
> In file included from ./include/linux/mm.h:16,
>                  from ./include/linux/page_frag_cache.h:6,
>                  from ./include/linux/sched.h:49,
>                  from ./include/linux/percpu.h:13,
>                  from ./arch/x86/include/asm/msr.h:15,
>                  from ./arch/x86/include/asm/tsc.h:10,
>                  from ./arch/x86/include/asm/timex.h:6,
>                  from ./include/linux/timex.h:67,
>                  from ./include/linux/time32.h:13,
>                  from ./include/linux/time.h:60,
>                  from ./include/linux/jiffies.h:10,
>                  from ./include/linux/ktime.h:25,
>                  from ./include/linux/timer.h:6,
>                  from ./include/linux/workqueue.h:9,
>                  from ./include/linux/srcu.h:21,
>                  from ./include/linux/notifier.h:16,
>                  from ./arch/x86/include/asm/uprobes.h:13,
>                  from ./include/linux/uprobes.h:49,
>                  from ./include/linux/mm_types.h:16,
>                  from ./include/linux/mmzone.h:22,
>                  from ./include/linux/gfp.h:7,
>                  from ./include/linux/slab.h:16,
>                  from ./include/linux/crypto.h:17,
>                  from arch/x86/kernel/asm-offsets.c:9:
> ./include/linux/mmap_lock.h: In function ‘mmap_assert_locked’:
> ./include/linux/mmap_lock.h:65:30: error: invalid use of undefined type ‘const struct mm_struct’
>    65 |         rwsem_assert_held(&mm->mmap_lock);
>       |                              ^~
> 
>> doesn't make any sense to have a define in one include that expects
>> the user to then figure out what other headers to include in order to
>> make the define work they should be included in the header itself to
>> avoid any sort of weird dependencies.
> 
> Perhaps there are some season why there are two headers for the mm subsystem, linux/mm_types.h and linux/mm.h?
> And .h file is supposed to include the linux/mm_types.h while .c file
> is supposed to include the linux/mm.h?
> If the above is correct, it seems the above rule is broked by including linux/mm.h in linux/page_frag_cache.h.
> .
>