Message-ID: <CAKgT0UcpLBtkX9qrngJAtpnnxT-YRqLFc+J4oMMVnTCPG5sMug@mail.gmail.com>
Date: Sun, 7 Jul 2024 10:12:26 -0700
From: Alexander Duyck <alexander.duyck@...il.com>
To: Yunsheng Lin <linyunsheng@...wei.com>
Cc: Yunsheng Lin <yunshenglin0825@...il.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 10/13] mm: page_frag: introduce
 prepare/probe/commit API

On Wed, Jul 3, 2024 at 5:40 AM Yunsheng Lin <linyunsheng@...wei.com> wrote:
>
> 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@...il.com> wrote:
> >>>
> >>> On 6/30/2024 1:37 AM, Alexander Duyck wrote:
> >>>> On Sat, Jun 29, 2024 at 4:15 AM Yunsheng Lin <linyunsheng@...wei.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.
> > .

The issue is the dependency mess that has been created with patch 11
in the set. Again you are conflating patches which makes this really
hard to debug or discuss as I make suggestions on one patch and you
claim it breaks things that are really due to issues in another patch.
So the issue is you included this header into include/linux/sched.h
which is included in linux/mm_types.h. So what happens then is that
you have to include page_frag_cache.h *before* you can include the
bits from mm_types.h

What might make more sense to solve this is to look at just moving the
page_frag_cache into mm_types_task.h and then having it replace the
page_frag struct there since mm_types.h will pull that in anyway. That
way sched.h can avoid having to pull in page_frag_cache.h.

Powered by blists - more mailing lists