| Message ID | tencent_08B979048FE091821B290B18AE97E70DC507@qq.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mempolicy.h: Remove unnecessary header file inclusions | expand |
On Fri, 6 Dec 2024 23:53:49 +0800 Junjie Fu <fujunjie1@qq.com> wrote: > Originally, linux/mempolicy.h included linux/pagemap.h because vma_migratable() > was implemented inline within the header, requiring mapping_gfp_mask() > function to implement vma_migratable(). Now that vma_migratable() is only > declared in linux/mempolicy.h and its implementation has been moved to mempolicy.c, > the inclusion of linux/pagemap.h in the header is no longer necessary. > > Additionally, since mempolicy.c includes internal.h, and internal.h already > includes linux/pagemap.h, so there is no need to modify mempolicy.c after > removing the direct inclusion of linux/pagemap.h from linux/mempolicy.h If mempolicy.c uses things whcih are defined in pagemap.h then mempolicy.c should include pagemap.h directly, and not rely upon such nested includes. It's simpler, directer, expresses what's actually happening and avoids build breakage due to ongoing header untanglings.
Hi Junie, On Fri, 6 Dec 2024 23:53:49 +0800 Junjie Fu <fujunjie1@qq.com> wrote: > Originally, linux/mempolicy.h included linux/pagemap.h because vma_migratable() > was implemented inline within the header, requiring mapping_gfp_mask() > function to implement vma_migratable(). Now that vma_migratable() is only > declared in linux/mempolicy.h and its implementation has been moved to mempolicy.c, > the inclusion of linux/pagemap.h in the header is no longer necessary. > > Additionally, since mempolicy.c includes internal.h, and internal.h already > includes linux/pagemap.h, so there is no need to modify mempolicy.c after > removing the direct inclusion of linux/pagemap.h from linux/mempolicy.h > > Signed-off-by: Junjie Fu <fujunjie1@qq.com> > --- > include/linux/mempolicy.h | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/include/linux/mempolicy.h b/include/linux/mempolicy.h > index ce9885e0178a..d36877557b00 100644 > --- a/include/linux/mempolicy.h > +++ b/include/linux/mempolicy.h > @@ -12,7 +12,6 @@ > #include <linux/rbtree.h> > #include <linux/spinlock.h> > #include <linux/nodemask.h> > -#include <linux/pagemap.h> > #include <uapi/linux/mempolicy.h> I noticed kunit UM build errors as below on mm-unstable, and git bisect points this patch. $ ./tools/testing/kunit/kunit.py run --kunitconfig ./mm/damon/tests/ [...] fs/aio.c:525:71: error: ‘FGP_CREAT’ undeclared (first use in this function); did you mean ‘IPC_CREAT’? 525 | FGP_LOCK | FGP_ACCESSED | FGP_CREAT, | ^~~~~~~~~ | IPC_CREAT fs/aio.c:532:17: error: implicit declaration of function ‘folio_end_read’; did you mean ‘folio_test_head’? [-Werror=implicit-function-declaration] 532 | folio_end_read(folio, true); | ^~~~~~~~~~~~~~ | folio_test_head [...] I also confirmed including pagemap.h on fs/aio.c as below fixes the issue. I would like to hear you or others opinions though, since I'm not familiar with the inclusion routes of the file. diff --git a/fs/aio.c b/fs/aio.c index 50671640b588..9fad51dc823f 100644 --- a/fs/aio.c +++ b/fs/aio.c @@ -39,6 +39,7 @@ #include <linux/compat.h> #include <linux/migrate.h> #include <linux/ramfs.h> +#include <linux/pagemap.h> #include <linux/percpu-refcount.h> #include <linux/mount.h> #include <linux/pseudo_fs.h> Thanks, SJ [...]
On Sat, Dec 07, 2024 at 11:53:41AM -0800, SeongJae Park wrote: > I noticed kunit UM build errors as below on mm-unstable, and git bisect points > this patch. this is correct, and should be squashed into the previous patch as a fixup.
On December 8, 2024 at 3:53, SeongJae Park wrote: > I noticed kunit UM build errors as below on mm-unstable, and git bisect points > this patch. > > $ ./tools/testing/kunit/kunit.py run --kunitconfig ./mm/damon/tests/ > [...] > fs/aio.c:525:71: error: ‘FGP_CREAT’ undeclared (first use in this function); did you mean ‘IPC_CREAT’? > 525 | FGP_LOCK | FGP_ACCESSED | FGP_CREAT, > | ^~~~~~~~~ > | IPC_CREAT > fs/aio.c:532:17: error: implicit declaration of function ‘folio_end_read’; did you mean ‘folio_test_head’? [-Werror=implicit-function-declaration] > 532 | folio_end_read(folio, true); > | ^~~~~~~~~~~~~~ > | folio_test_head > [...] > > I also confirmed including pagemap.h on fs/aio.c as below fixes the issue. I > would like to hear you or others opinions though, since I'm not familiar with > the inclusion routes of the file. Including unnecessary header files in a .h file is not a good practice. It can lead to troublesome dependency issues in the future. However, based on the testing on your side, this change might cause some other compilation issues, as indicated in your patch. I think all these issues should be resolvable by including the pagemap.h in the .c files instead.
diff --git a/include/linux/mempolicy.h b/include/linux/mempolicy.h index ce9885e0178a..d36877557b00 100644 --- a/include/linux/mempolicy.h +++ b/include/linux/mempolicy.h @@ -12,7 +12,6 @@ #include <linux/rbtree.h> #include <linux/spinlock.h> #include <linux/nodemask.h> -#include <linux/pagemap.h> #include <uapi/linux/mempolicy.h> struct mm_struct;
Originally, linux/mempolicy.h included linux/pagemap.h because vma_migratable() was implemented inline within the header, requiring mapping_gfp_mask() function to implement vma_migratable(). Now that vma_migratable() is only declared in linux/mempolicy.h and its implementation has been moved to mempolicy.c, the inclusion of linux/pagemap.h in the header is no longer necessary. Additionally, since mempolicy.c includes internal.h, and internal.h already includes linux/pagemap.h, so there is no need to modify mempolicy.c after removing the direct inclusion of linux/pagemap.h from linux/mempolicy.h Signed-off-by: Junjie Fu <fujunjie1@qq.com> --- include/linux/mempolicy.h | 1 - 1 file changed, 1 deletion(-)