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

[Yunsheng Lin <yunshenglin0825@gmail.com>]

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.

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.