tree: https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git sched/fifo
head: 8b700983de82f79e05b2c1136d6513ea4c9b22c4
commit: 616d91b68cd56bcb1954b6a5af7d542401fde772 [44/45] sched: Remove sched_setscheduler*() EXPORTs
config: x86_64-rhel (attached as .config)
compiler: gcc-9 (Debian 9.3.0-13) 9.3.0
reproduce (this is a W=1 build):
git checkout 616d91b68cd56bcb1954b6a5af7d542401fde772
# save the attached .config to linux build tree
make W=1 ARCH=x86_64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>
All errors (new ones prefixed by >>, old ones prefixed by <<):
>> ERROR: modpost: "sched_setscheduler" [kernel/trace/ring_buffer_benchmark.ko] undefined!
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]
On Fri, Jun 19, 2020 at 10:15:51PM +0800, kernel test robot wrote:
> tree: https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git sched/fifo
> head: 8b700983de82f79e05b2c1136d6513ea4c9b22c4
> commit: 616d91b68cd56bcb1954b6a5af7d542401fde772 [44/45] sched: Remove sched_setscheduler*() EXPORTs
> config: x86_64-rhel (attached as .config)
> compiler: gcc-9 (Debian 9.3.0-13) 9.3.0
> reproduce (this is a W=1 build):
> git checkout 616d91b68cd56bcb1954b6a5af7d542401fde772
> # save the attached .config to linux build tree
> make W=1 ARCH=x86_64
>
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <[email protected]>
>
> All errors (new ones prefixed by >>, old ones prefixed by <<):
>
> >> ERROR: modpost: "sched_setscheduler" [kernel/trace/ring_buffer_benchmark.ko] undefined!
Steve, do you have any preference on how to go about fixing this one?
On Thu, 9 Jul 2020 14:45:05 +0200
Peter Zijlstra <[email protected]> wrote:
> On Fri, Jun 19, 2020 at 10:15:51PM +0800, kernel test robot wrote:
> > tree: https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git sched/fifo
> > head: 8b700983de82f79e05b2c1136d6513ea4c9b22c4
> > commit: 616d91b68cd56bcb1954b6a5af7d542401fde772 [44/45] sched: Remove sched_setscheduler*() EXPORTs
> > config: x86_64-rhel (attached as .config)
> > compiler: gcc-9 (Debian 9.3.0-13) 9.3.0
> > reproduce (this is a W=1 build):
> > git checkout 616d91b68cd56bcb1954b6a5af7d542401fde772
> > # save the attached .config to linux build tree
> > make W=1 ARCH=x86_64
> >
> > If you fix the issue, kindly add following tag as appropriate
> > Reported-by: kernel test robot <[email protected]>
> >
> > All errors (new ones prefixed by >>, old ones prefixed by <<):
> >
> > >> ERROR: modpost: "sched_setscheduler" [kernel/trace/ring_buffer_benchmark.ko] undefined!
>
> Steve, do you have any preference on how to go about fixing this one?
Well, I use to manually set the priority of the test, but I guess we
can switch the parameters to accepting a "high, medium, and low" that
will correspond to your setting of the other sched_setscheduler() calls
that were replaced.
-- Steve
On Thu, Jul 09, 2020 at 11:58:18AM -0400, Steven Rostedt wrote:
> On Thu, 9 Jul 2020 14:45:05 +0200
> Peter Zijlstra <[email protected]> wrote:
>
> > On Fri, Jun 19, 2020 at 10:15:51PM +0800, kernel test robot wrote:
> > > tree: https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git sched/fifo
> > > head: 8b700983de82f79e05b2c1136d6513ea4c9b22c4
> > > commit: 616d91b68cd56bcb1954b6a5af7d542401fde772 [44/45] sched: Remove sched_setscheduler*() EXPORTs
> > > config: x86_64-rhel (attached as .config)
> > > compiler: gcc-9 (Debian 9.3.0-13) 9.3.0
> > > reproduce (this is a W=1 build):
> > > git checkout 616d91b68cd56bcb1954b6a5af7d542401fde772
> > > # save the attached .config to linux build tree
> > > make W=1 ARCH=x86_64
> > >
> > > If you fix the issue, kindly add following tag as appropriate
> > > Reported-by: kernel test robot <[email protected]>
> > >
> > > All errors (new ones prefixed by >>, old ones prefixed by <<):
> > >
> > > >> ERROR: modpost: "sched_setscheduler" [kernel/trace/ring_buffer_benchmark.ko] undefined!
> >
> > Steve, do you have any preference on how to go about fixing this one?
>
> Well, I use to manually set the priority of the test, but I guess we
> can switch the parameters to accepting a "high, medium, and low" that
> will correspond to your setting of the other sched_setscheduler() calls
> that were replaced.
Steve, would this work for you, or would you prefer renaming the
parameters as well?
---
kernel/trace/ring_buffer_benchmark.c | 22 ++++++++++------------
1 file changed, 10 insertions(+), 12 deletions(-)
diff --git a/kernel/trace/ring_buffer_benchmark.c b/kernel/trace/ring_buffer_benchmark.c
index 8df0aa810950..0ee3d41ceee4 100644
--- a/kernel/trace/ring_buffer_benchmark.c
+++ b/kernel/trace/ring_buffer_benchmark.c
@@ -455,21 +455,19 @@ static int __init ring_buffer_benchmark_init(void)
* Run them as low-prio background tasks by default:
*/
if (!disable_reader) {
- if (consumer_fifo >= 0) {
- struct sched_param param = {
- .sched_priority = consumer_fifo
- };
- sched_setscheduler(consumer, SCHED_FIFO, ¶m);
- } else
+ if (consumer_fifo > 1)
+ sched_set_fifo(consumer);
+ else if (consumer_fifo >= 0)
+ sched_set_fifo_low(consumer);
+ else
set_user_nice(consumer, consumer_nice);
}
- if (producer_fifo >= 0) {
- struct sched_param param = {
- .sched_priority = producer_fifo
- };
- sched_setscheduler(producer, SCHED_FIFO, ¶m);
- } else
+ if (producer_fifo > 1)
+ sched_set_fifo(producer);
+ else if (producer_fifo >= 0)
+ sched_set_fifo_low(producer);
+ else
set_user_nice(producer, producer_nice);
return 0;
On Mon, 20 Jul 2020 23:49:18 +0200
Peter Zijlstra <[email protected]> wrote:
> Steve, would this work for you, or would you prefer renaming the
> parameters as well?
>
Yeah, that's fine. You don't have any sched_fifo_high() ?
Sometimes I see what the numbers are at prio 98.
Not a big deal, as I seldom do that, and I'm probably the only one
doing it. I can live with patching the kernel to do that test.
-- Steve
On Mon, Jul 20, 2020 at 06:19:43PM -0400, Steven Rostedt wrote:
> On Mon, 20 Jul 2020 23:49:18 +0200
> Peter Zijlstra <[email protected]> wrote:
>
> > Steve, would this work for you, or would you prefer renaming the
> > parameters as well?
> >
>
> Yeah, that's fine. You don't have any sched_fifo_high() ?
Thanks! and no.
I'll go write a Changelog and add it to tip/sched/fifo, so that
hopefully, sfr can stop complaining about this build fail ;-)
I've even argued we should rename fifo_low() to something else, but
failed to come up with a sensible name. The intended case is for when
you want something above normal but don't particularly care about RT at
all.
The thing is, once you start adding priorities, even low,med,high, we're
back to where we were. And the whole argument is that the kernel cannot
set priorities in any sensible fashion.
On 07/21/20 10:36, [email protected] wrote:
> On Mon, Jul 20, 2020 at 06:19:43PM -0400, Steven Rostedt wrote:
> > On Mon, 20 Jul 2020 23:49:18 +0200
> > Peter Zijlstra <[email protected]> wrote:
> >
> > > Steve, would this work for you, or would you prefer renaming the
> > > parameters as well?
> > >
> >
> > Yeah, that's fine. You don't have any sched_fifo_high() ?
>
> Thanks! and no.
>
> I'll go write a Changelog and add it to tip/sched/fifo, so that
> hopefully, sfr can stop complaining about this build fail ;-)
>
> I've even argued we should rename fifo_low() to something else, but
> failed to come up with a sensible name. The intended case is for when
> you want something above normal but don't particularly care about RT at
> all.
>
> The thing is, once you start adding priorities, even low,med,high, we're
> back to where we were. And the whole argument is that the kernel cannot
> set priorities in any sensible fashion.
Agreed. I am worried about in-kernel users setting random uclamp values too.
This series should do most of the work but there are more pieces needed on-top.
From what I see we still need to move the sched_setscheduler() from
include/linux/sched.h to kernel/sched/sched.h. And sched_setattr() too. The
latter has a single user in kernel/trace/trace_selftest.c to create a deadline
task. I think that can be easily wrapped with a similar sched_set_dl()
function and exported instead.
Happy to do the work if you nudge me after you've published this fix on tip or
your queue.
Thanks
--
Qais Yousef
On Tue, 21 Jul 2020 10:36:43 +0200
[email protected] wrote:
> > Yeah, that's fine. You don't have any sched_fifo_high() ?
>
> Thanks! and no.
>
> I'll go write a Changelog and add it to tip/sched/fifo, so that
> hopefully, sfr can stop complaining about this build fail ;-)
>
> I've even argued we should rename fifo_low() to something else, but
> failed to come up with a sensible name. The intended case is for when
> you want something above normal but don't particularly care about RT at
> all.
>
> The thing is, once you start adding priorities, even low,med,high, we're
> back to where we were. And the whole argument is that the kernel cannot
> set priorities in any sensible fashion.
Actually, I was wondering about a "sched_fifo_benchmark()" used
specifically for internal testing, where you *want* to disrupt the
system. Perhaps have it depend on CONFIG_DEBUG to at least scare
people away from using it for normal production code. Or make it print
a nasty banner like trace_printk() does. That worked pretty well at
keeping people from using it ;-)
-- Steve
On Mon, Jul 20, 2020 at 11:49:18PM +0200, Peter Zijlstra wrote:
> Steve, would this work for you, or would you prefer renaming the
> parameters as well?
Steve mentioned he's like to have the parameters changes after all.
How's this then?
---
Subject: sched,tracing: Convert to sched_set_fifo()
From: Peter Zijlstra <[email protected]>
Date: Mon, 20 Jul 2020 23:49:18 +0200
One module user of sched_setscheduler() was overlooked and is
obviously causing build failures.
Convert ring_buffer_benchmark to use sched_set_fifo_low() when fifo==1
and sched_set_fifo() when fifo==2. This is a bit of an abuse, but it
makes the thing 'work' again.
Specifically, it enables all combinations that were previously
possible:
producer higher than consumer
consumer higher than producer
Fixes: 616d91b68cd5 ("sched: Remove sched_setscheduler*() EXPORTs")
Reported-by: kernel test robot <[email protected]>
Reported-by: Stephen Rothwell <[email protected]>
Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
---
kernel/trace/ring_buffer_benchmark.c | 48 ++++++++++++++++-------------------
1 file changed, 23 insertions(+), 25 deletions(-)
--- a/kernel/trace/ring_buffer_benchmark.c
+++ b/kernel/trace/ring_buffer_benchmark.c
@@ -45,8 +45,8 @@ MODULE_PARM_DESC(write_iteration, "# of
static int producer_nice = MAX_NICE;
static int consumer_nice = MAX_NICE;
-static int producer_fifo = -1;
-static int consumer_fifo = -1;
+static int producer_fifo = 0;
+static int consumer_fifo = 0;
module_param(producer_nice, int, 0644);
MODULE_PARM_DESC(producer_nice, "nice prio for producer");
@@ -55,10 +55,10 @@ module_param(consumer_nice, int, 0644);
MODULE_PARM_DESC(consumer_nice, "nice prio for consumer");
module_param(producer_fifo, int, 0644);
-MODULE_PARM_DESC(producer_fifo, "fifo prio for producer");
+MODULE_PARM_DESC(producer_fifo, "use fifo for producer: 0 - disabled, 1 - low prio, 2 - fifo");
module_param(consumer_fifo, int, 0644);
-MODULE_PARM_DESC(consumer_fifo, "fifo prio for consumer");
+MODULE_PARM_DESC(consumer_fifo, "use fifo for consumer: 0 - disabled, 1 - low prio, 2 - fifo");
static int read_events;
@@ -303,22 +303,22 @@ static void ring_buffer_producer(void)
trace_printk("ERROR!\n");
if (!disable_reader) {
- if (consumer_fifo < 0)
- trace_printk("Running Consumer at nice: %d\n",
- consumer_nice);
- else
+ if (consumer_fifo)
trace_printk("Running Consumer at SCHED_FIFO %d\n",
consumer_fifo);
+ else
+ trace_printk("Running Consumer at nice: %d\n",
+ consumer_nice);
}
- if (producer_fifo < 0)
- trace_printk("Running Producer at nice: %d\n",
- producer_nice);
- else
+ if (producer_fifo)
trace_printk("Running Producer at SCHED_FIFO %d\n",
producer_fifo);
+ else
+ trace_printk("Running Producer at nice: %d\n",
+ producer_nice);
/* Let the user know that the test is running at low priority */
- if (producer_fifo < 0 && consumer_fifo < 0 &&
+ if (!producer_fifo && !consumer_fifo &&
producer_nice == MAX_NICE && consumer_nice == MAX_NICE)
trace_printk("WARNING!!! This test is running at lowest priority.\n");
@@ -455,21 +455,19 @@ static int __init ring_buffer_benchmark_
* Run them as low-prio background tasks by default:
*/
if (!disable_reader) {
- if (consumer_fifo >= 0) {
- struct sched_param param = {
- .sched_priority = consumer_fifo
- };
- sched_setscheduler(consumer, SCHED_FIFO, ¶m);
- } else
+ if (consumer_fifo == 2)
+ sched_set_fifo(consumer);
+ else if (consumer_fifo == 1)
+ sched_set_fifo_low(consumer);
+ else
set_user_nice(consumer, consumer_nice);
}
- if (producer_fifo >= 0) {
- struct sched_param param = {
- .sched_priority = producer_fifo
- };
- sched_setscheduler(producer, SCHED_FIFO, ¶m);
- } else
+ if (producer_fifo == 2)
+ sched_set_fifo(producer);
+ else if (producer_fifo == 1)
+ sched_set_fifo_low(producer);
+ else
set_user_nice(producer, producer_nice);
return 0;
On Fri, 24 Jul 2020 23:39:11 +0200
[email protected] wrote:
> On Mon, Jul 20, 2020 at 11:49:18PM +0200, Peter Zijlstra wrote:
> > Steve, would this work for you, or would you prefer renaming the
> > parameters as well?
>
> Steve mentioned he's like to have the parameters changes after all.
> How's this then?
>
> ---
> Subject: sched,tracing: Convert to sched_set_fifo()
> From: Peter Zijlstra <[email protected]>
> Date: Mon, 20 Jul 2020 23:49:18 +0200
>
> One module user of sched_setscheduler() was overlooked and is
> obviously causing build failures.
>
> Convert ring_buffer_benchmark to use sched_set_fifo_low() when fifo==1
> and sched_set_fifo() when fifo==2. This is a bit of an abuse, but it
> makes the thing 'work' again.
>
> Specifically, it enables all combinations that were previously
> possible:
>
> producer higher than consumer
> consumer higher than producer
>
> Fixes: 616d91b68cd5 ("sched: Remove sched_setscheduler*() EXPORTs")
> Reported-by: kernel test robot <[email protected]>
> Reported-by: Stephen Rothwell <[email protected]>
> Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
> ---
> kernel/trace/ring_buffer_benchmark.c | 48 ++++++++++++++++-------------------
> 1 file changed, 23 insertions(+), 25 deletions(-)
>
> --- a/kernel/trace/ring_buffer_benchmark.c
> +++ b/kernel/trace/ring_buffer_benchmark.c
> @@ -45,8 +45,8 @@ MODULE_PARM_DESC(write_iteration, "# of
> static int producer_nice = MAX_NICE;
> static int consumer_nice = MAX_NICE;
>
> -static int producer_fifo = -1;
> -static int consumer_fifo = -1;
> +static int producer_fifo = 0;
> +static int consumer_fifo = 0;
The initialization of zero for static variables isn't needed.
>
> module_param(producer_nice, int, 0644);
> MODULE_PARM_DESC(producer_nice, "nice prio for producer");
> @@ -55,10 +55,10 @@ module_param(consumer_nice, int, 0644);
> MODULE_PARM_DESC(consumer_nice, "nice prio for consumer");
>
> module_param(producer_fifo, int, 0644);
> -MODULE_PARM_DESC(producer_fifo, "fifo prio for producer");
> +MODULE_PARM_DESC(producer_fifo, "use fifo for producer: 0 - disabled, 1 - low prio, 2 - fifo");
>
> module_param(consumer_fifo, int, 0644);
> -MODULE_PARM_DESC(consumer_fifo, "fifo prio for consumer");
> +MODULE_PARM_DESC(consumer_fifo, "use fifo for consumer: 0 - disabled, 1 - low prio, 2 - fifo");
>
> static int read_events;
>
> @@ -303,22 +303,22 @@ static void ring_buffer_producer(void)
> trace_printk("ERROR!\n");
>
> if (!disable_reader) {
> - if (consumer_fifo < 0)
> - trace_printk("Running Consumer at nice: %d\n",
> - consumer_nice);
> - else
> + if (consumer_fifo)
> trace_printk("Running Consumer at SCHED_FIFO %d\n",
> consumer_fifo);
Can we change the above to:
trace_printk("Running Consumer at SCHED_FIFO %s\n",
consumer_fifo == 1 ? "low" : "high");
> + else
> + trace_printk("Running Consumer at nice: %d\n",
> + consumer_nice);
> }
> - if (producer_fifo < 0)
> - trace_printk("Running Producer at nice: %d\n",
> - producer_nice);
> - else
> + if (producer_fifo)
> trace_printk("Running Producer at SCHED_FIFO %d\n",
> producer_fifo);
Same here.
> + else
> + trace_printk("Running Producer at nice: %d\n",
> + producer_nice);
>
> /* Let the user know that the test is running at low priority */
> - if (producer_fifo < 0 && consumer_fifo < 0 &&
> + if (!producer_fifo && !consumer_fifo &&
> producer_nice == MAX_NICE && consumer_nice == MAX_NICE)
> trace_printk("WARNING!!! This test is running at lowest priority.\n");
>
> @@ -455,21 +455,19 @@ static int __init ring_buffer_benchmark_
> * Run them as low-prio background tasks by default:
> */
> if (!disable_reader) {
> - if (consumer_fifo >= 0) {
> - struct sched_param param = {
> - .sched_priority = consumer_fifo
> - };
> - sched_setscheduler(consumer, SCHED_FIFO, ¶m);
> - } else
> + if (consumer_fifo == 2)
Let's make this be:
if (consumer_fifo > 1)
Then if someone sends in 3 it doesn't go low again.
> + sched_set_fifo(consumer);
> + else if (consumer_fifo == 1)
> + sched_set_fifo_low(consumer);
> + else
> set_user_nice(consumer, consumer_nice);
> }
>
> - if (producer_fifo >= 0) {
> - struct sched_param param = {
> - .sched_priority = producer_fifo
> - };
> - sched_setscheduler(producer, SCHED_FIFO, ¶m);
> - } else
> + if (producer_fifo == 2)
Same here.
-- Steve
> + sched_set_fifo(producer);
> + else if (producer_fifo == 1)
> + sched_set_fifo_low(producer);
> + else
> set_user_nice(producer, producer_nice);
>
> return 0;
Subject: sched,tracing: Convert to sched_set_fifo()
From: Peter Zijlstra <[email protected]>
Date: Mon, 20 Jul 2020 23:49:18 +0200
One module user of sched_setscheduler() was overlooked and is
obviously causing build failures.
Convert ring_buffer_benchmark to use sched_set_fifo_low() when fifo==1
and sched_set_fifo() when fifo==2. This is a bit of an abuse, but it
makes the thing 'work' again.
Specifically, it enables all combinations that were previously
possible:
producer higher than consumer
consumer higher than producer
Fixes: 616d91b68cd5 ("sched: Remove sched_setscheduler*() EXPORTs")
Reported-by: kernel test robot <[email protected]>
Reported-by: Stephen Rothwell <[email protected]>
Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
---
kernel/trace/ring_buffer_benchmark.c | 48 ++++++++++++++++-------------------
1 file changed, 23 insertions(+), 25 deletions(-)
--- a/kernel/trace/ring_buffer_benchmark.c
+++ b/kernel/trace/ring_buffer_benchmark.c
@@ -45,8 +45,8 @@ MODULE_PARM_DESC(write_iteration, "# of
static int producer_nice = MAX_NICE;
static int consumer_nice = MAX_NICE;
-static int producer_fifo = -1;
-static int consumer_fifo = -1;
+static int producer_fifo;
+static int consumer_fifo;
module_param(producer_nice, int, 0644);
MODULE_PARM_DESC(producer_nice, "nice prio for producer");
@@ -55,10 +55,10 @@ module_param(consumer_nice, int, 0644);
MODULE_PARM_DESC(consumer_nice, "nice prio for consumer");
module_param(producer_fifo, int, 0644);
-MODULE_PARM_DESC(producer_fifo, "fifo prio for producer");
+MODULE_PARM_DESC(producer_fifo, "use fifo for producer: 0 - disabled, 1 - low prio, 2 - fifo");
module_param(consumer_fifo, int, 0644);
-MODULE_PARM_DESC(consumer_fifo, "fifo prio for consumer");
+MODULE_PARM_DESC(consumer_fifo, "use fifo for consumer: 0 - disabled, 1 - low prio, 2 - fifo");
static int read_events;
@@ -303,22 +303,22 @@ static void ring_buffer_producer(void)
trace_printk("ERROR!\n");
if (!disable_reader) {
- if (consumer_fifo < 0)
+ if (consumer_fifo)
+ trace_printk("Running Consumer at SCHED_FIFO %s\n",
+ consumer_fifo == 1 ? "low" : "high");
+ else
trace_printk("Running Consumer at nice: %d\n",
consumer_nice);
- else
- trace_printk("Running Consumer at SCHED_FIFO %d\n",
- consumer_fifo);
}
- if (producer_fifo < 0)
+ if (producer_fifo)
+ trace_printk("Running Producer at SCHED_FIFO %s\n",
+ consumer_fifo == 1 ? "low" : "high");
+ else
trace_printk("Running Producer at nice: %d\n",
producer_nice);
- else
- trace_printk("Running Producer at SCHED_FIFO %d\n",
- producer_fifo);
/* Let the user know that the test is running at low priority */
- if (producer_fifo < 0 && consumer_fifo < 0 &&
+ if (!producer_fifo && !consumer_fifo &&
producer_nice == MAX_NICE && consumer_nice == MAX_NICE)
trace_printk("WARNING!!! This test is running at lowest priority.\n");
@@ -455,21 +455,19 @@ static int __init ring_buffer_benchmark_
* Run them as low-prio background tasks by default:
*/
if (!disable_reader) {
- if (consumer_fifo >= 0) {
- struct sched_param param = {
- .sched_priority = consumer_fifo
- };
- sched_setscheduler(consumer, SCHED_FIFO, ¶m);
- } else
+ if (consumer_fifo >= 2)
+ sched_set_fifo(consumer);
+ else if (consumer_fifo == 1)
+ sched_set_fifo_low(consumer);
+ else
set_user_nice(consumer, consumer_nice);
}
- if (producer_fifo >= 0) {
- struct sched_param param = {
- .sched_priority = producer_fifo
- };
- sched_setscheduler(producer, SCHED_FIFO, ¶m);
- } else
+ if (producer_fifo >= 2)
+ sched_set_fifo(producer);
+ else if (producer_fifo == 1)
+ sched_set_fifo_low(producer);
+ else
set_user_nice(producer, producer_nice);
return 0;
On Fri, 24 Jul 2020 23:50:03 +0200
[email protected] wrote:
> - if (producer_fifo < 0)
> + if (producer_fifo)
> + trace_printk("Running Producer at SCHED_FIFO %s\n",
> + consumer_fifo == 1 ? "low" : "high");
I'm going to take cut-and-paste away from you!
-- Steve
> + else
> trace_printk("Running Producer at nice: %d\n",
> producer_nice);
On Fri, Jul 24, 2020 at 06:18:46PM -0400, Steven Rostedt wrote:
> On Fri, 24 Jul 2020 23:50:03 +0200
> [email protected] wrote:
>
> > - if (producer_fifo < 0)
> > + if (producer_fifo)
> > + trace_printk("Running Producer at SCHED_FIFO %s\n",
> > + consumer_fifo == 1 ? "low" : "high");
>
> I'm going to take cut-and-paste away from you!
Well, yes, I said so, I also already have it fixed.
Aside from that, is the patch ok?
On Sat, 25 Jul 2020 18:58:16 +0200
Peter Zijlstra <[email protected]> wrote:
> On Fri, Jul 24, 2020 at 06:18:46PM -0400, Steven Rostedt wrote:
> > On Fri, 24 Jul 2020 23:50:03 +0200
> > [email protected] wrote:
> >
> > > - if (producer_fifo < 0)
> > > + if (producer_fifo)
> > > + trace_printk("Running Producer at SCHED_FIFO %s\n",
> > > + consumer_fifo == 1 ? "low" : "high");
> >
> > I'm going to take cut-and-paste away from you!
>
> Well, yes, I said so, I also already have it fixed.
I thought you're comment on IRC was about v1, as I read it before
seeing v2.
>
> Aside from that, is the patch ok?
Beside this, yes.
Reviewed-by: Steven Rostedt (VMware) <[email protected]>
-- Steve
On 21/07/2020 12:13, Qais Yousef wrote:
> On 07/21/20 10:36, [email protected] wrote:
>> On Mon, Jul 20, 2020 at 06:19:43PM -0400, Steven Rostedt wrote:
>>> On Mon, 20 Jul 2020 23:49:18 +0200
>>> Peter Zijlstra <[email protected]> wrote:
>>>
>>>> Steve, would this work for you, or would you prefer renaming the
>>>> parameters as well?
>>>>
>>>
>>> Yeah, that's fine. You don't have any sched_fifo_high() ?
>>
>> Thanks! and no.
>>
>> I'll go write a Changelog and add it to tip/sched/fifo, so that
>> hopefully, sfr can stop complaining about this build fail ;-)
>>
>> I've even argued we should rename fifo_low() to something else, but
>> failed to come up with a sensible name. The intended case is for when
>> you want something above normal but don't particularly care about RT at
>> all.
>>
>> The thing is, once you start adding priorities, even low,med,high, we're
>> back to where we were. And the whole argument is that the kernel cannot
>> set priorities in any sensible fashion.
>
> Agreed. I am worried about in-kernel users setting random uclamp values too.
Do we really have to restrict in-kernel user?
And avoiding module uclamp abuse is covered by 616d91b68cd5 ("sched:
Remove sched_setscheduler*() EXPORTs").
> This series should do most of the work but there are more pieces needed on-top.
>
> From what I see we still need to move the sched_setscheduler() from
> include/linux/sched.h to kernel/sched/sched.h. And sched_setattr() too. The
> latter has a single user in kernel/trace/trace_selftest.c to create a deadline
> task. I think that can be easily wrapped with a similar sched_set_dl()
> function and exported instead.
But DL does not have the same issue like the FIFO/RR when it comes to
resource management.
Not sure if we have to restrict in-kernel user.
[...]
On 07/29/20 12:23, Dietmar Eggemann wrote:
> On 21/07/2020 12:13, Qais Yousef wrote:
> > On 07/21/20 10:36, [email protected] wrote:
> >> On Mon, Jul 20, 2020 at 06:19:43PM -0400, Steven Rostedt wrote:
> >>> On Mon, 20 Jul 2020 23:49:18 +0200
> >>> Peter Zijlstra <[email protected]> wrote:
> >>>
> >>>> Steve, would this work for you, or would you prefer renaming the
> >>>> parameters as well?
> >>>>
> >>>
> >>> Yeah, that's fine. You don't have any sched_fifo_high() ?
> >>
> >> Thanks! and no.
> >>
> >> I'll go write a Changelog and add it to tip/sched/fifo, so that
> >> hopefully, sfr can stop complaining about this build fail ;-)
> >>
> >> I've even argued we should rename fifo_low() to something else, but
> >> failed to come up with a sensible name. The intended case is for when
> >> you want something above normal but don't particularly care about RT at
> >> all.
> >>
> >> The thing is, once you start adding priorities, even low,med,high, we're
> >> back to where we were. And the whole argument is that the kernel cannot
> >> set priorities in any sensible fashion.
> >
> > Agreed. I am worried about in-kernel users setting random uclamp values too.
>
> Do we really have to restrict in-kernel user?
>
> And avoiding module uclamp abuse is covered by 616d91b68cd5 ("sched:
> Remove sched_setscheduler*() EXPORTs").
The worry is not just about modules abuse IMO. We can put a filter in our
emails to catch all patches that try to use this API. I don't think we can
assume we'd catch all.
>
> > This series should do most of the work but there are more pieces needed on-top.
> >
> > From what I see we still need to move the sched_setscheduler() from
> > include/linux/sched.h to kernel/sched/sched.h. And sched_setattr() too. The
> > latter has a single user in kernel/trace/trace_selftest.c to create a deadline
> > task. I think that can be easily wrapped with a similar sched_set_dl()
> > function and exported instead.
>
> But DL does not have the same issue like the FIFO/RR when it comes to
> resource management.
> Not sure if we have to restrict in-kernel user.
I didn't think much about it. But we can relax the wrapper if really needed.
IMO the kernel should present a predictable behavior for userspace. But I don't
know a lot about DL to comment. The easy answer the wrapper could be relaxed to
offer the required tunables without giving direct access to
sched_setscheduler().
Thanks
--
Qais Yousef
The following commit has been merged into the sched/fifo branch of tip:
Commit-ID: 4fd5750af02ab7bba7c58a073060cc1da8a69173
Gitweb: https://git.kernel.org/tip/4fd5750af02ab7bba7c58a073060cc1da8a69173
Author: Peter Zijlstra <[email protected]>
AuthorDate: Mon, 20 Jul 2020 23:49:18 +02:00
Committer: Peter Zijlstra <[email protected]>
CommitterDate: Wed, 29 Jul 2020 11:43:53 +02:00
sched,tracing: Convert to sched_set_fifo()
One module user of sched_setscheduler() was overlooked and is
obviously causing build failures.
Convert ring_buffer_benchmark to use sched_set_fifo_low() when fifo==1
and sched_set_fifo() when fifo==2. This is a bit of an abuse, but it
makes the thing 'work' again.
Specifically, it enables all combinations that were previously
possible:
producer higher than consumer
consumer higher than producer
Fixes: 616d91b68cd5 ("sched: Remove sched_setscheduler*() EXPORTs")
Reported-by: kernel test robot <[email protected]>
Reported-by: Stephen Rothwell <[email protected]>
Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
Reviewed-by: Steven Rostedt (VMware) <[email protected]>
Link: https://lkml.kernel.org/r/[email protected]
---
kernel/trace/ring_buffer_benchmark.c | 48 ++++++++++++---------------
1 file changed, 23 insertions(+), 25 deletions(-)
diff --git a/kernel/trace/ring_buffer_benchmark.c b/kernel/trace/ring_buffer_benchmark.c
index 8df0aa8..78e5765 100644
--- a/kernel/trace/ring_buffer_benchmark.c
+++ b/kernel/trace/ring_buffer_benchmark.c
@@ -45,8 +45,8 @@ MODULE_PARM_DESC(write_iteration, "# of writes between timestamp readings");
static int producer_nice = MAX_NICE;
static int consumer_nice = MAX_NICE;
-static int producer_fifo = -1;
-static int consumer_fifo = -1;
+static int producer_fifo;
+static int consumer_fifo;
module_param(producer_nice, int, 0644);
MODULE_PARM_DESC(producer_nice, "nice prio for producer");
@@ -55,10 +55,10 @@ module_param(consumer_nice, int, 0644);
MODULE_PARM_DESC(consumer_nice, "nice prio for consumer");
module_param(producer_fifo, int, 0644);
-MODULE_PARM_DESC(producer_fifo, "fifo prio for producer");
+MODULE_PARM_DESC(producer_fifo, "use fifo for producer: 0 - disabled, 1 - low prio, 2 - fifo");
module_param(consumer_fifo, int, 0644);
-MODULE_PARM_DESC(consumer_fifo, "fifo prio for consumer");
+MODULE_PARM_DESC(consumer_fifo, "use fifo for consumer: 0 - disabled, 1 - low prio, 2 - fifo");
static int read_events;
@@ -303,22 +303,22 @@ static void ring_buffer_producer(void)
trace_printk("ERROR!\n");
if (!disable_reader) {
- if (consumer_fifo < 0)
+ if (consumer_fifo)
+ trace_printk("Running Consumer at SCHED_FIFO %s\n",
+ consumer_fifo == 1 ? "low" : "high");
+ else
trace_printk("Running Consumer at nice: %d\n",
consumer_nice);
- else
- trace_printk("Running Consumer at SCHED_FIFO %d\n",
- consumer_fifo);
}
- if (producer_fifo < 0)
+ if (producer_fifo)
+ trace_printk("Running Producer at SCHED_FIFO %s\n",
+ producer_fifo == 1 ? "low" : "high");
+ else
trace_printk("Running Producer at nice: %d\n",
producer_nice);
- else
- trace_printk("Running Producer at SCHED_FIFO %d\n",
- producer_fifo);
/* Let the user know that the test is running at low priority */
- if (producer_fifo < 0 && consumer_fifo < 0 &&
+ if (!producer_fifo && !consumer_fifo &&
producer_nice == MAX_NICE && consumer_nice == MAX_NICE)
trace_printk("WARNING!!! This test is running at lowest priority.\n");
@@ -455,21 +455,19 @@ static int __init ring_buffer_benchmark_init(void)
* Run them as low-prio background tasks by default:
*/
if (!disable_reader) {
- if (consumer_fifo >= 0) {
- struct sched_param param = {
- .sched_priority = consumer_fifo
- };
- sched_setscheduler(consumer, SCHED_FIFO, ¶m);
- } else
+ if (consumer_fifo >= 2)
+ sched_set_fifo(consumer);
+ else if (consumer_fifo == 1)
+ sched_set_fifo_low(consumer);
+ else
set_user_nice(consumer, consumer_nice);
}
- if (producer_fifo >= 0) {
- struct sched_param param = {
- .sched_priority = producer_fifo
- };
- sched_setscheduler(producer, SCHED_FIFO, ¶m);
- } else
+ if (producer_fifo >= 2)
+ sched_set_fifo(producer);
+ else if (producer_fifo == 1)
+ sched_set_fifo_low(producer);
+ else
set_user_nice(producer, producer_nice);
return 0;
On 07/29/20 12:23, Dietmar Eggemann wrote:
> On 21/07/2020 12:13, Qais Yousef wrote:
> > On 07/21/20 10:36, [email protected] wrote:
> >> On Mon, Jul 20, 2020 at 06:19:43PM -0400, Steven Rostedt wrote:
> >>> On Mon, 20 Jul 2020 23:49:18 +0200
> >>> Peter Zijlstra <[email protected]> wrote:
> >>>
> >>>> Steve, would this work for you, or would you prefer renaming the
> >>>> parameters as well?
> >>>>
> >>>
> >>> Yeah, that's fine. You don't have any sched_fifo_high() ?
> >>
> >> Thanks! and no.
> >>
> >> I'll go write a Changelog and add it to tip/sched/fifo, so that
> >> hopefully, sfr can stop complaining about this build fail ;-)
> >>
> >> I've even argued we should rename fifo_low() to something else, but
> >> failed to come up with a sensible name. The intended case is for when
> >> you want something above normal but don't particularly care about RT at
> >> all.
> >>
> >> The thing is, once you start adding priorities, even low,med,high, we're
> >> back to where we were. And the whole argument is that the kernel cannot
> >> set priorities in any sensible fashion.
> >
> > Agreed. I am worried about in-kernel users setting random uclamp values too.
>
> Do we really have to restrict in-kernel user?
>
> And avoiding module uclamp abuse is covered by 616d91b68cd5 ("sched:
> Remove sched_setscheduler*() EXPORTs").
>
> > This series should do most of the work but there are more pieces needed on-top.
> >
> > From what I see we still need to move the sched_setscheduler() from
> > include/linux/sched.h to kernel/sched/sched.h. And sched_setattr() too. The
> > latter has a single user in kernel/trace/trace_selftest.c to create a deadline
> > task. I think that can be easily wrapped with a similar sched_set_dl()
> > function and exported instead.
>
> But DL does not have the same issue like the FIFO/RR when it comes to
> resource management.
> Not sure if we have to restrict in-kernel user.
FWIW, I have the patches ready and tested for posting.
http://linux-arm.org/git?p=linux-qy.git;a=shortlog;h=refs/heads/sched-setscheduler-hide
Peter, do you think it's not necessary to go to that level of restriction too?
It'll just avoid surprises IMO by making sched_set{sched, attr}() static. The
wrapper can be flexible to change if needed, but at least we'll know.
I addressed Paul's concern on IRC too.
Thanks
--
Qais Yousef