* [PATCH v2] sched/sysctl: Fix attributes of some extern declarations
@ 2017-10-30 18:08 Matthias Kaehlcke
2017-10-31 9:57 ` Ingo Molnar
2017-11-01 8:45 ` [tip:sched/core] " tip-bot for Matthias Kaehlcke
0 siblings, 2 replies; 6+ messages in thread
From: Matthias Kaehlcke @ 2017-10-30 18:08 UTC (permalink / raw)
To: Ingo Molnar, Shile Zhang, Peter Zijlstra
Cc: linux-kernel, Nick Desaulniers, Douglas Anderson, Guenter Roeck,
Matthias Kaehlcke
The definition of sysctl_sched_migration_cost, sysctl_sched_nr_migrate
and sysctl_sched_time_avg includes the attribute const_debug. This
attribute is not part of the extern declaration of these variables in
include/linux/sched/sysctl.h, as a result clang generates warnings like
this:
kernel/sched/sched.h:1618:33: warning: section attribute is specified on
redeclared variable [-Wsection]
extern const_debug unsigned int sysctl_sched_time_avg;
^
./include/linux/sched/sysctl.h:42:21: note: previous declaration is here
extern unsigned int sysctl_sched_time_avg;
The header only declares the variables when CONFIG_SCHED_DEBUG is defined,
therefore it is not necessary to duplicate the definition of const_debug.
Instead we can use the attribute __read_mostly, which is the expansion of
const_debug when CONFIG_SCHED_DEBUG is set.
Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
Reviewed-by: Nick Desaulniers <nick.desaulniers@gmail.com>
---
Changes in v2:
- removed pointless include of linux/static_key.h
- added Reviewed-by tag
include/linux/sched/sysctl.h | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/include/linux/sched/sysctl.h b/include/linux/sched/sysctl.h
index 0f5ecd4d298e..d34c823f3d36 100644
--- a/include/linux/sched/sysctl.h
+++ b/include/linux/sched/sysctl.h
@@ -37,9 +37,9 @@ extern unsigned int sysctl_numa_balancing_scan_period_max;
extern unsigned int sysctl_numa_balancing_scan_size;
#ifdef CONFIG_SCHED_DEBUG
-extern unsigned int sysctl_sched_migration_cost;
-extern unsigned int sysctl_sched_nr_migrate;
-extern unsigned int sysctl_sched_time_avg;
+extern __read_mostly unsigned int sysctl_sched_migration_cost;
+extern __read_mostly unsigned int sysctl_sched_nr_migrate;
+extern __read_mostly unsigned int sysctl_sched_time_avg;
int sched_proc_update_handler(struct ctl_table *table, int write,
void __user *buffer, size_t *length,
--
2.15.0.rc2.357.g7e34df9404-goog
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH v2] sched/sysctl: Fix attributes of some extern declarations
2017-10-30 18:08 [PATCH v2] sched/sysctl: Fix attributes of some extern declarations Matthias Kaehlcke
@ 2017-10-31 9:57 ` Ingo Molnar
2017-10-31 22:31 ` Matthias Kaehlcke
2017-11-01 8:45 ` [tip:sched/core] " tip-bot for Matthias Kaehlcke
1 sibling, 1 reply; 6+ messages in thread
From: Ingo Molnar @ 2017-10-31 9:57 UTC (permalink / raw)
To: Matthias Kaehlcke
Cc: Shile Zhang, Peter Zijlstra, linux-kernel, Nick Desaulniers,
Douglas Anderson, Guenter Roeck
* Matthias Kaehlcke <mka@chromium.org> wrote:
> The definition of sysctl_sched_migration_cost, sysctl_sched_nr_migrate
> and sysctl_sched_time_avg includes the attribute const_debug. This
> attribute is not part of the extern declaration of these variables in
> include/linux/sched/sysctl.h, as a result clang generates warnings like
> this:
>
> kernel/sched/sched.h:1618:33: warning: section attribute is specified on
> redeclared variable [-Wsection]
> extern const_debug unsigned int sysctl_sched_time_avg;
> ^
> ./include/linux/sched/sysctl.h:42:21: note: previous declaration is here
> extern unsigned int sysctl_sched_time_avg;
>
> The header only declares the variables when CONFIG_SCHED_DEBUG is defined,
> therefore it is not necessary to duplicate the definition of const_debug.
> Instead we can use the attribute __read_mostly, which is the expansion of
> const_debug when CONFIG_SCHED_DEBUG is set.
>
> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> Reviewed-by: Nick Desaulniers <nick.desaulniers@gmail.com>
> ---
> Changes in v2:
> - removed pointless include of linux/static_key.h
> - added Reviewed-by tag
>
> include/linux/sched/sysctl.h | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/include/linux/sched/sysctl.h b/include/linux/sched/sysctl.h
> index 0f5ecd4d298e..d34c823f3d36 100644
> --- a/include/linux/sched/sysctl.h
> +++ b/include/linux/sched/sysctl.h
> @@ -37,9 +37,9 @@ extern unsigned int sysctl_numa_balancing_scan_period_max;
> extern unsigned int sysctl_numa_balancing_scan_size;
>
> #ifdef CONFIG_SCHED_DEBUG
> -extern unsigned int sysctl_sched_migration_cost;
> -extern unsigned int sysctl_sched_nr_migrate;
> -extern unsigned int sysctl_sched_time_avg;
> +extern __read_mostly unsigned int sysctl_sched_migration_cost;
> +extern __read_mostly unsigned int sysctl_sched_nr_migrate;
> +extern __read_mostly unsigned int sysctl_sched_time_avg;
So I hate this change, because it pointlessly duplicates an attribute that should
only matter at the definition site. The Clang warning:
> kernel/sched/sched.h:1618:33: warning: section attribute is specified on
> redeclared variable [-Wsection]
suggests that the -Wsection warning can be turned off. The Clang build should
probably do that.
Thanks,
Ingo
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] sched/sysctl: Fix attributes of some extern declarations
2017-10-31 9:57 ` Ingo Molnar
@ 2017-10-31 22:31 ` Matthias Kaehlcke
2017-11-01 4:33 ` Nick Desaulniers
2017-11-01 8:34 ` Ingo Molnar
0 siblings, 2 replies; 6+ messages in thread
From: Matthias Kaehlcke @ 2017-10-31 22:31 UTC (permalink / raw)
To: Ingo Molnar, Masahiro Yamada
Cc: Shile Zhang, Peter Zijlstra, linux-kernel, Nick Desaulniers,
Douglas Anderson, Guenter Roeck
El Tue, Oct 30, 2017 at 10:57:58AM +0100 Ingo Molnar ha dit:
> * Matthias Kaehlcke <mka@chromium.org> wrote:
>
> > The definition of sysctl_sched_migration_cost, sysctl_sched_nr_migrate
> > and sysctl_sched_time_avg includes the attribute const_debug. This
> > attribute is not part of the extern declaration of these variables in
> > include/linux/sched/sysctl.h, as a result clang generates warnings like
> > this:
> >
> > kernel/sched/sched.h:1618:33: warning: section attribute is specified on
> > redeclared variable [-Wsection]
> > extern const_debug unsigned int sysctl_sched_time_avg;
> > ^
> > ./include/linux/sched/sysctl.h:42:21: note: previous declaration is here
> > extern unsigned int sysctl_sched_time_avg;
> >
> > The header only declares the variables when CONFIG_SCHED_DEBUG is defined,
> > therefore it is not necessary to duplicate the definition of const_debug.
> > Instead we can use the attribute __read_mostly, which is the expansion of
> > const_debug when CONFIG_SCHED_DEBUG is set.
> >
> > Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> > Reviewed-by: Nick Desaulniers <nick.desaulniers@gmail.com>
> > ---
> > Changes in v2:
> > - removed pointless include of linux/static_key.h
> > - added Reviewed-by tag
> >
> > include/linux/sched/sysctl.h | 6 +++---
> > 1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/include/linux/sched/sysctl.h b/include/linux/sched/sysctl.h
> > index 0f5ecd4d298e..d34c823f3d36 100644
> > --- a/include/linux/sched/sysctl.h
> > +++ b/include/linux/sched/sysctl.h
> > @@ -37,9 +37,9 @@ extern unsigned int sysctl_numa_balancing_scan_period_max;
> > extern unsigned int sysctl_numa_balancing_scan_size;
> >
> > #ifdef CONFIG_SCHED_DEBUG
> > -extern unsigned int sysctl_sched_migration_cost;
> > -extern unsigned int sysctl_sched_nr_migrate;
> > -extern unsigned int sysctl_sched_time_avg;
> > +extern __read_mostly unsigned int sysctl_sched_migration_cost;
> > +extern __read_mostly unsigned int sysctl_sched_nr_migrate;
> > +extern __read_mostly unsigned int sysctl_sched_time_avg;
>
> So I hate this change, because it pointlessly duplicates an attribute that should
> only matter at the definition site.
It's certainly not ideal, and then again essentially the same is done
in kernel/sched/sched.h, just that here the specific attribute is
hidden behind const_debug.
> The Clang warning:
>
> > kernel/sched/sched.h:1618:33: warning: section attribute is specified on
> > redeclared variable [-Wsection]
>
> suggests that the -Wsection warning can be turned off. The Clang build should
> probably do that.
That can definitely be done. I don't have a really strong opinion on
this, just wonder if the warning could be useful in other circumstances
to spot conflicting declarations. This instance looks like a somewhat
special case with the declarations in two header files and it might be
worth to pay the price of the pointlessly duplicate attribute to get a
useful warning in others.
Adding kbuild maintainer Masahiro Yamada to the thread, who tends to
be reluctant about disabling warnings globally.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] sched/sysctl: Fix attributes of some extern declarations
2017-10-31 22:31 ` Matthias Kaehlcke
@ 2017-11-01 4:33 ` Nick Desaulniers
2017-11-01 8:34 ` Ingo Molnar
1 sibling, 0 replies; 6+ messages in thread
From: Nick Desaulniers @ 2017-11-01 4:33 UTC (permalink / raw)
To: Matthias Kaehlcke
Cc: Ingo Molnar, Masahiro Yamada, Shile Zhang, Peter Zijlstra,
Linux Kernel Mailing List, Douglas Anderson, Guenter Roeck
> El Tue, Oct 30, 2017 at 10:57:58AM +0100 Ingo Molnar ha dit:
>> So I hate this change, because it pointlessly duplicates an attribute that should
>> only matter at the definition site.
>
> It's certainly not ideal, and then again essentially the same is done
> in kernel/sched/sched.h, just that here the specific attribute is
> hidden behind const_debug.
>
>> The Clang warning:
>>
>> > kernel/sched/sched.h:1618:33: warning: section attribute is specified on
>> > redeclared variable [-Wsection]
>>
>> suggests that the -Wsection warning can be turned off. The Clang build should
>> probably do that.
Naive question: can these definitions be hoisted to include/linux/sched.h?
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] sched/sysctl: Fix attributes of some extern declarations
2017-10-31 22:31 ` Matthias Kaehlcke
2017-11-01 4:33 ` Nick Desaulniers
@ 2017-11-01 8:34 ` Ingo Molnar
1 sibling, 0 replies; 6+ messages in thread
From: Ingo Molnar @ 2017-11-01 8:34 UTC (permalink / raw)
To: Matthias Kaehlcke
Cc: Masahiro Yamada, Shile Zhang, Peter Zijlstra, linux-kernel,
Nick Desaulniers, Douglas Anderson, Guenter Roeck
* Matthias Kaehlcke <mka@chromium.org> wrote:
> El Tue, Oct 30, 2017 at 10:57:58AM +0100 Ingo Molnar ha dit:
>
> > * Matthias Kaehlcke <mka@chromium.org> wrote:
> >
> > > The definition of sysctl_sched_migration_cost, sysctl_sched_nr_migrate
> > > and sysctl_sched_time_avg includes the attribute const_debug. This
> > > attribute is not part of the extern declaration of these variables in
> > > include/linux/sched/sysctl.h, as a result clang generates warnings like
> > > this:
> > >
> > > kernel/sched/sched.h:1618:33: warning: section attribute is specified on
> > > redeclared variable [-Wsection]
> > > extern const_debug unsigned int sysctl_sched_time_avg;
> > > ^
> > > ./include/linux/sched/sysctl.h:42:21: note: previous declaration is here
> > > extern unsigned int sysctl_sched_time_avg;
> > >
> > > The header only declares the variables when CONFIG_SCHED_DEBUG is defined,
> > > therefore it is not necessary to duplicate the definition of const_debug.
> > > Instead we can use the attribute __read_mostly, which is the expansion of
> > > const_debug when CONFIG_SCHED_DEBUG is set.
> > >
> > > Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> > > Reviewed-by: Nick Desaulniers <nick.desaulniers@gmail.com>
> > > ---
> > > Changes in v2:
> > > - removed pointless include of linux/static_key.h
> > > - added Reviewed-by tag
> > >
> > > include/linux/sched/sysctl.h | 6 +++---
> > > 1 file changed, 3 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/include/linux/sched/sysctl.h b/include/linux/sched/sysctl.h
> > > index 0f5ecd4d298e..d34c823f3d36 100644
> > > --- a/include/linux/sched/sysctl.h
> > > +++ b/include/linux/sched/sysctl.h
> > > @@ -37,9 +37,9 @@ extern unsigned int sysctl_numa_balancing_scan_period_max;
> > > extern unsigned int sysctl_numa_balancing_scan_size;
> > >
> > > #ifdef CONFIG_SCHED_DEBUG
> > > -extern unsigned int sysctl_sched_migration_cost;
> > > -extern unsigned int sysctl_sched_nr_migrate;
> > > -extern unsigned int sysctl_sched_time_avg;
> > > +extern __read_mostly unsigned int sysctl_sched_migration_cost;
> > > +extern __read_mostly unsigned int sysctl_sched_nr_migrate;
> > > +extern __read_mostly unsigned int sysctl_sched_time_avg;
> >
> > So I hate this change, because it pointlessly duplicates an attribute that should
> > only matter at the definition site.
>
> It's certainly not ideal, and then again essentially the same is done
> in kernel/sched/sched.h, just that here the specific attribute is
> hidden behind const_debug.
Ok - and that indeed is a problem and makes the Clang build warning useful,
as there are two conflicting prototypes and one definition.
I'll apply your fix.
Thanks,
Ingo
^ permalink raw reply [flat|nested] 6+ messages in thread
* [tip:sched/core] sched/sysctl: Fix attributes of some extern declarations
2017-10-30 18:08 [PATCH v2] sched/sysctl: Fix attributes of some extern declarations Matthias Kaehlcke
2017-10-31 9:57 ` Ingo Molnar
@ 2017-11-01 8:45 ` tip-bot for Matthias Kaehlcke
1 sibling, 0 replies; 6+ messages in thread
From: tip-bot for Matthias Kaehlcke @ 2017-11-01 8:45 UTC (permalink / raw)
To: linux-tip-commits
Cc: groeck, mka, linux-kernel, torvalds, mingo, nick.desaulniers,
tglx, dianders, shile.zhang, peterz, hpa
Commit-ID: a9903f04e0a4ea522d959c2f287cdf0ab029e324
Gitweb: https://git.kernel.org/tip/a9903f04e0a4ea522d959c2f287cdf0ab029e324
Author: Matthias Kaehlcke <mka@chromium.org>
AuthorDate: Mon, 30 Oct 2017 11:08:16 -0700
Committer: Ingo Molnar <mingo@kernel.org>
CommitDate: Wed, 1 Nov 2017 09:36:17 +0100
sched/sysctl: Fix attributes of some extern declarations
The definition of sysctl_sched_migration_cost, sysctl_sched_nr_migrate
and sysctl_sched_time_avg includes the attribute const_debug. This
attribute is not part of the extern declaration of these variables in
include/linux/sched/sysctl.h, while it is in kernel/sched/sched.h,
and as a result Clang generates warnings like this:
kernel/sched/sched.h:1618:33: warning: section attribute is specified on redeclared variable [-Wsection]
extern const_debug unsigned int sysctl_sched_time_avg;
^
./include/linux/sched/sysctl.h:42:21: note: previous declaration is here
extern unsigned int sysctl_sched_time_avg;
The header only declares the variables when CONFIG_SCHED_DEBUG is defined,
therefore it is not necessary to duplicate the definition of const_debug.
Instead we can use the attribute __read_mostly, which is the expansion of
const_debug when CONFIG_SCHED_DEBUG=y is set.
Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
Reviewed-by: Nick Desaulniers <nick.desaulniers@gmail.com>
Cc: Douglas Anderson <dianders@chromium.org>
Cc: Guenter Roeck <groeck@chromium.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Shile Zhang <shile.zhang@nokia.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Link: http://lkml.kernel.org/r/20171030180816.170850-1-mka@chromium.org
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
include/linux/sched/sysctl.h | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/include/linux/sched/sysctl.h b/include/linux/sched/sysctl.h
index 0f5ecd4..d34c823 100644
--- a/include/linux/sched/sysctl.h
+++ b/include/linux/sched/sysctl.h
@@ -37,9 +37,9 @@ extern unsigned int sysctl_numa_balancing_scan_period_max;
extern unsigned int sysctl_numa_balancing_scan_size;
#ifdef CONFIG_SCHED_DEBUG
-extern unsigned int sysctl_sched_migration_cost;
-extern unsigned int sysctl_sched_nr_migrate;
-extern unsigned int sysctl_sched_time_avg;
+extern __read_mostly unsigned int sysctl_sched_migration_cost;
+extern __read_mostly unsigned int sysctl_sched_nr_migrate;
+extern __read_mostly unsigned int sysctl_sched_time_avg;
int sched_proc_update_handler(struct ctl_table *table, int write,
void __user *buffer, size_t *length,
^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-11-01 8:51 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-10-30 18:08 [PATCH v2] sched/sysctl: Fix attributes of some extern declarations Matthias Kaehlcke
2017-10-31 9:57 ` Ingo Molnar
2017-10-31 22:31 ` Matthias Kaehlcke
2017-11-01 4:33 ` Nick Desaulniers
2017-11-01 8:34 ` Ingo Molnar
2017-11-01 8:45 ` [tip:sched/core] " tip-bot for Matthias Kaehlcke
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).