mempolicy.h: Remove unnecessary header file inclusions

[PATCH] mempolicy.h: Remove unnecessary header file inclusions

Posted by Junjie Fu 1 year ago

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>
 
 struct mm_struct;
-- 
2.34.1

Re: [PATCH] mempolicy.h: Remove unnecessary header file inclusions

Posted by SeongJae Park 1 year ago

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

[...]

Re: [PATCH] mempolicy.h: Remove unnecessary header file inclusions

Posted by Junjie Fu 1 year ago


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.

Re: [PATCH] mempolicy.h: Remove unnecessary header file inclusions

Posted by Matthew Wilcox 1 year ago

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.

Re: [PATCH] mempolicy.h: Remove unnecessary header file inclusions

Posted by Andrew Morton 1 year ago

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.

[PATCH] mm/mempolicy.c: include pagemap.h directly

Posted by Junjie Fu 1 year ago

Thanks for the review and suggestion. I agree that depending on indirect
includes isn't ideal. This patch followed your suggestion and now directly
includes pagemap.h in mempolicy.c

Signed-off-by: Junjie Fu <fujunjie1@qq.com>
---
 mm/mempolicy.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/mempolicy.c b/mm/mempolicy.c
index 88eef9776bb0..c3f25a7951e0 100644
--- a/mm/mempolicy.c
+++ b/mm/mempolicy.c
@@ -113,7 +113,7 @@
 #include <asm/tlbflush.h>
 #include <asm/tlb.h>
 #include <linux/uaccess.h>
-
+#include <linux/pagemap.h>
 #include "internal.h"
 
 /* Internal flags */
-- 
2.34.1

Re: [PATCH] mm/mempolicy.c: include pagemap.h directly

Posted by Matthew Wilcox 1 year ago

On Sat, Dec 07, 2024 at 04:14:30PM +0800, Junjie Fu wrote:
> @@ -113,7 +113,7 @@
>  #include <asm/tlbflush.h>
>  #include <asm/tlb.h>
>  #include <linux/uaccess.h>
> -
> +#include <linux/pagemap.h>
>  #include "internal.h"

This is in the wrong place.  The linux/ includes go before the asm/
includes.  I like to stick to alphabetical order within that divide,
but other people have other preferences.

[PATCH] mm/mempolicy.c: include pagemap.h directly in right place

Posted by Junjie Fu 1 year ago

Thank you for your reminder. I have changed the place where the header
file was imported again.

Signed-off-by: Junjie Fu <fujunjie1@qq.com>
---
 mm/mempolicy.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/mm/mempolicy.c b/mm/mempolicy.c
index 88eef9776bb0..9ee22263591d 100644
--- a/mm/mempolicy.c
+++ b/mm/mempolicy.c
@@ -109,6 +109,7 @@
 #include <linux/mmu_notifier.h>
 #include <linux/printk.h>
 #include <linux/swapops.h>
+#include <linux/pagemap.h>
 
 #include <asm/tlbflush.h>
 #include <asm/tlb.h>
-- 
2.34.1