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@xxxxxxxxx> wrote: >>> >>> On 6/30/2024 1:37 AM, Alexander Duyck wrote: >>>> On Sat, Jun 29, 2024 at 4:15 AM Yunsheng Lin <linyunsheng@xxxxxxxxxx> 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. > . >
