All of lore.kernel.org
 help / color / mirror / Atom feed
From: Dennis Zhou <dennis@kernel.org>
To: Tejun Heo <tj@kernel.org>, Christoph Lameter <cl@linux.com>
Cc: linux-mm@kvack.org, linux-kernel@vger.kernel.org,
	clang-built-linux@googlegroups.com, kbuild-all@lists.01.org,
	Dennis Zhou <dennis@kernel.org>,
	kernel test robot <lkp@intel.com>
Subject: [PATCH] percpu: fix clang modpost warning in pcpu_build_alloc_info()
Date: Thu, 31 Dec 2020 21:28:52 +0000	[thread overview]
Message-ID: <20201231212852.3175381-1-dennis@kernel.org> (raw)

This is an unusual situation so I thought it best to explain it in a
separate patch.

"percpu: reduce the number of cpu distance comparisons" introduces a
dependency on cpumask helper functions in __init code. This code
references a struct cpumask annotated __initdata. When the function is
inlined (gcc), everything is fine, but clang decides not to inline these
function calls. This causes modpost to warn about an __initdata access
by a function not annotated with __init [1].

Ways I thought about fixing it:
1. figure out why clang thinks this inlining is too costly.
2. create a wrapper function annotated __init (this).
3. annotate cpumask with __refdata.

Ultimately it comes down to if it's worth saving the cpumask memory and
allowing it to be freed. IIUC, __refdata won't be freed, so option 3 is
just a little wasteful. 1 is out of my depth, leaving 2. I don't feel
great about this behavior being dependent on inlining semantics, but
cpumask helpers are small and probably should be inlined.

modpost complaint:
  WARNING: modpost: vmlinux.o(.text+0x735425): Section mismatch in reference from the function cpumask_clear_cpu() to the variable .init.data:pcpu_build_alloc_info.mask
  The function cpumask_clear_cpu() references
  the variable __initdata pcpu_build_alloc_info.mask.
  This is often because cpumask_clear_cpu lacks a __initdata
  annotation or the annotation of pcpu_build_alloc_info.mask is wrong.

clang output:
  mm/percpu.c:2724:5: remark: cpumask_clear_cpu not inlined into pcpu_build_alloc_info because too costly to inline (cost=725, threshold=325) [-Rpass-missed=inline]

[1] https://lore.kernel.org/linux-mm/202012220454.9F6Bkz9q-lkp@intel.com/

Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: Dennis Zhou <dennis@kernel.org>
---
This is on top of percpu#for-5.12.

 mm/percpu.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/mm/percpu.c b/mm/percpu.c
index 80f8f885a990..357977c4cb00 100644
--- a/mm/percpu.c
+++ b/mm/percpu.c
@@ -2642,6 +2642,18 @@ early_param("percpu_alloc", percpu_alloc_setup);
 
 /* pcpu_build_alloc_info() is used by both embed and page first chunk */
 #if defined(BUILD_EMBED_FIRST_CHUNK) || defined(BUILD_PAGE_FIRST_CHUNK)
+
+/*
+ * This wrapper is to avoid a warning where cpumask_clear_cpu() is not inlined
+ * when compiling with clang causing modpost to warn about accessing __initdata
+ * from a non __init function.  By doing this, we allow the struct cpumask to be
+ * freed instead of it taking space by annotating with __refdata.
+ */
+static void __init pcpu_cpumask_clear_cpu(int cpu, struct cpumask *mask)
+{
+	cpumask_clear_cpu(cpu, mask);
+}
+
 /**
  * pcpu_build_alloc_info - build alloc_info considering distances between CPUs
  * @reserved_size: the size of reserved percpu area in bytes
@@ -2713,7 +2725,7 @@ static struct pcpu_alloc_info * __init pcpu_build_alloc_info(
 		cpu = cpumask_first(&mask);
 		group_map[cpu] = group;
 		group_cnt[group]++;
-		cpumask_clear_cpu(cpu, &mask);
+		pcpu_cpumask_clear_cpu(cpu, &mask);
 
 		for_each_cpu(tcpu, &mask) {
 			if (!cpu_distance_fn ||
@@ -2721,7 +2733,7 @@ static struct pcpu_alloc_info * __init pcpu_build_alloc_info(
 			     cpu_distance_fn(tcpu, cpu) == LOCAL_DISTANCE)) {
 				group_map[tcpu] = group;
 				group_cnt[group]++;
-				cpumask_clear_cpu(tcpu, &mask);
+				pcpu_cpumask_clear_cpu(tcpu, &mask);
 			}
 		}
 	}
-- 
2.29.2.729.g45daf8777d-goog


WARNING: multiple messages have this Message-ID (diff)
From: Dennis Zhou <dennis@kernel.org>
To: kbuild-all@lists.01.org
Subject: [PATCH] percpu: fix clang modpost warning in pcpu_build_alloc_info()
Date: Thu, 31 Dec 2020 21:28:52 +0000	[thread overview]
Message-ID: <20201231212852.3175381-1-dennis@kernel.org> (raw)

[-- Attachment #1: Type: text/plain, Size: 3528 bytes --]

This is an unusual situation so I thought it best to explain it in a
separate patch.

"percpu: reduce the number of cpu distance comparisons" introduces a
dependency on cpumask helper functions in __init code. This code
references a struct cpumask annotated __initdata. When the function is
inlined (gcc), everything is fine, but clang decides not to inline these
function calls. This causes modpost to warn about an __initdata access
by a function not annotated with __init [1].

Ways I thought about fixing it:
1. figure out why clang thinks this inlining is too costly.
2. create a wrapper function annotated __init (this).
3. annotate cpumask with __refdata.

Ultimately it comes down to if it's worth saving the cpumask memory and
allowing it to be freed. IIUC, __refdata won't be freed, so option 3 is
just a little wasteful. 1 is out of my depth, leaving 2. I don't feel
great about this behavior being dependent on inlining semantics, but
cpumask helpers are small and probably should be inlined.

modpost complaint:
  WARNING: modpost: vmlinux.o(.text+0x735425): Section mismatch in reference from the function cpumask_clear_cpu() to the variable .init.data:pcpu_build_alloc_info.mask
  The function cpumask_clear_cpu() references
  the variable __initdata pcpu_build_alloc_info.mask.
  This is often because cpumask_clear_cpu lacks a __initdata
  annotation or the annotation of pcpu_build_alloc_info.mask is wrong.

clang output:
  mm/percpu.c:2724:5: remark: cpumask_clear_cpu not inlined into pcpu_build_alloc_info because too costly to inline (cost=725, threshold=325) [-Rpass-missed=inline]

[1] https://lore.kernel.org/linux-mm/202012220454.9F6Bkz9q-lkp(a)intel.com/

Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: Dennis Zhou <dennis@kernel.org>
---
This is on top of percpu#for-5.12.

 mm/percpu.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/mm/percpu.c b/mm/percpu.c
index 80f8f885a990..357977c4cb00 100644
--- a/mm/percpu.c
+++ b/mm/percpu.c
@@ -2642,6 +2642,18 @@ early_param("percpu_alloc", percpu_alloc_setup);
 
 /* pcpu_build_alloc_info() is used by both embed and page first chunk */
 #if defined(BUILD_EMBED_FIRST_CHUNK) || defined(BUILD_PAGE_FIRST_CHUNK)
+
+/*
+ * This wrapper is to avoid a warning where cpumask_clear_cpu() is not inlined
+ * when compiling with clang causing modpost to warn about accessing __initdata
+ * from a non __init function.  By doing this, we allow the struct cpumask to be
+ * freed instead of it taking space by annotating with __refdata.
+ */
+static void __init pcpu_cpumask_clear_cpu(int cpu, struct cpumask *mask)
+{
+	cpumask_clear_cpu(cpu, mask);
+}
+
 /**
  * pcpu_build_alloc_info - build alloc_info considering distances between CPUs
  * @reserved_size: the size of reserved percpu area in bytes
@@ -2713,7 +2725,7 @@ static struct pcpu_alloc_info * __init pcpu_build_alloc_info(
 		cpu = cpumask_first(&mask);
 		group_map[cpu] = group;
 		group_cnt[group]++;
-		cpumask_clear_cpu(cpu, &mask);
+		pcpu_cpumask_clear_cpu(cpu, &mask);
 
 		for_each_cpu(tcpu, &mask) {
 			if (!cpu_distance_fn ||
@@ -2721,7 +2733,7 @@ static struct pcpu_alloc_info * __init pcpu_build_alloc_info(
 			     cpu_distance_fn(tcpu, cpu) == LOCAL_DISTANCE)) {
 				group_map[tcpu] = group;
 				group_cnt[group]++;
-				cpumask_clear_cpu(tcpu, &mask);
+				pcpu_cpumask_clear_cpu(tcpu, &mask);
 			}
 		}
 	}
-- 
2.29.2.729.g45daf8777d-goog

             reply	other threads:[~2020-12-31 21:47 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-12-31 21:28 Dennis Zhou [this message]
2020-12-31 21:28 ` [PATCH] percpu: fix clang modpost warning in pcpu_build_alloc_info() Dennis Zhou
2021-01-04 23:46 ` Nathan Chancellor
2021-01-05  0:55   ` Dennis Zhou
2021-01-05  0:55     ` Dennis Zhou
2021-01-25 11:07     ` Arnd Bergmann
2021-01-25 11:07       ` Arnd Bergmann
2021-01-25 11:07       ` Arnd Bergmann
2021-01-25 18:27       ` Nick Desaulniers
2021-01-25 18:27         ` Nick Desaulniers
2021-01-25 18:27         ` Nick Desaulniers
2021-01-26  5:11         ` Dennis Zhou
2021-01-26  5:11           ` Dennis Zhou
2021-01-26  5:04       ` Dennis Zhou
2021-01-26  5:04         ` Dennis Zhou

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20201231212852.3175381-1-dennis@kernel.org \
    --to=dennis@kernel.org \
    --cc=cl@linux.com \
    --cc=clang-built-linux@googlegroups.com \
    --cc=kbuild-all@lists.01.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=lkp@intel.com \
    --cc=tj@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.