[PATCH 0/2 v3] Small cleanup of bdi_forker_thread()

[PATCH 0/2 v3] Small cleanup of bdi_forker_thread()

[Jan Kara]

  Hi Jens,

  so after I realized how I underestimated subtlety of bdi_forker_thread()
here are two small cleanups to it. The first one puts clearing of BDI_pending
in a more natural place, the second patch adds a comment so that fools like
me don't try to optimize the function in a wrong place...

								Honza

[PATCH 1/2] mm: Cleanup clearing of BDI_pending bit in bdi_forker_thread()

[Jan Kara]

bdi_forker_thread() clears BDI_pending bit at the end of the main loop.
However clearing of this bit must not be done in some cases which is handled by
calling 'continue' from switch statement. That's kind of unusual construct and
without a good reason so change the function into more intuitive code flow.

CC: Wu Fengguang <fengguang.wu@intel.com>
CC: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Jan Kara <jack@suse.cz>
---
 mm/backing-dev.c |   23 ++++++++++++++---------
 1 files changed, 14 insertions(+), 9 deletions(-)

diff --git a/mm/backing-dev.c b/mm/backing-dev.c
index d6edf8d..94a047b 100644
--- a/mm/backing-dev.c
+++ b/mm/backing-dev.c
@@ -359,6 +359,17 @@ static unsigned long bdi_longest_inactive(void)
 	return max(5UL * 60 * HZ, interval);
 }
 
+/*
+ * Clear pending bit and wakeup anybody waiting for flusher thread creation or
+ * shutdown
+ */
+static void bdi_clear_pending(struct backing_dev_info *bdi)
+{
+	clear_bit(BDI_pending, &bdi->state);
+	smp_mb__after_clear_bit();
+	wake_up_bit(&bdi->state, BDI_pending);
+}
+
 static int bdi_forker_thread(void *ptr)
 {
 	struct bdi_writeback *me = ptr;
@@ -469,11 +480,13 @@ static int bdi_forker_thread(void *ptr)
 				spin_unlock_bh(&bdi->wb_lock);
 				wake_up_process(task);
 			}
+			bdi_clear_pending(bdi);
 			break;
 
 		case KILL_THREAD:
 			__set_current_state(TASK_RUNNING);
 			kthread_stop(task);
+			bdi_clear_pending(bdi);
 			break;
 
 		case NO_ACTION:
@@ -489,16 +502,8 @@ static int bdi_forker_thread(void *ptr)
 			else
 				schedule_timeout(msecs_to_jiffies(dirty_writeback_interval * 10));
 			try_to_freeze();
-			/* Back to the main loop */
-			continue;
+			break;
 		}
-
-		/*
-		 * Clear pending bit and wakeup anybody waiting to tear us down.
-		 */
-		clear_bit(BDI_pending, &bdi->state);
-		smp_mb__after_clear_bit();
-		wake_up_bit(&bdi->state, BDI_pending);
 	}
 
 	return 0;
-- 
1.7.1

[PATCH 2/2] mm: Add comment explaining task state setting in bdi_forker_thread()

[Jan Kara]

CC: Wu Fengguang <fengguang.wu@intel.com>
CC: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Jan Kara <jack@suse.cz>
---
 mm/backing-dev.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/mm/backing-dev.c b/mm/backing-dev.c
index 94a047b..a87da52 100644
--- a/mm/backing-dev.c
+++ b/mm/backing-dev.c
@@ -401,6 +401,13 @@ static int bdi_forker_thread(void *ptr)
 		}
 
 		spin_lock_bh(&bdi_lock);
+		/*
+		 * In the following loop we are going to check whether we have
+		 * some work to do without any synchronization with tasks
+		 * waking us up to do work for them. So we have to set task
+		 * state already here so that we don't miss wakeups coming
+		 * after we verify some condition.
+		 */
 		set_current_state(TASK_INTERRUPTIBLE);
 
 		list_for_each_entry(bdi, &bdi_list, bdi_list) {
-- 
1.7.1

Re: [PATCH 0/2 v3] Small cleanup of bdi_forker_thread()

[Jens Axboe]

On 2011-09-02 16:54, Jan Kara wrote:
> 
>   Hi Jens,
> 
>   so after I realized how I underestimated subtlety of bdi_forker_thread()
> here are two small cleanups to it. The first one puts clearing of BDI_pending
> in a more natural place, the second patch adds a comment so that fools like
> me don't try to optimize the function in a wrong place...

Thanks Jan, I've queued it up for 3.1. I've marked it as stable, looks
like we need it from 2.6.36 and above.

-- 
Jens Axboe

Re: [PATCH 0/2 v3] Small cleanup of bdi_forker_thread()

[Jan Kara]

On Fri 02-09-11 17:07:10, Jens Axboe wrote:
> On 2011-09-02 16:54, Jan Kara wrote:
> > 
> >   Hi Jens,
> > 
> >   so after I realized how I underestimated subtlety of bdi_forker_thread()
> > here are two small cleanups to it. The first one puts clearing of BDI_pending
> > in a more natural place, the second patch adds a comment so that fools like
> > me don't try to optimize the function in a wrong place...
> 
> Thanks Jan, I've queued it up for 3.1. I've marked it as stable, looks
> like we need it from 2.6.36 and above.
  Thanks. But is it really a stable material? It's just a cleanup after
all...

								Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

Re: [PATCH 0/2 v3] Small cleanup of bdi_forker_thread()

[Jens Axboe]

On 2011-09-02 17:12, Jan Kara wrote:
> On Fri 02-09-11 17:07:10, Jens Axboe wrote:
>> On 2011-09-02 16:54, Jan Kara wrote:
>>>
>>>   Hi Jens,
>>>
>>>   so after I realized how I underestimated subtlety of bdi_forker_thread()
>>> here are two small cleanups to it. The first one puts clearing of BDI_pending
>>> in a more natural place, the second patch adds a comment so that fools like
>>> me don't try to optimize the function in a wrong place...
>>
>> Thanks Jan, I've queued it up for 3.1. I've marked it as stable, looks
>> like we need it from 2.6.36 and above.
>   Thanks. But is it really a stable material? It's just a cleanup after
> all...

I guess it's not strictly stable material, since it's not fixing a
problematic issue.

-- 
Jens Axboe

Re: [PATCH 1/2] mm: Cleanup clearing of BDI_pending bit in bdi_forker_thread()

[Wu Fengguang]

On Sat, Sep 03, 2011 at 06:54:18AM +0800, Jan Kara wrote:
> bdi_forker_thread() clears BDI_pending bit at the end of the main loop.
> However clearing of this bit must not be done in some cases which is handled by
> calling 'continue' from switch statement. That's kind of unusual construct and
> without a good reason so change the function into more intuitive code flow.
> 
> CC: Wu Fengguang <fengguang.wu@intel.com>
> CC: Andrew Morton <akpm@linux-foundation.org>
> Signed-off-by: Jan Kara <jack@suse.cz>

It's pure code refactor.

Reviewed-by: Wu Fengguang <fengguang.wu@intel.com>

Thanks,
Fengguang

Re: [PATCH 2/2] mm: Add comment explaining task state setting in bdi_forker_thread()

[Wu Fengguang]

> @@ -401,6 +401,13 @@ static int bdi_forker_thread(void *ptr)
>  		}
>  
>  		spin_lock_bh(&bdi_lock);
> +		/*
> +		 * In the following loop we are going to check whether we have
> +		 * some work to do without any synchronization with tasks
> +		 * waking us up to do work for them. So we have to set task
> +		 * state already here so that we don't miss wakeups coming

s/already/early/ ?

Acked-by: Wu Fengguang <fengguang.wu@intel.com>

> +		 * after we verify some condition.
> +		 */
>  		set_current_state(TASK_INTERRUPTIBLE);
>  
>  		list_for_each_entry(bdi, &bdi_list, bdi_list) {
> -- 
> 1.7.1

Re: [PATCH 1/2] mm: Cleanup clearing of BDI_pending bit in bdi_forker_thread()

[Wu Fengguang]

On Sun, Sep 04, 2011 at 11:04:42AM +0800, Wu Fengguang wrote:
> On Sat, Sep 03, 2011 at 06:54:18AM +0800, Jan Kara wrote:
> > bdi_forker_thread() clears BDI_pending bit at the end of the main loop.
> > However clearing of this bit must not be done in some cases which is handled by
> > calling 'continue' from switch statement. That's kind of unusual construct and
> > without a good reason so change the function into more intuitive code flow.
> > 
> > CC: Wu Fengguang <fengguang.wu@intel.com>
> > CC: Andrew Morton <akpm@linux-foundation.org>
> > Signed-off-by: Jan Kara <jack@suse.cz>
> 
> It's pure code refactor.
> 
> Reviewed-by: Wu Fengguang <fengguang.wu@intel.com>

But I do suspect it will slightly increase the code size.
What do you think?

Thanks,
Fengguang

Re: [PATCH 2/2] mm: Add comment explaining task state setting in bdi_forker_thread()

[Jan Kara]

On Sun 04-09-11 11:05:51, Wu Fengguang wrote:
> > @@ -401,6 +401,13 @@ static int bdi_forker_thread(void *ptr)
> >  		}
> >  
> >  		spin_lock_bh(&bdi_lock);
> > +		/*
> > +		 * In the following loop we are going to check whether we have
> > +		 * some work to do without any synchronization with tasks
> > +		 * waking us up to do work for them. So we have to set task
> > +		 * state already here so that we don't miss wakeups coming
> 
> s/already/early/ ?
  Thanks for review. We'd have to substitute 'already here' with 'early'
for the sentence to make make sense. But frankly I don't see why one would
be better than the other one...

								Honza
> 
> Acked-by: Wu Fengguang <fengguang.wu@intel.com>
> 
> > +		 * after we verify some condition.
> > +		 */
> >  		set_current_state(TASK_INTERRUPTIBLE);
> >  
> >  		list_for_each_entry(bdi, &bdi_list, bdi_list) {
> > -- 
> > 1.7.1
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

Re: [PATCH 1/2] mm: Cleanup clearing of BDI_pending bit in bdi_forker_thread()

[Jan Kara]

On Sun 04-09-11 12:13:05, Wu Fengguang wrote:
> On Sun, Sep 04, 2011 at 11:04:42AM +0800, Wu Fengguang wrote:
> > On Sat, Sep 03, 2011 at 06:54:18AM +0800, Jan Kara wrote:
> > > bdi_forker_thread() clears BDI_pending bit at the end of the main loop.
> > > However clearing of this bit must not be done in some cases which is handled by
> > > calling 'continue' from switch statement. That's kind of unusual construct and
> > > without a good reason so change the function into more intuitive code flow.
> > > 
> > > CC: Wu Fengguang <fengguang.wu@intel.com>
> > > CC: Andrew Morton <akpm@linux-foundation.org>
> > > Signed-off-by: Jan Kara <jack@suse.cz>
> > 
> > It's pure code refactor.
> > 
> > Reviewed-by: Wu Fengguang <fengguang.wu@intel.com>
> 
> But I do suspect it will slightly increase the code size.
> What do you think?
  I haven't checked, maybe it will if the compiler is not clever enough to
merge two occurences of the function which is going to be inlined. But the
overhead will be really small and the code is not really performance critical
so I think clarity has priority.

								Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

Re: [PATCH 2/2] mm: Add comment explaining task state setting in bdi_forker_thread()

[Wu Fengguang]

On Mon, Sep 05, 2011 at 06:01:41PM +0800, Jan Kara wrote:
> On Sun 04-09-11 11:05:51, Wu Fengguang wrote:
> > > @@ -401,6 +401,13 @@ static int bdi_forker_thread(void *ptr)
> > >  		}
> > >  
> > >  		spin_lock_bh(&bdi_lock);
> > > +		/*
> > > +		 * In the following loop we are going to check whether we have
> > > +		 * some work to do without any synchronization with tasks
> > > +		 * waking us up to do work for them. So we have to set task
> > > +		 * state already here so that we don't miss wakeups coming
> > 
> > s/already/early/ ?
>   Thanks for review. We'd have to substitute 'already here' with 'early'
> for the sentence to make make sense. But frankly I don't see why one would
> be better than the other one...

You are the native English speaker?  OK, I have no more problems... ;)

Thanks,
Fengguang

Re: [PATCH 1/2] mm: Cleanup clearing of BDI_pending bit in bdi_forker_thread()

[Wu Fengguang]

On Mon, Sep 05, 2011 at 06:06:12PM +0800, Jan Kara wrote:
> On Sun 04-09-11 12:13:05, Wu Fengguang wrote:
> > On Sun, Sep 04, 2011 at 11:04:42AM +0800, Wu Fengguang wrote:
> > > On Sat, Sep 03, 2011 at 06:54:18AM +0800, Jan Kara wrote:
> > > > bdi_forker_thread() clears BDI_pending bit at the end of the main loop.
> > > > However clearing of this bit must not be done in some cases which is handled by
> > > > calling 'continue' from switch statement. That's kind of unusual construct and
> > > > without a good reason so change the function into more intuitive code flow.
> > > > 
> > > > CC: Wu Fengguang <fengguang.wu@intel.com>
> > > > CC: Andrew Morton <akpm@linux-foundation.org>
> > > > Signed-off-by: Jan Kara <jack@suse.cz>
> > > 
> > > It's pure code refactor.
> > > 
> > > Reviewed-by: Wu Fengguang <fengguang.wu@intel.com>
> > 
> > But I do suspect it will slightly increase the code size.
> > What do you think?
>   I haven't checked, maybe it will if the compiler is not clever enough to
> merge two occurences of the function which is going to be inlined. But the
> overhead will be really small and the code is not really performance critical
> so I think clarity has priority.

I have no problem then. It's not a big matter.

Thanks,
Fengguang

Re: [PATCH 2/2] mm: Add comment explaining task state setting in bdi_forker_thread()

[Jan Kara]

On Mon 05-09-11 20:44:11, Wu Fengguang wrote:
> On Mon, Sep 05, 2011 at 06:01:41PM +0800, Jan Kara wrote:
> > On Sun 04-09-11 11:05:51, Wu Fengguang wrote:
> > > > @@ -401,6 +401,13 @@ static int bdi_forker_thread(void *ptr)
> > > >  		}
> > > >  
> > > >  		spin_lock_bh(&bdi_lock);
> > > > +		/*
> > > > +		 * In the following loop we are going to check whether we have
> > > > +		 * some work to do without any synchronization with tasks
> > > > +		 * waking us up to do work for them. So we have to set task
> > > > +		 * state already here so that we don't miss wakeups coming
> > > 
> > > s/already/early/ ?
> >   Thanks for review. We'd have to substitute 'already here' with 'early'
> > for the sentence to make make sense. But frankly I don't see why one would
> > be better than the other one...
> 
> You are the native English speaker?  OK, I have no more problems... ;)
  No, I'm not a native English speaker. Maybe that's why I don't see that
much difference :-))

									Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

Re: [PATCH 2/2] mm: Add comment explaining task state setting in bdi_forker_thread()

[Andrew Morton]

On Mon, 5 Sep 2011 17:53:47 +0200
Jan Kara <jack@suse.cz> wrote:

> On Mon 05-09-11 20:44:11, Wu Fengguang wrote:
> > On Mon, Sep 05, 2011 at 06:01:41PM +0800, Jan Kara wrote:
> > > On Sun 04-09-11 11:05:51, Wu Fengguang wrote:
> > > > > @@ -401,6 +401,13 @@ static int bdi_forker_thread(void *ptr)
> > > > >  		}
> > > > >  
> > > > >  		spin_lock_bh(&bdi_lock);
> > > > > +		/*
> > > > > +		 * In the following loop we are going to check whether we have
> > > > > +		 * some work to do without any synchronization with tasks
> > > > > +		 * waking us up to do work for them. So we have to set task
> > > > > +		 * state already here so that we don't miss wakeups coming
> > > > 
> > > > s/already/early/ ?
> > >   Thanks for review. We'd have to substitute 'already here' with 'early'
> > > for the sentence to make make sense. But frankly I don't see why one would
> > > be better than the other one...
> > 
> > You are the native English speaker?  OK, I have no more problems... ;)
>   No, I'm not a native English speaker. Maybe that's why I don't see that
> much difference :-))

grammar can often be fixed/improved by deleting stuff ;)

--- a/mm/backing-dev.c~mm-add-comment-explaining-task-state-setting-in-bdi_forker_thread-fix
+++ a/mm/backing-dev.c
@@ -404,9 +404,8 @@ static int bdi_forker_thread(void *ptr)
 		/*
 		 * In the following loop we are going to check whether we have
 		 * some work to do without any synchronization with tasks
-		 * waking us up to do work for them. So we have to set task
-		 * state already here so that we don't miss wakeups coming
-		 * after we verify some condition.
+		 * waking us up to do work for them. Set the task state here
+		 * so that we don't miss wakeups after verifying conditions.
 		 */
 		set_current_state(TASK_INTERRUPTIBLE);
 
_

Re: [PATCH 2/2] mm: Add comment explaining task state setting in bdi_forker_thread()

[Wu Fengguang]

> grammar can often be fixed/improved by deleting stuff ;)
> 
> --- a/mm/backing-dev.c~mm-add-comment-explaining-task-state-setting-in-bdi_forker_thread-fix
> +++ a/mm/backing-dev.c
> @@ -404,9 +404,8 @@ static int bdi_forker_thread(void *ptr)
>  		/*
>  		 * In the following loop we are going to check whether we have
>  		 * some work to do without any synchronization with tasks
> -		 * waking us up to do work for them. So we have to set task
> -		 * state already here so that we don't miss wakeups coming
> -		 * after we verify some condition.
> +		 * waking us up to do work for them. Set the task state here
> +		 * so that we don't miss wakeups after verifying conditions.
>  		 */
>  		set_current_state(TASK_INTERRUPTIBLE);

That's nice and readable, thanks!

Regards,
Fengguang