* [RFC][PATCH] dm: Remove dm_bufio_cond_resched()
@ 2016-09-13 8:45 Peter Zijlstra
2016-09-13 12:49 ` Thomas Gleixner
` (2 more replies)
0 siblings, 3 replies; 20+ messages in thread
From: Peter Zijlstra @ 2016-09-13 8:45 UTC (permalink / raw)
To: Alasdair Kergon, Mike Snitzer, Thomas Gleixner, Ingo Molnar
Cc: Mikulas Patocka, linux-kernel, dm-devel
Hi all,
While grepping for PREEMPT_VOLUNTARY I ran into dm_bufio_cond_resched()
and wondered WTH it was about.
Is there anything wrong with the below patch?
---
diff --git a/drivers/md/dm-bufio.c b/drivers/md/dm-bufio.c
index 8625040bae92..125aedc3875f 100644
--- a/drivers/md/dm-bufio.c
+++ b/drivers/md/dm-bufio.c
@@ -191,19 +191,6 @@ static void dm_bufio_unlock(struct dm_bufio_client *c)
mutex_unlock(&c->lock);
}
-/*
- * FIXME Move to sched.h?
- */
-#ifdef CONFIG_PREEMPT_VOLUNTARY
-# define dm_bufio_cond_resched() \
-do { \
- if (unlikely(need_resched())) \
- _cond_resched(); \
-} while (0)
-#else
-# define dm_bufio_cond_resched() do { } while (0)
-#endif
-
/*----------------------------------------------------------------*/
/*
@@ -741,7 +728,7 @@ static void __flush_write_list(struct list_head *write_list)
list_entry(write_list->next, struct dm_buffer, write_list);
list_del(&b->write_list);
submit_io(b, WRITE, b->block, write_endio);
- dm_bufio_cond_resched();
+ cond_resched();
}
blk_finish_plug(&plug);
}
@@ -780,7 +767,7 @@ static struct dm_buffer *__get_unclaimed_buffer(struct dm_bufio_client *c)
__unlink_buffer(b);
return b;
}
- dm_bufio_cond_resched();
+ cond_resched();
}
list_for_each_entry_reverse(b, &c->lru[LIST_DIRTY], lru_list) {
@@ -791,7 +778,7 @@ static struct dm_buffer *__get_unclaimed_buffer(struct dm_bufio_client *c)
__unlink_buffer(b);
return b;
}
- dm_bufio_cond_resched();
+ cond_resched();
}
return NULL;
@@ -923,7 +910,7 @@ static void __write_dirty_buffers_async(struct dm_bufio_client *c, int no_wait,
return;
__write_dirty_buffer(b, write_list);
- dm_bufio_cond_resched();
+ cond_resched();
}
}
@@ -973,7 +960,7 @@ static void __check_watermark(struct dm_bufio_client *c,
return;
__free_buffer_wake(b);
- dm_bufio_cond_resched();
+ cond_resched();
}
if (c->n_buffers[LIST_DIRTY] > threshold_buffers)
@@ -1170,7 +1157,7 @@ void dm_bufio_prefetch(struct dm_bufio_client *c,
submit_io(b, READ, b->block, read_endio);
dm_bufio_release(b);
- dm_bufio_cond_resched();
+ cond_resched();
if (!n_blocks)
goto flush_plug;
@@ -1291,7 +1278,7 @@ int dm_bufio_write_dirty_buffers(struct dm_bufio_client *c)
!test_bit(B_WRITING, &b->state))
__relink_lru(b, LIST_CLEAN);
- dm_bufio_cond_resched();
+ cond_resched();
/*
* If we dropped the lock, the list is no longer consistent,
@@ -1574,7 +1561,7 @@ static unsigned long __scan(struct dm_bufio_client *c, unsigned long nr_to_scan,
freed++;
if (!--nr_to_scan || ((count - freed) <= retain_target))
return freed;
- dm_bufio_cond_resched();
+ cond_resched();
}
}
return freed;
@@ -1808,7 +1795,7 @@ static void __evict_old_buffers(struct dm_bufio_client *c, unsigned long age_hz)
if (__try_evict_buffer(b, 0))
count--;
- dm_bufio_cond_resched();
+ cond_resched();
}
dm_bufio_unlock(c);
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [RFC][PATCH] dm: Remove dm_bufio_cond_resched()
2016-09-13 8:45 [RFC][PATCH] dm: Remove dm_bufio_cond_resched() Peter Zijlstra
@ 2016-09-13 12:49 ` Thomas Gleixner
2016-09-13 13:39 ` Mike Snitzer
2016-09-19 9:49 ` Mikulas Patocka
2 siblings, 0 replies; 20+ messages in thread
From: Thomas Gleixner @ 2016-09-13 12:49 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Alasdair Kergon, Mike Snitzer, Ingo Molnar, Mikulas Patocka,
linux-kernel, dm-devel
On Tue, 13 Sep 2016, Peter Zijlstra wrote:
> Hi all,
>
> While grepping for PREEMPT_VOLUNTARY I ran into dm_bufio_cond_resched()
> and wondered WTH it was about.
>
> Is there anything wrong with the below patch?
Not at all, except that you forgot to add your SOB to it :)
Acked-by: Thomas Gleixner <tglx@linutronix.de>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFC][PATCH] dm: Remove dm_bufio_cond_resched()
2016-09-13 8:45 [RFC][PATCH] dm: Remove dm_bufio_cond_resched() Peter Zijlstra
2016-09-13 12:49 ` Thomas Gleixner
@ 2016-09-13 13:39 ` Mike Snitzer
2016-09-19 10:53 ` Peter Zijlstra
2016-09-19 9:49 ` Mikulas Patocka
2 siblings, 1 reply; 20+ messages in thread
From: Mike Snitzer @ 2016-09-13 13:39 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Alasdair Kergon, Thomas Gleixner, Ingo Molnar, Mikulas Patocka,
linux-kernel, dm-devel, Joe Thornber
On Tue, Sep 13 2016 at 4:45am -0400,
Peter Zijlstra <peterz@infradead.org> wrote:
> Hi all,
>
> While grepping for PREEMPT_VOLUNTARY I ran into dm_bufio_cond_resched()
> and wondered WTH it was about.
>
> Is there anything wrong with the below patch?
No, I'll pick it up for 4.9 merge. Mikulas added it for sparc or
something. I cannot recall _the_ reason (I wasn't maintaining DM back
then) but at the time both Alasdair and Joe Thornber reasoned that it
needed to go -- and that if it was really needed that it should be done
in terms of a proper patch to sched.h, etc.
So I'm not sure how this dm-bufio local cond_resched() wrapper still got
in... happy to take your patch.
Please respond with whatever SOB you'd like applied to the patch header.
Thanks,
Mike
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFC][PATCH] dm: Remove dm_bufio_cond_resched()
2016-09-13 13:39 ` Mike Snitzer
@ 2016-09-19 10:53 ` Peter Zijlstra
2016-09-22 20:53 ` Mikulas Patocka
0 siblings, 1 reply; 20+ messages in thread
From: Peter Zijlstra @ 2016-09-19 10:53 UTC (permalink / raw)
To: Mike Snitzer
Cc: Alasdair Kergon, Thomas Gleixner, Ingo Molnar, Mikulas Patocka,
linux-kernel, dm-devel, Joe Thornber
On Tue, Sep 13, 2016 at 09:39:59AM -0400, Mike Snitzer wrote:
> So I'm not sure how this dm-bufio local cond_resched() wrapper still got
> in... happy to take your patch.
>
> Please respond with whatever SOB you'd like applied to the patch header.
Sorry, for the delay, here goes.
---
Subject: dm: Remove dm_bufio_cond_resched()
From: Peter Zijlstra <peterz@infradead.org>
Date: Tue, 13 Sep 2016 10:45:20 +0200
Remove pointless local wrappery. Use cond_resched() like everybody else.
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Mikulas Patocka <mpatocka@redhat.com>
Cc: Mike Snitzer <snitzer@redhat.com>
Cc: Alasdair Kergon <agk@redhat.com>
Acked-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
drivers/md/dm-bufio.c | 31 +++++++++----------------------
1 file changed, 9 insertions(+), 22 deletions(-)
--- a/drivers/md/dm-bufio.c
+++ b/drivers/md/dm-bufio.c
@@ -191,19 +191,6 @@ static void dm_bufio_unlock(struct dm_bu
mutex_unlock(&c->lock);
}
-/*
- * FIXME Move to sched.h?
- */
-#ifdef CONFIG_PREEMPT_VOLUNTARY
-# define dm_bufio_cond_resched() \
-do { \
- if (unlikely(need_resched())) \
- _cond_resched(); \
-} while (0)
-#else
-# define dm_bufio_cond_resched() do { } while (0)
-#endif
-
/*----------------------------------------------------------------*/
/*
@@ -741,7 +728,7 @@ static void __flush_write_list(struct li
list_entry(write_list->next, struct dm_buffer, write_list);
list_del(&b->write_list);
submit_io(b, WRITE, b->block, write_endio);
- dm_bufio_cond_resched();
+ cond_resched();
}
blk_finish_plug(&plug);
}
@@ -780,7 +767,7 @@ static struct dm_buffer *__get_unclaimed
__unlink_buffer(b);
return b;
}
- dm_bufio_cond_resched();
+ cond_resched();
}
list_for_each_entry_reverse(b, &c->lru[LIST_DIRTY], lru_list) {
@@ -791,7 +778,7 @@ static struct dm_buffer *__get_unclaimed
__unlink_buffer(b);
return b;
}
- dm_bufio_cond_resched();
+ cond_resched();
}
return NULL;
@@ -923,7 +910,7 @@ static void __write_dirty_buffers_async(
return;
__write_dirty_buffer(b, write_list);
- dm_bufio_cond_resched();
+ cond_resched();
}
}
@@ -973,7 +960,7 @@ static void __check_watermark(struct dm_
return;
__free_buffer_wake(b);
- dm_bufio_cond_resched();
+ cond_resched();
}
if (c->n_buffers[LIST_DIRTY] > threshold_buffers)
@@ -1170,7 +1157,7 @@ void dm_bufio_prefetch(struct dm_bufio_c
submit_io(b, READ, b->block, read_endio);
dm_bufio_release(b);
- dm_bufio_cond_resched();
+ cond_resched();
if (!n_blocks)
goto flush_plug;
@@ -1291,7 +1278,7 @@ int dm_bufio_write_dirty_buffers(struct
!test_bit(B_WRITING, &b->state))
__relink_lru(b, LIST_CLEAN);
- dm_bufio_cond_resched();
+ cond_resched();
/*
* If we dropped the lock, the list is no longer consistent,
@@ -1574,7 +1561,7 @@ static unsigned long __scan(struct dm_bu
freed++;
if (!--nr_to_scan || ((count - freed) <= retain_target))
return freed;
- dm_bufio_cond_resched();
+ cond_resched();
}
}
return freed;
@@ -1808,7 +1795,7 @@ static void __evict_old_buffers(struct d
if (__try_evict_buffer(b, 0))
count--;
- dm_bufio_cond_resched();
+ cond_resched();
}
dm_bufio_unlock(c);
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFC][PATCH] dm: Remove dm_bufio_cond_resched()
2016-09-19 10:53 ` Peter Zijlstra
@ 2016-09-22 20:53 ` Mikulas Patocka
2016-09-22 20:59 ` Thomas Gleixner
0 siblings, 1 reply; 20+ messages in thread
From: Mikulas Patocka @ 2016-09-22 20:53 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Mike Snitzer, Alasdair Kergon, Thomas Gleixner, Ingo Molnar,
linux-kernel, dm-devel, Joe Thornber
On Mon, 19 Sep 2016, Peter Zijlstra wrote:
> On Tue, Sep 13, 2016 at 09:39:59AM -0400, Mike Snitzer wrote:
> > So I'm not sure how this dm-bufio local cond_resched() wrapper still got
> > in... happy to take your patch.
> >
> > Please respond with whatever SOB you'd like applied to the patch header.
>
> Sorry, for the delay, here goes.
Why not change it to might_sleep()? - that would be almost equivalent to
the code that was there before (i.e. reschedule only if
CONFIG_PREEMPT_VOLUNTARY is set).
If we call the cond_resched() function in tight loops such as walking all
buffers in a list, there may be performance penalty due to the call, so
the call should be done only if it is really needed (i.e. in
CONFIG_PREEMPT_VOLUNTARY case).
Mikulas
> ---
> Subject: dm: Remove dm_bufio_cond_resched()
> From: Peter Zijlstra <peterz@infradead.org>
> Date: Tue, 13 Sep 2016 10:45:20 +0200
>
> Remove pointless local wrappery. Use cond_resched() like everybody else.
>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: Mikulas Patocka <mpatocka@redhat.com>
> Cc: Mike Snitzer <snitzer@redhat.com>
> Cc: Alasdair Kergon <agk@redhat.com>
> Acked-by: Thomas Gleixner <tglx@linutronix.de>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
> drivers/md/dm-bufio.c | 31 +++++++++----------------------
> 1 file changed, 9 insertions(+), 22 deletions(-)
>
> --- a/drivers/md/dm-bufio.c
> +++ b/drivers/md/dm-bufio.c
> @@ -191,19 +191,6 @@ static void dm_bufio_unlock(struct dm_bu
> mutex_unlock(&c->lock);
> }
>
> -/*
> - * FIXME Move to sched.h?
> - */
> -#ifdef CONFIG_PREEMPT_VOLUNTARY
> -# define dm_bufio_cond_resched() \
> -do { \
> - if (unlikely(need_resched())) \
> - _cond_resched(); \
> -} while (0)
> -#else
> -# define dm_bufio_cond_resched() do { } while (0)
> -#endif
> -
> /*----------------------------------------------------------------*/
>
> /*
> @@ -741,7 +728,7 @@ static void __flush_write_list(struct li
> list_entry(write_list->next, struct dm_buffer, write_list);
> list_del(&b->write_list);
> submit_io(b, WRITE, b->block, write_endio);
> - dm_bufio_cond_resched();
> + cond_resched();
> }
> blk_finish_plug(&plug);
> }
> @@ -780,7 +767,7 @@ static struct dm_buffer *__get_unclaimed
> __unlink_buffer(b);
> return b;
> }
> - dm_bufio_cond_resched();
> + cond_resched();
> }
>
> list_for_each_entry_reverse(b, &c->lru[LIST_DIRTY], lru_list) {
> @@ -791,7 +778,7 @@ static struct dm_buffer *__get_unclaimed
> __unlink_buffer(b);
> return b;
> }
> - dm_bufio_cond_resched();
> + cond_resched();
> }
>
> return NULL;
> @@ -923,7 +910,7 @@ static void __write_dirty_buffers_async(
> return;
>
> __write_dirty_buffer(b, write_list);
> - dm_bufio_cond_resched();
> + cond_resched();
> }
> }
>
> @@ -973,7 +960,7 @@ static void __check_watermark(struct dm_
> return;
>
> __free_buffer_wake(b);
> - dm_bufio_cond_resched();
> + cond_resched();
> }
>
> if (c->n_buffers[LIST_DIRTY] > threshold_buffers)
> @@ -1170,7 +1157,7 @@ void dm_bufio_prefetch(struct dm_bufio_c
> submit_io(b, READ, b->block, read_endio);
> dm_bufio_release(b);
>
> - dm_bufio_cond_resched();
> + cond_resched();
>
> if (!n_blocks)
> goto flush_plug;
> @@ -1291,7 +1278,7 @@ int dm_bufio_write_dirty_buffers(struct
> !test_bit(B_WRITING, &b->state))
> __relink_lru(b, LIST_CLEAN);
>
> - dm_bufio_cond_resched();
> + cond_resched();
>
> /*
> * If we dropped the lock, the list is no longer consistent,
> @@ -1574,7 +1561,7 @@ static unsigned long __scan(struct dm_bu
> freed++;
> if (!--nr_to_scan || ((count - freed) <= retain_target))
> return freed;
> - dm_bufio_cond_resched();
> + cond_resched();
> }
> }
> return freed;
> @@ -1808,7 +1795,7 @@ static void __evict_old_buffers(struct d
> if (__try_evict_buffer(b, 0))
> count--;
>
> - dm_bufio_cond_resched();
> + cond_resched();
> }
>
> dm_bufio_unlock(c);
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFC][PATCH] dm: Remove dm_bufio_cond_resched()
2016-09-22 20:53 ` Mikulas Patocka
@ 2016-09-22 20:59 ` Thomas Gleixner
2016-09-23 7:34 ` Peter Zijlstra
0 siblings, 1 reply; 20+ messages in thread
From: Thomas Gleixner @ 2016-09-22 20:59 UTC (permalink / raw)
To: Mikulas Patocka
Cc: Peter Zijlstra, Mike Snitzer, Alasdair Kergon, Ingo Molnar,
linux-kernel, dm-devel, Joe Thornber
On Thu, 22 Sep 2016, Mikulas Patocka wrote:
> On Mon, 19 Sep 2016, Peter Zijlstra wrote:
>
> > On Tue, Sep 13, 2016 at 09:39:59AM -0400, Mike Snitzer wrote:
> > > So I'm not sure how this dm-bufio local cond_resched() wrapper still got
> > > in... happy to take your patch.
> > >
> > > Please respond with whatever SOB you'd like applied to the patch header.
> >
> > Sorry, for the delay, here goes.
>
> Why not change it to might_sleep()? - that would be almost equivalent to
You mean might_resched(). might_sleep() is not even remotely equivalent.
> If we call the cond_resched() function in tight loops such as walking all
> buffers in a list, there may be performance penalty due to the call, so
> the call should be done only if it is really needed (i.e. in
> CONFIG_PREEMPT_VOLUNTARY case).
Makes sense.
Thanks,
tglx
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFC][PATCH] dm: Remove dm_bufio_cond_resched()
2016-09-22 20:59 ` Thomas Gleixner
@ 2016-09-23 7:34 ` Peter Zijlstra
2016-09-23 8:00 ` Thomas Gleixner
2016-09-23 14:32 ` Bart Van Assche
0 siblings, 2 replies; 20+ messages in thread
From: Peter Zijlstra @ 2016-09-23 7:34 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Mikulas Patocka, Mike Snitzer, Alasdair Kergon, Ingo Molnar,
linux-kernel, dm-devel, Joe Thornber
On Thu, Sep 22, 2016 at 10:59:30PM +0200, Thomas Gleixner wrote:
> On Thu, 22 Sep 2016, Mikulas Patocka wrote:
> > On Mon, 19 Sep 2016, Peter Zijlstra wrote:
> >
> > > On Tue, Sep 13, 2016 at 09:39:59AM -0400, Mike Snitzer wrote:
> > > > So I'm not sure how this dm-bufio local cond_resched() wrapper still got
> > > > in... happy to take your patch.
> > > >
> > > > Please respond with whatever SOB you'd like applied to the patch header.
> > >
> > > Sorry, for the delay, here goes.
> >
> > Why not change it to might_sleep()? - that would be almost equivalent to
>
> You mean might_resched(). might_sleep() is not even remotely equivalent.
It is, might_sleep() implies might_resched(). In fact, that's all what
PREEMPT_VOLUNTARY is, make the might_sleep() debug test imply a resched
point.
> > If we call the cond_resched() function in tight loops such as walking all
> > buffers in a list, there may be performance penalty due to the call, so
> > the call should be done only if it is really needed (i.e. in
> > CONFIG_PREEMPT_VOLUNTARY case).
>
> Makes sense.
Is anybody still using PREEMPT_NONE? Most workloads also care about
latency to some extend. Lots of code has explicit cond_resched() and
doesn't worry.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFC][PATCH] dm: Remove dm_bufio_cond_resched()
2016-09-23 7:34 ` Peter Zijlstra
@ 2016-09-23 8:00 ` Thomas Gleixner
2016-09-23 9:05 ` Peter Zijlstra
2016-09-23 12:17 ` Mike Galbraith
2016-09-23 14:32 ` Bart Van Assche
1 sibling, 2 replies; 20+ messages in thread
From: Thomas Gleixner @ 2016-09-23 8:00 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Mikulas Patocka, Mike Snitzer, Alasdair Kergon, Ingo Molnar,
linux-kernel, dm-devel, Joe Thornber
On Fri, 23 Sep 2016, Peter Zijlstra wrote:
> On Thu, Sep 22, 2016 at 10:59:30PM +0200, Thomas Gleixner wrote:
> > On Thu, 22 Sep 2016, Mikulas Patocka wrote:
> > > On Mon, 19 Sep 2016, Peter Zijlstra wrote:
> > >
> > > > On Tue, Sep 13, 2016 at 09:39:59AM -0400, Mike Snitzer wrote:
> > > > > So I'm not sure how this dm-bufio local cond_resched() wrapper still got
> > > > > in... happy to take your patch.
> > > > >
> > > > > Please respond with whatever SOB you'd like applied to the patch header.
> > > >
> > > > Sorry, for the delay, here goes.
> > >
> > > Why not change it to might_sleep()? - that would be almost equivalent to
> >
> > You mean might_resched(). might_sleep() is not even remotely equivalent.
>
> It is, might_sleep() implies might_resched(). In fact, that's all what
> PREEMPT_VOLUNTARY is, make the might_sleep() debug test imply a resched
> point.
Grr, how intuitive - NOT!
> > > If we call the cond_resched() function in tight loops such as walking all
> > > buffers in a list, there may be performance penalty due to the call, so
> > > the call should be done only if it is really needed (i.e. in
> > > CONFIG_PREEMPT_VOLUNTARY case).
> >
> > Makes sense.
>
> Is anybody still using PREEMPT_NONE? Most workloads also care about
> latency to some extend. Lots of code has explicit cond_resched() and
> doesn't worry.
Dunno. But I bet there are workloads which love it.
Thanks,
tglx
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFC][PATCH] dm: Remove dm_bufio_cond_resched()
2016-09-23 8:00 ` Thomas Gleixner
@ 2016-09-23 9:05 ` Peter Zijlstra
2016-09-23 9:13 ` Thomas Gleixner
2016-09-23 12:17 ` Mike Galbraith
1 sibling, 1 reply; 20+ messages in thread
From: Peter Zijlstra @ 2016-09-23 9:05 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Mikulas Patocka, Mike Snitzer, Alasdair Kergon, Ingo Molnar,
linux-kernel, dm-devel, Joe Thornber
On Fri, Sep 23, 2016 at 10:00:37AM +0200, Thomas Gleixner wrote:
> On Fri, 23 Sep 2016, Peter Zijlstra wrote:
> > It is, might_sleep() implies might_resched(). In fact, that's all what
> > PREEMPT_VOLUNTARY is, make the might_sleep() debug test imply a resched
> > point.
>
> Grr, how intuitive - NOT!
No, it actually makes sense. Because you 'obviously' only call
might_sleep() in contexts that should be able to sleep (if not, it'll
holler). So they're already placed right for preemption.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFC][PATCH] dm: Remove dm_bufio_cond_resched()
2016-09-23 9:05 ` Peter Zijlstra
@ 2016-09-23 9:13 ` Thomas Gleixner
2016-09-23 9:26 ` Ingo Molnar
0 siblings, 1 reply; 20+ messages in thread
From: Thomas Gleixner @ 2016-09-23 9:13 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Mikulas Patocka, Mike Snitzer, Alasdair Kergon, Ingo Molnar,
linux-kernel, dm-devel, Joe Thornber
On Fri, 23 Sep 2016, Peter Zijlstra wrote:
> On Fri, Sep 23, 2016 at 10:00:37AM +0200, Thomas Gleixner wrote:
> > On Fri, 23 Sep 2016, Peter Zijlstra wrote:
> > > It is, might_sleep() implies might_resched(). In fact, that's all what
> > > PREEMPT_VOLUNTARY is, make the might_sleep() debug test imply a resched
> > > point.
> >
> > Grr, how intuitive - NOT!
>
> No, it actually makes sense. Because you 'obviously' only call
> might_sleep() in contexts that should be able to sleep (if not, it'll
> holler). So they're already placed right for preemption.
I disagree. might_sleep() is commonly known as a debug mechanism and it
existed before the preemption stuff went in. So the easy way to sprinkle
preemption points into the kernel was to hijack might_sleep(). I know it's
historical, but that doesnt make it any more intuitive.
Thanks,
tglx
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFC][PATCH] dm: Remove dm_bufio_cond_resched()
2016-09-23 9:13 ` Thomas Gleixner
@ 2016-09-23 9:26 ` Ingo Molnar
0 siblings, 0 replies; 20+ messages in thread
From: Ingo Molnar @ 2016-09-23 9:26 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Peter Zijlstra, Mikulas Patocka, Mike Snitzer, Alasdair Kergon,
linux-kernel, dm-devel, Joe Thornber
* Thomas Gleixner <tglx@linutronix.de> wrote:
> On Fri, 23 Sep 2016, Peter Zijlstra wrote:
> > On Fri, Sep 23, 2016 at 10:00:37AM +0200, Thomas Gleixner wrote:
> > > On Fri, 23 Sep 2016, Peter Zijlstra wrote:
> > > > It is, might_sleep() implies might_resched(). In fact, that's all what
> > > > PREEMPT_VOLUNTARY is, make the might_sleep() debug test imply a resched
> > > > point.
> > >
> > > Grr, how intuitive - NOT!
> >
> > No, it actually makes sense. Because you 'obviously' only call
> > might_sleep() in contexts that should be able to sleep (if not, it'll
> > holler). So they're already placed right for preemption.
>
> I disagree. might_sleep() is commonly known as a debug mechanism and it
> existed before the preemption stuff went in. So the easy way to sprinkle
> preemption points into the kernel was to hijack might_sleep(). I know it's
> historical, but that doesnt make it any more intuitive.
If we rename it to might_as_well_sleep() it becomes more intuitive! ;-)
Thanks,
Ingo
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFC][PATCH] dm: Remove dm_bufio_cond_resched()
2016-09-23 8:00 ` Thomas Gleixner
2016-09-23 9:05 ` Peter Zijlstra
@ 2016-09-23 12:17 ` Mike Galbraith
2016-09-23 12:26 ` Peter Zijlstra
1 sibling, 1 reply; 20+ messages in thread
From: Mike Galbraith @ 2016-09-23 12:17 UTC (permalink / raw)
To: Thomas Gleixner, Peter Zijlstra
Cc: Mikulas Patocka, Mike Snitzer, Alasdair Kergon, Ingo Molnar,
linux-kernel, dm-devel, Joe Thornber
On Fri, 2016-09-23 at 10:00 +0200, Thomas Gleixner wrote:
> On Fri, 23 Sep 2016, Peter Zijlstra wrote:
> > Is anybody still using PREEMPT_NONE? Most workloads also care about
> > latency to some extend. Lots of code has explicit cond_resched() and
> > doesn't worry.
>
> Dunno. But I bet there are workloads which love it.
SUSE definitely uses it. I had presumed that was enterprise standard.
-Mike
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFC][PATCH] dm: Remove dm_bufio_cond_resched()
2016-09-23 12:17 ` Mike Galbraith
@ 2016-09-23 12:26 ` Peter Zijlstra
2016-09-23 12:39 ` Mike Galbraith
2016-09-23 12:42 ` Mike Snitzer
0 siblings, 2 replies; 20+ messages in thread
From: Peter Zijlstra @ 2016-09-23 12:26 UTC (permalink / raw)
To: Mike Galbraith
Cc: Thomas Gleixner, Mikulas Patocka, Mike Snitzer, Alasdair Kergon,
Ingo Molnar, linux-kernel, dm-devel, Joe Thornber
On Fri, Sep 23, 2016 at 02:17:10PM +0200, Mike Galbraith wrote:
> On Fri, 2016-09-23 at 10:00 +0200, Thomas Gleixner wrote:
> > On Fri, 23 Sep 2016, Peter Zijlstra wrote:
>
> > > Is anybody still using PREEMPT_NONE? Most workloads also care about
> > > latency to some extend. Lots of code has explicit cond_resched() and
> > > doesn't worry.
> >
> > Dunno. But I bet there are workloads which love it.
>
> SUSE definitely uses it. I had presumed that was enterprise standard.
Hmm, I thought most distros defaulted to PREEMPT_VOLUNTARY.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFC][PATCH] dm: Remove dm_bufio_cond_resched()
2016-09-23 12:26 ` Peter Zijlstra
@ 2016-09-23 12:39 ` Mike Galbraith
2016-09-23 12:42 ` Mike Snitzer
1 sibling, 0 replies; 20+ messages in thread
From: Mike Galbraith @ 2016-09-23 12:39 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Thomas Gleixner, Mikulas Patocka, Mike Snitzer, Alasdair Kergon,
Ingo Molnar, linux-kernel, dm-devel, Joe Thornber
On Fri, 2016-09-23 at 14:26 +0200, Peter Zijlstra wrote:
> On Fri, Sep 23, 2016 at 02:17:10PM +0200, Mike Galbraith wrote:
> > On Fri, 2016-09-23 at 10:00 +0200, Thomas Gleixner wrote:
> > > On Fri, 23 Sep 2016, Peter Zijlstra wrote:
> >
> > > > Is anybody still using PREEMPT_NONE? Most workloads also care
> > > > about
> > > > latency to some extend. Lots of code has explicit
> > > > cond_resched() and
> > > > doesn't worry.
> > >
> > > Dunno. But I bet there are workloads which love it.
> >
> > SUSE definitely uses it. I had presumed that was enterprise
> > standard.
>
> Hmm, I thought most distros defaulted to PREEMPT_VOLUNTARY.
I use PREEMPT_VOLUNTARY for my desktop, that offering much better
performance than the PREEMPT desktop targeted kernels (ick), but
workhorses run PREEMPT_NONE for maximum throughput.
-Mike
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFC][PATCH] dm: Remove dm_bufio_cond_resched()
2016-09-23 12:26 ` Peter Zijlstra
2016-09-23 12:39 ` Mike Galbraith
@ 2016-09-23 12:42 ` Mike Snitzer
2016-09-23 12:46 ` Peter Zijlstra
1 sibling, 1 reply; 20+ messages in thread
From: Mike Snitzer @ 2016-09-23 12:42 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Mike Galbraith, Thomas Gleixner, Mikulas Patocka,
Alasdair Kergon, Ingo Molnar, linux-kernel, dm-devel,
Joe Thornber
On Fri, Sep 23 2016 at 8:26am -0400,
Peter Zijlstra <peterz@infradead.org> wrote:
> On Fri, Sep 23, 2016 at 02:17:10PM +0200, Mike Galbraith wrote:
> > On Fri, 2016-09-23 at 10:00 +0200, Thomas Gleixner wrote:
> > > On Fri, 23 Sep 2016, Peter Zijlstra wrote:
> >
> > > > Is anybody still using PREEMPT_NONE? Most workloads also care about
> > > > latency to some extend. Lots of code has explicit cond_resched() and
> > > > doesn't worry.
> > >
> > > Dunno. But I bet there are workloads which love it.
> >
> > SUSE definitely uses it. I had presumed that was enterprise standard.
>
> Hmm, I thought most distros defaulted to PREEMPT_VOLUNTARY.
So what is the concensus on this? Switch dm-bufio's cond_resched calls
(in peter's patch) to might_sleep()? Or continue using cond_resched but
fix cond_resched to do the might_sleep() equivalent if PREEMPT_NONE?
Mike
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFC][PATCH] dm: Remove dm_bufio_cond_resched()
2016-09-23 12:42 ` Mike Snitzer
@ 2016-09-23 12:46 ` Peter Zijlstra
0 siblings, 0 replies; 20+ messages in thread
From: Peter Zijlstra @ 2016-09-23 12:46 UTC (permalink / raw)
To: Mike Snitzer
Cc: Mike Galbraith, Thomas Gleixner, Mikulas Patocka,
Alasdair Kergon, Ingo Molnar, linux-kernel, dm-devel,
Joe Thornber
On Fri, Sep 23, 2016 at 08:42:51AM -0400, Mike Snitzer wrote:
> On Fri, Sep 23 2016 at 8:26am -0400,
> Peter Zijlstra <peterz@infradead.org> wrote:
>
> > On Fri, Sep 23, 2016 at 02:17:10PM +0200, Mike Galbraith wrote:
> > > On Fri, 2016-09-23 at 10:00 +0200, Thomas Gleixner wrote:
> > > > On Fri, 23 Sep 2016, Peter Zijlstra wrote:
> > >
> > > > > Is anybody still using PREEMPT_NONE? Most workloads also care about
> > > > > latency to some extend. Lots of code has explicit cond_resched() and
> > > > > doesn't worry.
> > > >
> > > > Dunno. But I bet there are workloads which love it.
> > >
> > > SUSE definitely uses it. I had presumed that was enterprise standard.
> >
> > Hmm, I thought most distros defaulted to PREEMPT_VOLUNTARY.
>
> So what is the concensus on this? Switch dm-bufio's cond_resched calls
> (in peter's patch) to might_sleep()? Or continue using cond_resched but
> fix cond_resched to do the might_sleep() equivalent if PREEMPT_NONE?
I'd go with the one I posted and look again if ever a performance issue
shows up.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [dm-devel] [RFC][PATCH] dm: Remove dm_bufio_cond_resched()
2016-09-23 7:34 ` Peter Zijlstra
@ 2016-09-23 14:32 ` Bart Van Assche
2016-09-23 14:32 ` Bart Van Assche
1 sibling, 0 replies; 20+ messages in thread
From: Bart Van Assche @ 2016-09-23 14:32 UTC (permalink / raw)
To: Peter Zijlstra, Thomas Gleixner
Cc: Mike Snitzer, Joe Thornber, linux-kernel, dm-devel,
Mikulas Patocka, Ingo Molnar, Alasdair Kergon
On 09/23/16 00:34, Peter Zijlstra wrote:
> Is anybody still using PREEMPT_NONE? Most workloads also care about
> latency to some extend. Lots of code has explicit cond_resched() and
> doesn't worry.
From a SLES11 system:
$ grep PREEMPT_NONE /boot/config-3.0.101-0.47.67-default
CONFIG_PREEMPT_NONE=y
From a SLES12 system:
$ grep CONFIG_PREEMPT_NONE /boot/config-4.4.16-56-default
CONFIG_PREEMPT_NONE=y
Bart.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [dm-devel] [RFC][PATCH] dm: Remove dm_bufio_cond_resched()
@ 2016-09-23 14:32 ` Bart Van Assche
0 siblings, 0 replies; 20+ messages in thread
From: Bart Van Assche @ 2016-09-23 14:32 UTC (permalink / raw)
To: Peter Zijlstra, Thomas Gleixner
Cc: Mike Snitzer, Joe Thornber, linux-kernel, dm-devel,
Mikulas Patocka, Ingo Molnar, Alasdair Kergon
On 09/23/16 00:34, Peter Zijlstra wrote:
> Is anybody still using PREEMPT_NONE? Most workloads also care about
> latency to some extend. Lots of code has explicit cond_resched() and
> doesn't worry.
From a SLES11 system:
$ grep PREEMPT_NONE /boot/config-3.0.101-0.47.67-default
CONFIG_PREEMPT_NONE=y
From a SLES12 system:
$ grep CONFIG_PREEMPT_NONE /boot/config-4.4.16-56-default
CONFIG_PREEMPT_NONE=y
Bart.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFC][PATCH] dm: Remove dm_bufio_cond_resched()
2016-09-13 8:45 [RFC][PATCH] dm: Remove dm_bufio_cond_resched() Peter Zijlstra
2016-09-13 12:49 ` Thomas Gleixner
2016-09-13 13:39 ` Mike Snitzer
@ 2016-09-19 9:49 ` Mikulas Patocka
2016-09-19 10:47 ` Peter Zijlstra
2 siblings, 1 reply; 20+ messages in thread
From: Mikulas Patocka @ 2016-09-19 9:49 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Alasdair Kergon, Mike Snitzer, Thomas Gleixner, Ingo Molnar,
linux-kernel, dm-devel
On Tue, 13 Sep 2016, Peter Zijlstra wrote:
> Hi all,
>
> While grepping for PREEMPT_VOLUNTARY I ran into dm_bufio_cond_resched()
> and wondered WTH it was about.
cond_resched() calls _cond_resched() even if when we have a preemptive
kernel - with preemptive kernel, calling cond_resched is pointless because
rescheduling is done peemtively.
So, I added that dm_bufio_cond_resched(), that does nothing on peemptive
kernels (and also on PREEMPT_NONE kernels where the user doesn't care
about latency).
What is the reason why cond_resched() tests for rescheduling with
preemptive kernel? Why should I use cond_resched() in that case?
Mikulas
> Is there anything wrong with the below patch?
>
> ---
> diff --git a/drivers/md/dm-bufio.c b/drivers/md/dm-bufio.c
> index 8625040bae92..125aedc3875f 100644
> --- a/drivers/md/dm-bufio.c
> +++ b/drivers/md/dm-bufio.c
> @@ -191,19 +191,6 @@ static void dm_bufio_unlock(struct dm_bufio_client *c)
> mutex_unlock(&c->lock);
> }
>
> -/*
> - * FIXME Move to sched.h?
> - */
> -#ifdef CONFIG_PREEMPT_VOLUNTARY
> -# define dm_bufio_cond_resched() \
> -do { \
> - if (unlikely(need_resched())) \
> - _cond_resched(); \
> -} while (0)
> -#else
> -# define dm_bufio_cond_resched() do { } while (0)
> -#endif
> -
> /*----------------------------------------------------------------*/
>
> /*
> @@ -741,7 +728,7 @@ static void __flush_write_list(struct list_head *write_list)
> list_entry(write_list->next, struct dm_buffer, write_list);
> list_del(&b->write_list);
> submit_io(b, WRITE, b->block, write_endio);
> - dm_bufio_cond_resched();
> + cond_resched();
> }
> blk_finish_plug(&plug);
> }
> @@ -780,7 +767,7 @@ static struct dm_buffer *__get_unclaimed_buffer(struct dm_bufio_client *c)
> __unlink_buffer(b);
> return b;
> }
> - dm_bufio_cond_resched();
> + cond_resched();
> }
>
> list_for_each_entry_reverse(b, &c->lru[LIST_DIRTY], lru_list) {
> @@ -791,7 +778,7 @@ static struct dm_buffer *__get_unclaimed_buffer(struct dm_bufio_client *c)
> __unlink_buffer(b);
> return b;
> }
> - dm_bufio_cond_resched();
> + cond_resched();
> }
>
> return NULL;
> @@ -923,7 +910,7 @@ static void __write_dirty_buffers_async(struct dm_bufio_client *c, int no_wait,
> return;
>
> __write_dirty_buffer(b, write_list);
> - dm_bufio_cond_resched();
> + cond_resched();
> }
> }
>
> @@ -973,7 +960,7 @@ static void __check_watermark(struct dm_bufio_client *c,
> return;
>
> __free_buffer_wake(b);
> - dm_bufio_cond_resched();
> + cond_resched();
> }
>
> if (c->n_buffers[LIST_DIRTY] > threshold_buffers)
> @@ -1170,7 +1157,7 @@ void dm_bufio_prefetch(struct dm_bufio_client *c,
> submit_io(b, READ, b->block, read_endio);
> dm_bufio_release(b);
>
> - dm_bufio_cond_resched();
> + cond_resched();
>
> if (!n_blocks)
> goto flush_plug;
> @@ -1291,7 +1278,7 @@ int dm_bufio_write_dirty_buffers(struct dm_bufio_client *c)
> !test_bit(B_WRITING, &b->state))
> __relink_lru(b, LIST_CLEAN);
>
> - dm_bufio_cond_resched();
> + cond_resched();
>
> /*
> * If we dropped the lock, the list is no longer consistent,
> @@ -1574,7 +1561,7 @@ static unsigned long __scan(struct dm_bufio_client *c, unsigned long nr_to_scan,
> freed++;
> if (!--nr_to_scan || ((count - freed) <= retain_target))
> return freed;
> - dm_bufio_cond_resched();
> + cond_resched();
> }
> }
> return freed;
> @@ -1808,7 +1795,7 @@ static void __evict_old_buffers(struct dm_bufio_client *c, unsigned long age_hz)
> if (__try_evict_buffer(b, 0))
> count--;
>
> - dm_bufio_cond_resched();
> + cond_resched();
> }
>
> dm_bufio_unlock(c);
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFC][PATCH] dm: Remove dm_bufio_cond_resched()
2016-09-19 9:49 ` Mikulas Patocka
@ 2016-09-19 10:47 ` Peter Zijlstra
0 siblings, 0 replies; 20+ messages in thread
From: Peter Zijlstra @ 2016-09-19 10:47 UTC (permalink / raw)
To: Mikulas Patocka
Cc: Alasdair Kergon, Mike Snitzer, Thomas Gleixner, Ingo Molnar,
linux-kernel, dm-devel
On Mon, Sep 19, 2016 at 05:49:07AM -0400, Mikulas Patocka wrote:
>
>
> On Tue, 13 Sep 2016, Peter Zijlstra wrote:
>
> > Hi all,
> >
> > While grepping for PREEMPT_VOLUNTARY I ran into dm_bufio_cond_resched()
> > and wondered WTH it was about.
>
> cond_resched() calls _cond_resched() even if when we have a preemptive
> kernel - with preemptive kernel, calling cond_resched is pointless because
> rescheduling is done peemtively.
>
> So, I added that dm_bufio_cond_resched(), that does nothing on peemptive
> kernels (and also on PREEMPT_NONE kernels where the user doesn't care
> about latency).
>
> What is the reason why cond_resched() tests for rescheduling with
> preemptive kernel? Why should I use cond_resched() in that case?
Because every body else does too. 'Fixing' something like that in the dm
code is entirely the wrong place. Also, you loose out on the
might_sleep() warning implied in it.
As it happens, I have a patch fixing that somewhere, let me try and get
it merged.
But thanks for the reminder, I'll go write a Changelog for this so that
people can commit.
^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2016-09-23 14:33 UTC | newest]
Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-09-13 8:45 [RFC][PATCH] dm: Remove dm_bufio_cond_resched() Peter Zijlstra
2016-09-13 12:49 ` Thomas Gleixner
2016-09-13 13:39 ` Mike Snitzer
2016-09-19 10:53 ` Peter Zijlstra
2016-09-22 20:53 ` Mikulas Patocka
2016-09-22 20:59 ` Thomas Gleixner
2016-09-23 7:34 ` Peter Zijlstra
2016-09-23 8:00 ` Thomas Gleixner
2016-09-23 9:05 ` Peter Zijlstra
2016-09-23 9:13 ` Thomas Gleixner
2016-09-23 9:26 ` Ingo Molnar
2016-09-23 12:17 ` Mike Galbraith
2016-09-23 12:26 ` Peter Zijlstra
2016-09-23 12:39 ` Mike Galbraith
2016-09-23 12:42 ` Mike Snitzer
2016-09-23 12:46 ` Peter Zijlstra
2016-09-23 14:32 ` [dm-devel] " Bart Van Assche
2016-09-23 14:32 ` Bart Van Assche
2016-09-19 9:49 ` Mikulas Patocka
2016-09-19 10:47 ` Peter Zijlstra
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.