[PATCH 2/2] bcachefs: Set rebalance thread to SCHED_BATCH and nice 19

[PATCH 2/2] bcachefs: Set rebalance thread to SCHED_BATCH and nice 19

[Florian Schmaus]

Set the rebalance thread's scheduling class to BATCH, which means it
could experience a higher scheduling latency. However, it reduces
preemption events of running threads.

And while the rebalance thread is ually not compute bound, it does
cause a considerable amount of I/O. By increasing its nice level from
0 to 19 we also implicitly reduce the thread's best-effort I/O
scheduling class level from 4 to 7. Therefore, the rebalance thread's
I/O operations will be deprioritized over standard I/O operations.

Signed-off-by: Florian Schmaus <flo@geekplace.eu>
---
 fs/bcachefs/rebalance.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/bcachefs/rebalance.c b/fs/bcachefs/rebalance.c
index 4adc74cd3f70..b26c68007c5a 100644
--- a/fs/bcachefs/rebalance.c
+++ b/fs/bcachefs/rebalance.c
@@ -22,6 +22,7 @@
 
 #include <linux/freezer.h>
 #include <linux/kthread.h>
+#include <linux/sched.h>
 #include <linux/sched/cputime.h>
 
 /* bch_extent_rebalance: */
@@ -662,6 +663,8 @@ int bch2_rebalance_start(struct bch_fs *c)
 	if (ret)
 		return ret;
 
+	sched_set_batch(p, 19);
+
 	get_task_struct(p);
 	rcu_assign_pointer(c->rebalance.thread, p);
 	wake_up_process(p);
-- 
2.45.2

Re: [PATCH 2/2] bcachefs: Set rebalance thread to SCHED_BATCH and nice 19

[Kent Overstreet]

On Thu, Nov 14, 2024 at 10:06:48PM +0100, Florian Schmaus wrote:
> Set the rebalance thread's scheduling class to BATCH, which means it
> could experience a higher scheduling latency. However, it reduces
> preemption events of running threads.
> 
> And while the rebalance thread is ually not compute bound, it does
> cause a considerable amount of I/O. By increasing its nice level from
> 0 to 19 we also implicitly reduce the thread's best-effort I/O
> scheduling class level from 4 to 7. Therefore, the rebalance thread's
> I/O operations will be deprioritized over standard I/O operations.

Is there a patch 1/2?

> 
> Signed-off-by: Florian Schmaus <flo@geekplace.eu>
> ---
>  fs/bcachefs/rebalance.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/fs/bcachefs/rebalance.c b/fs/bcachefs/rebalance.c
> index 4adc74cd3f70..b26c68007c5a 100644
> --- a/fs/bcachefs/rebalance.c
> +++ b/fs/bcachefs/rebalance.c
> @@ -22,6 +22,7 @@
>  
>  #include <linux/freezer.h>
>  #include <linux/kthread.h>
> +#include <linux/sched.h>
>  #include <linux/sched/cputime.h>
>  
>  /* bch_extent_rebalance: */
> @@ -662,6 +663,8 @@ int bch2_rebalance_start(struct bch_fs *c)
>  	if (ret)
>  		return ret;
>  
> +	sched_set_batch(p, 19);
> +
>  	get_task_struct(p);
>  	rcu_assign_pointer(c->rebalance.thread, p);
>  	wake_up_process(p);
> -- 
> 2.45.2
> 

Re: [PATCH 2/2] bcachefs: Set rebalance thread to SCHED_BATCH and nice 19

[Florian Schmaus]

[-- Attachment #1.1: Type: text/plain, Size: 866 bytes --]

On 15/11/2024 06.43, Kent Overstreet wrote:
> On Thu, Nov 14, 2024 at 10:06:48PM +0100, Florian Schmaus wrote:
>> Set the rebalance thread's scheduling class to BATCH, which means it
>> could experience a higher scheduling latency. However, it reduces
>> preemption events of running threads.
>>
>> And while the rebalance thread is ually not compute bound, it does
>> cause a considerable amount of I/O. By increasing its nice level from
>> 0 to 19 we also implicitly reduce the thread's best-effort I/O
>> scheduling class level from 4 to 7. Therefore, the rebalance thread's
>> I/O operations will be deprioritized over standard I/O operations.
> 
> Is there a patch 1/2?

Sorry, patch 1/2 was unfortunately not send to linux-bcachefs@. You can 
find it at

https://lore.kernel.org/lkml/20241114210649.71377-1-flo@geekplace.eu/

- Florian



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 618 bytes --]

Re: [PATCH 2/2] bcachefs: Set rebalance thread to SCHED_BATCH and nice 19

[Kent Overstreet]

On Fri, Nov 15, 2024 at 09:26:56AM +0100, Florian Schmaus wrote:
> On 15/11/2024 06.43, Kent Overstreet wrote:
> > On Thu, Nov 14, 2024 at 10:06:48PM +0100, Florian Schmaus wrote:
> > > Set the rebalance thread's scheduling class to BATCH, which means it
> > > could experience a higher scheduling latency. However, it reduces
> > > preemption events of running threads.
> > > 
> > > And while the rebalance thread is ually not compute bound, it does
> > > cause a considerable amount of I/O. By increasing its nice level from
> > > 0 to 19 we also implicitly reduce the thread's best-effort I/O
> > > scheduling class level from 4 to 7. Therefore, the rebalance thread's
> > > I/O operations will be deprioritized over standard I/O operations.
> > 
> > Is there a patch 1/2?
> 
> Sorry, patch 1/2 was unfortunately not send to linux-bcachefs@. You can find
> it at
> 
> https://lore.kernel.org/lkml/20241114210649.71377-1-flo@geekplace.eu/

2/2: https://lore.kernel.org/lkml/20241114210649.71377-2-flo@geekplace.eu/

Ingo, for sanity could we keep these two patches together? your tree or
mine is fine with me, if patch 1/2 is acceptable to you

Florian also had another idea I wanted to mention for giving userspace
control over sched policy - exposing the pids of our
rebalance/copygc/etc. threads in /sys/fs/bcachefs.

I'm liking it, do we have precedent elsewhere in the kernel for that?

[RESEND] Add sched_set_batch() and run bcachefs-rebalance under SCHED_BATCH

[Florian Schmaus]

Resending the patchset which first adds the sched_set_batch(),
akin to the existing sched_set_normal() and sched_set_fifo_low()
functions. In the second patch, we use sched_set_batch() to run
bcachefs' rebalance thread under SCHED_BATCH.

I am resending the patchset [1,2], since the two patches of the set
where not send to the same recipients. This probably caused some
confusion.

1: https://lore.kernel.org/lkml/20241114210649.71377-1-flo@geekplace.eu/
2: https://lore.kernel.org/lkml/20241114210649.71377-2-flo@geekplace.eu/

[PATCH 1/2] sched: provide sched_set_batch()

[Florian Schmaus]

This allows kernel threads created by modules to run under
SCHED_BATCH. Typically this may be a good option if the task of the
kernel thread is not sensitive to scheduling latency, for example
rebalancing or gc tasks.

Signed-off-by: Florian Schmaus <flo@geekplace.eu>
---
 include/linux/sched.h   |  1 +
 kernel/sched/syscalls.c | 10 ++++++++++
 2 files changed, 11 insertions(+)

diff --git a/include/linux/sched.h b/include/linux/sched.h
index 64934e0830af..80d46ed1dfa8 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1874,6 +1874,7 @@ extern int sched_setscheduler_nocheck(struct task_struct *, int, const struct sc
 extern void sched_set_fifo(struct task_struct *p);
 extern void sched_set_fifo_low(struct task_struct *p);
 extern void sched_set_normal(struct task_struct *p, int nice);
+extern void sched_set_batch(struct task_struct *p, int nice);
 extern int sched_setattr(struct task_struct *, const struct sched_attr *);
 extern int sched_setattr_nocheck(struct task_struct *, const struct sched_attr *);
 extern struct task_struct *idle_task(int cpu);
diff --git a/kernel/sched/syscalls.c b/kernel/sched/syscalls.c
index ff0e5ab4e37c..e0e0be4223df 100644
--- a/kernel/sched/syscalls.c
+++ b/kernel/sched/syscalls.c
@@ -880,6 +880,16 @@ void sched_set_normal(struct task_struct *p, int nice)
 }
 EXPORT_SYMBOL_GPL(sched_set_normal);
 
+void sched_set_batch(struct task_struct *p, int nice)
+{
+	struct sched_attr attr = {
+		.sched_policy = SCHED_BATCH,
+		.sched_nice = nice,
+	};
+	WARN_ON_ONCE(sched_setattr_nocheck(p, &attr) != 0);
+}
+EXPORT_SYMBOL_GPL(sched_set_batch);
+
 static int
 do_sched_setscheduler(pid_t pid, int policy, struct sched_param __user *param)
 {
-- 
2.45.2

[PATCH 2/2] bcachefs: set rebalance thread to SCHED_BATCH and nice 19

[Florian Schmaus]

While the rebalance thread is isually not compute bound, it does cause
a considerable amount of I/O. Since "reducing" the nice level from 0
to 19, also implicitly reduces the threads best-effort I/O scheduling
class level from 4 to 7, the reblance thread's I/O will be depriotized
over normal I/O.

Furthermore, we set the rebalance thread's scheduling class to BATCH,
which means that it will potentially receive a higher scheduling
latency. Making room for threads that need a low
schedulinglatency (e.g., interactive onces).

Signed-off-by: Florian Schmaus <flo@geekplace.eu>
---
 fs/bcachefs/rebalance.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/bcachefs/rebalance.c b/fs/bcachefs/rebalance.c
index cd6647374353..87e4f507af80 100644
--- a/fs/bcachefs/rebalance.c
+++ b/fs/bcachefs/rebalance.c
@@ -22,6 +22,7 @@
 
 #include <linux/freezer.h>
 #include <linux/kthread.h>
+#include <linux/sched.h>
 #include <linux/sched/cputime.h>
 
 #define REBALANCE_WORK_SCAN_OFFSET	(U64_MAX - 1)
@@ -478,6 +479,8 @@ int bch2_rebalance_start(struct bch_fs *c)
 	if (ret)
 		return ret;
 
+	sched_set_batch(p, 19);
+
 	get_task_struct(p);
 	rcu_assign_pointer(c->rebalance.thread, p);
 	wake_up_process(p);
-- 
2.45.2

Re: [PATCH 2/2] bcachefs: set rebalance thread to SCHED_BATCH and nice 19

[Christian Loehle]

On 1/14/25 12:47, Florian Schmaus wrote:
> While the rebalance thread is isually not compute bound, it does cause

s/isually/usually

> a considerable amount of I/O. Since "reducing" the nice level from 0
> to 19, also implicitly reduces the threads best-effort I/O scheduling
> class level from 4 to 7, the reblance thread's I/O will be depriotized

s/depriotized/deprioritized/

> over normal I/O.
> 
> Furthermore, we set the rebalance thread's scheduling class to BATCH,
> which means that it will potentially receive a higher scheduling
> latency. Making room for threads that need a low
> schedulinglatency (e.g., interactive onces).

s/schedulinglatency/
I know nothing about bcachefs internals, but could this also be a problem?
The rebalance thread might not run for O(second) or so? 

Re: [PATCH 2/2] bcachefs: set rebalance thread to SCHED_BATCH and nice 19

[Peter Zijlstra]

On Tue, Jan 14, 2025 at 01:47:28PM +0100, Florian Schmaus wrote:
> While the rebalance thread is isually not compute bound, it does cause
> a considerable amount of I/O. Since "reducing" the nice level from 0
> to 19, also implicitly reduces the threads best-effort I/O scheduling
> class level from 4 to 7, the reblance thread's I/O will be depriotized
> over normal I/O.
> 
> Furthermore, we set the rebalance thread's scheduling class to BATCH,
> which means that it will potentially receive a higher scheduling
> latency. Making room for threads that need a low
> schedulinglatency (e.g., interactive onces).

sorta.. what worries me most about these patches are the claims without
backing numbers.

Supposedly there is a problem, and this here fixes it, but it doesn't
really get quantified much here.

Re: [PATCH 2/2] bcachefs: set rebalance thread to SCHED_BATCH and nice 19

[Peter Zijlstra]

On Tue, Jan 14, 2025 at 01:29:04PM +0000, Christian Loehle wrote:

> I know nothing about bcachefs internals, but could this also be a problem?
> The rebalance thread might not run for O(second) or so? 

SCHED_BATCH should not behave anything like that, mostly SCHED_BATCH
tasks will not cause wakeup preemption. But otherwise they compete at
the same level as everybody else.

Notably a BATCH and NORMAL task that are each while(1) loops will get
the normal 50-50 distribution of time. It's just that when a NORMAL task
is running, the waking of a BATCH task won't ever kick the NORMAL from
the CPU, instead waiting for the tick to do so.

So a task that is IO heavy (as suggested here), that wakes a lot to
issue further IO, will not immediately interrupt whatever is on the CPU,
instead it waits until it gets selected through other means.

Re: [PATCH 2/2] bcachefs: set rebalance thread to SCHED_BATCH and nice 19

[Christian Loehle]

On 1/14/25 14:40, Peter Zijlstra wrote:
> On Tue, Jan 14, 2025 at 01:29:04PM +0000, Christian Loehle wrote:
> 
>> I know nothing about bcachefs internals, but could this also be a problem?
>> The rebalance thread might not run for O(second) or so? 
> 
> SCHED_BATCH should not behave anything like that, mostly SCHED_BATCH
> tasks will not cause wakeup preemption. But otherwise they compete at
> the same level as everybody else.
> 
> Notably a BATCH and NORMAL task that are each while(1) loops will get
> the normal 50-50 distribution of time. It's just that when a NORMAL task
> is running, the waking of a BATCH task won't ever kick the NORMAL from
> the CPU, instead waiting for the tick to do so.
> 
> So a task that is IO heavy (as suggested here), that wakes a lot to
> issue further IO, will not immediately interrupt whatever is on the CPU,
> instead it waits until it gets selected through other means.

I was thinking about two SCHED_BATCH tasks here and one having to wait
for a long time for it to complete, but that was because I was still
under the impression that SCHED_BATCH uses a different base slice which
apparently isn't true. My bad!

Re: [PATCH 2/2] bcachefs: set rebalance thread to SCHED_BATCH and nice 19

[Kent Overstreet]

On Tue, Jan 14, 2025 at 01:29:04PM +0000, Christian Loehle wrote:
> On 1/14/25 12:47, Florian Schmaus wrote:
> > While the rebalance thread is isually not compute bound, it does cause
> 
> s/isually/usually
> 
> > a considerable amount of I/O. Since "reducing" the nice level from 0
> > to 19, also implicitly reduces the threads best-effort I/O scheduling
> > class level from 4 to 7, the reblance thread's I/O will be depriotized
> 
> s/depriotized/deprioritized/
> 
> > over normal I/O.
> > 
> > Furthermore, we set the rebalance thread's scheduling class to BATCH,
> > which means that it will potentially receive a higher scheduling
> > latency. Making room for threads that need a low
> > schedulinglatency (e.g., interactive onces).
> 
> s/schedulinglatency/
> I know nothing about bcachefs internals, but could this also be a problem?
> The rebalance thread might not run for O(second) or so? 

Actually that is a concern - six locks don't have priority inheritence.

For that matter, standard mutexes and rw semaphores don't either, just
the RT variants, which seems questionable to me...

Re: [PATCH 2/2] bcachefs: set rebalance thread to SCHED_BATCH and nice 19

[Kent Overstreet]

On Tue, Jan 14, 2025 at 03:32:14PM +0100, Peter Zijlstra wrote:
> On Tue, Jan 14, 2025 at 01:47:28PM +0100, Florian Schmaus wrote:
> > While the rebalance thread is isually not compute bound, it does cause
> > a considerable amount of I/O. Since "reducing" the nice level from 0
> > to 19, also implicitly reduces the threads best-effort I/O scheduling
> > class level from 4 to 7, the reblance thread's I/O will be depriotized
> > over normal I/O.
> > 
> > Furthermore, we set the rebalance thread's scheduling class to BATCH,
> > which means that it will potentially receive a higher scheduling
> > latency. Making room for threads that need a low
> > schedulinglatency (e.g., interactive onces).
> 
> sorta.. what worries me most about these patches are the claims without
> backing numbers.
> 
> Supposedly there is a problem, and this here fixes it, but it doesn't
> really get quantified much here.

yeah, it was explained to me and made sense at the time, but things
somehow keep falling out of my overflowing brain.

Florian, could you update the patch message with that? Was it intended
as a partial workaround for the rebalance spinning issue some users have
been hitting?

Re: [PATCH 2/2] bcachefs: set rebalance thread to SCHED_BATCH and nice 19

[Kent Overstreet]

On Tue, Jan 14, 2025 at 03:40:00PM +0100, Peter Zijlstra wrote:
> On Tue, Jan 14, 2025 at 01:29:04PM +0000, Christian Loehle wrote:
> 
> > I know nothing about bcachefs internals, but could this also be a problem?
> > The rebalance thread might not run for O(second) or so? 
> 
> SCHED_BATCH should not behave anything like that, mostly SCHED_BATCH
> tasks will not cause wakeup preemption. But otherwise they compete at
> the same level as everybody else.
> 
> Notably a BATCH and NORMAL task that are each while(1) loops will get
> the normal 50-50 distribution of time. It's just that when a NORMAL task
> is running, the waking of a BATCH task won't ever kick the NORMAL from
> the CPU, instead waiting for the tick to do so.
> 
> So a task that is IO heavy (as suggested here), that wakes a lot to
> issue further IO, will not immediately interrupt whatever is on the CPU,
> instead it waits until it gets selected through other means.

Ok, thanks for the clarification on SCHED_BATCH - that does make perfect
sense here.

Using nice as a proxy for io scheduler priority does still bug me a bit,
simply because having a working io scheduler is not something we've ever
been able to depend on. That's a hidden internal knob we'd like to make
visible. I wonder if there's some way the tooling could be improved.

Re: [PATCH 2/2] bcachefs: set rebalance thread to SCHED_BATCH and nice 19

[Florian Schmaus]

[-- Attachment #1.1: Type: text/plain, Size: 2251 bytes --]

On 14/01/2025 16.25, Kent Overstreet wrote:
> On Tue, Jan 14, 2025 at 03:32:14PM +0100, Peter Zijlstra wrote:
>> On Tue, Jan 14, 2025 at 01:47:28PM +0100, Florian Schmaus wrote:
>>> While the rebalance thread is isually not compute bound, it does cause
>>> a considerable amount of I/O. Since "reducing" the nice level from 0
>>> to 19, also implicitly reduces the threads best-effort I/O scheduling
>>> class level from 4 to 7, the reblance thread's I/O will be depriotized
>>> over normal I/O.
>>>
>>> Furthermore, we set the rebalance thread's scheduling class to BATCH,
>>> which means that it will potentially receive a higher scheduling
>>> latency. Making room for threads that need a low
>>> schedulinglatency (e.g., interactive onces).
>>
>> sorta.. what worries me most about these patches are the claims without
>> backing numbers.
>>
>> Supposedly there is a problem, and this here fixes it, but it doesn't
>> really get quantified much here.

I am sorry, Peter; I know that changes should be motivated by some data, 
but I unfortunately don't have any in this case.

As you wrote, the difference between BATCH and NORMAL tasks is that the 
former will not immediately kick a running task from the CPU.

With that in mind, it made sense that janitorial tasks running in the 
background and not requiring a low scheduling latency should run under 
BATCH (and not NORMAL). Bcachefs' rebalance thread is a prime example of 
such a task.

Additionally, I believe, but please correct me if I am wrong, that tasks 
using BATCH instead of NORMAL grant the scheduler more flexibility to 
provide scheduling-latency-sensitive tasks with lower latency. But you 
are right, I should have made some experiments if this is really the case.


> yeah, it was explained to me and made sense at the time, but things
> somehow keep falling out of my overflowing brain.
> 
> Florian, could you update the patch message with that? Was it intended
> as a partial workaround for the rebalance spinning issue some users have
> been hitting?

I did not run into that issue myself, but it probably would help 
somewhat mitigate the effects of the periods during which the rebalance 
task is CPU bound.

- Florian


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 618 bytes --]

Re: [PATCH 2/2] bcachefs: set rebalance thread to SCHED_BATCH and nice 19

[Florian Schmaus]

[-- Attachment #1.1: Type: text/plain, Size: 1228 bytes --]

On 14/01/2025 14.29, Christian Loehle wrote:
> On 1/14/25 12:47, Florian Schmaus wrote:
>> While the rebalance thread is isually not compute bound, it does cause
> 
> s/isually/usually
> 
>> a considerable amount of I/O. Since "reducing" the nice level from 0
>> to 19, also implicitly reduces the threads best-effort I/O scheduling
>> class level from 4 to 7, the reblance thread's I/O will be depriotized
> 
> s/depriotized/deprioritized/
> 
>> over normal I/O.
>>
>> Furthermore, we set the rebalance thread's scheduling class to BATCH,
>> which means that it will potentially receive a higher scheduling
>> latency. Making room for threads that need a low
>> schedulinglatency (e.g., interactive onces).
> 
> s/schedulinglatency/
> I know nothing about bcachefs internals, but could this also be a problem?
> The rebalance thread might not run for O(second) or so?

Thanks for the review, much appreciated. I have adjusted the commit 
message in my branch [1].

Bcachefs rebalance task is not really scheduling latency sensitive, 
unlike bcachefs' copygc task, so it is fine to run it under BATCH.

- Florian

1: 
https://github.com/Flowdalic/linux/tree/bcachefs-rebalance-set-sched-batch


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 618 bytes --]