dm: Remove dm_bufio_cond_resched()

RFC PATCH In review Peter Zijlstra 7 participants 19 messages Sep 13, 2016 – Sep 23, 2016 Device mapper

lore

Share & tools

lore

Peter Zijlstra

author

2016-09-13 08:45 UTC #0 lore raw

RFC PATCH 1/1 dm: Remove dm_bufio_cond_resched()

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?

---

drivers/md/dm-bufio.c +9 ~~-22~~

index 8625040bae92..125aedc3875f 100644
@@ -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);

Thomas Gleixner

Acked-by on v1 1/1

2016-09-13 12:49 UTC #1 lore raw

Quoted 6 lines · Peter Zijlstra

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 <[email protected]>

Mike Snitzer

2016-09-13 13:39 UTC #2 lore raw

In reply to Peter Zijlstra

Quoted 6 lines · Peter Zijlstra

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

Peter Zijlstra

author Acked-by Thomas Gleixner

2016-09-19 10:53 UTC #3 lore raw

Quoted 4 lines · Mike Snitzer

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 <[email protected]>
Date: Tue, 13 Sep 2016 10:45:20 +0200

Remove pointless local wrappery. Use cond_resched() like everybody else.

Cc Ingo Molnar <[email protected]>

Cc Mikulas Patocka <[email protected]>

Cc Mike Snitzer <[email protected]>

Cc Alasdair Kergon <[email protected]>

Acked-by Thomas Gleixner <[email protected]>

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

---
 drivers/md/dm-bufio.c |   31 +++++++++----------------------
 1 file changed, 9 insertions(+), 22 deletions(-)

drivers/md/dm-bufio.c +9 ~~-22~~

@@ -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);

Mikulas Patocka

maintainer

2016-09-22 20:53 UTC #4 lore raw

Quoted 7 lines · Peter Zijlstra

Quoted 4 lines · Mike Snitzer
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

Quoted 121 lines

---
Subject: dm: Remove dm_bufio_cond_resched()
From: Peter Zijlstra <[email protected]>
Date: Tue, 13 Sep 2016 10:45:20 +0200

Remove pointless local wrappery. Use cond_resched() like everybody else.

Cc Ingo Molnar <[email protected]>

Cc Mikulas Patocka <[email protected]>

Cc Mike Snitzer <[email protected]>

Cc Alasdair Kergon <[email protected]>

Acked-by Thomas Gleixner <[email protected]>

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

---
 drivers/md/dm-bufio.c |   31 +++++++++----------------------
 1 file changed, 9 insertions(+), 22 deletions(-)

drivers/md/dm-bufio.c +9 ~~-22~~

@@ -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);

Thomas Gleixner

2016-09-22 20:59 UTC #5 lore raw

Quoted 11 lines · Mikulas Patocka

Quoted 7 lines · Peter Zijlstra
Quoted 4 lines · Mike Snitzer
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.

Quoted 4 lines

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

Peter Zijlstra

author

2016-09-23 07:34 UTC #6 lore raw

Quoted 7 lines · Thomas Gleixner

Quoted 6 lines · Mikulas Patocka
Quoted 4 lines · Peter Zijlstra
Quoted 3 lines · Mike Snitzer
So I'm not sure how this dm-bufio local cond_resched() wrapper still got in... happy to take your patch.

Quoted 7 lines

Quoted 5 lines
Quoted 3 lines
Quoted 1 line
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.

Quoted 6 lines

Quoted 4 lines
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.

Thomas Gleixner

2016-09-23 08:00 UTC #7 lore raw

Quoted 12 lines · Peter Zijlstra

Quoted 11 lines · Thomas Gleixner
Quoted 10 lines · Mikulas Patocka
Quoted 7 lines · Peter Zijlstra
Quoted 4 lines · Mike Snitzer
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.

Quoted 7 lines

Quoted 3 lines
Quoted 1 line
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!

Quoted 10 lines

Quoted 6 lines
Quoted 4 lines
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

Peter Zijlstra

author

2016-09-23 09:05 UTC #8 lore raw

Quoted 6 lines · Thomas Gleixner

Quoted 3 lines · Peter Zijlstra
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.

Thomas Gleixner

2016-09-23 09:13 UTC #9 lore raw

Quoted 11 lines · Peter Zijlstra

Quoted 6 lines · Thomas Gleixner
Quoted 3 lines · Peter Zijlstra
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

Ingo Molnar

2016-09-23 09:26 UTC #10 lore raw

Quoted 9 lines · Thomas Gleixner

Quoted 8 lines · Peter Zijlstra
Quoted 6 lines · Thomas Gleixner
Quoted 3 lines · Peter Zijlstra
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!

Quoted 8 lines

Quoted 3 lines
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

Mike Galbraith

2016-09-23 12:17 UTC #11 lore raw

In reply to Thomas Gleixner

Quoted 7 lines · Thomas Gleixner

Quoted 3 lines · Peter Zijlstra
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

Peter Zijlstra

author

2016-09-23 12:26 UTC #12 lore raw

Quoted 10 lines · Mike Galbraith

Quoted 7 lines · Thomas Gleixner
Quoted 3 lines · Peter Zijlstra
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.

Mike Galbraith

2016-09-23 12:39 UTC #13 lore raw

Quoted 10 lines · Peter Zijlstra

Quoted 9 lines · Mike Galbraith
Quoted 8 lines · Thomas Gleixner
Quoted 5 lines · Peter Zijlstra
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.

Quoted 6 lines

Quoted 4 lines
Quoted 1 line
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

Mike Snitzer

2016-09-23 12:42 UTC #14 lore raw

In reply to Peter Zijlstra

Quoted 8 lines · Peter Zijlstra

Quoted 7 lines · Mike Galbraith
Quoted 6 lines · Thomas Gleixner
Quoted 3 lines · Peter Zijlstra
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.

Quoted 5 lines

Quoted 3 lines
Quoted 1 line
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

Peter Zijlstra

author

2016-09-23 12:46 UTC #15 lore raw

Quoted 13 lines · Mike Snitzer

Quoted 10 lines · Peter Zijlstra
Quoted 9 lines · Mike Galbraith
Quoted 7 lines · Thomas Gleixner
Quoted 3 lines · Peter Zijlstra
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.

Quoted 7 lines

Quoted 3 lines
Quoted 1 line
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.

Bart Van Assche

2016-09-23 14:32 UTC #16 lore raw

In reply to Peter Zijlstra

Quoted 3 lines · Peter Zijlstra

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.

Mikulas Patocka

maintainer

2016-09-19 09:49 UTC #17 lore raw

In reply to Peter Zijlstra

Quoted 4 lines · Peter Zijlstra

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

Quoted 109 lines

Is there anything wrong with the below patch?

---

drivers/md/dm-bufio.c +9 ~~-22~~

index 8625040bae92..125aedc3875f 100644
@@ -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);

Peter Zijlstra

author

2016-09-19 10:47 UTC #18 lore raw

Quoted 13 lines · Mikulas Patocka

Quoted 4 lines · Peter Zijlstra
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.

Quoted 6 lines

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.