On Mon, Jul 8, 2024 at 11:58 PM Yunsheng Lin [off-list ref] wrote:

On 2024/7/8 22:30, Alexander Duyck wrote:

quoted

On Mon, Jul 8, 2024 at 3:58 AM Yunsheng Lin [off-list ref] wrote:

quoted

On 2024/7/8 1:12, Alexander Duyck wrote:

...

quoted

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.

It seems the above didn't work either, as asm-offsets.c does depend on
mm_types_task.h too.

In file included from ./include/linux/mm.h:16,
                 from ./include/linux/page_frag_cache.h:10,
                 from ./include/linux/mm_types_task.h:11,
                 from ./include/linux/mm_types.h:5,
                 from ./include/linux/mmzone.h:22,
                 from ./include/linux/gfp.h:7,
                 from ./include/linux/slab.h:16,
                 from ./include/linux/resource_ext.h:11,
                 from ./include/linux/acpi.h:13,
                 from ./include/acpi/apei.h:9,
                 from ./include/acpi/ghes.h:5,
                 from ./include/linux/arm_sdei.h:8,
                 from arch/arm64/kernel/asm-offsets.c:10:
./include/linux/mmap_lock.h: In function ‘mmap_assert_locked’:
./include/linux/mmap_lock.h:65:23: error: invalid use of undefined type ‘const struct mm_struct’
   65 |  rwsem_assert_held(&mm->mmap_lock);

Do not include page_frag_cache.h in mm_types_task.h. Just move the
struct page_frag_cache there to replace struct page_frag.

The above does seem a clever idea, but doesn't doing above also seem to
defeat some purpose of patch 1? Anyway, it seems workable for trying
to avoid adding a new header for a single struct.

About the 'replace' part, as mentioned in [1], the 'struct page_frag'
is still needed as this patchset is large enough that replacing is only
done for sk_page_frag(), there are still other places using page_frag
that can be replaced by page_frag_cache in the following patchset.

1. https://lore.kernel.org/all/b200a609-2f30-ec37-39b6-f37ed8092f41@huawei.com/ (local)

The point is you need to avoid pulling mm.h into sched.h. To do that
you have to pull the data structure out and place it in a different
header file. So maybe instead of creating yet another header file you
can just place the structure in mm_types_task.h and once you have
dealt with all of the other users you can finally drop the page_frag
structure.