* [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 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.