Re: [patch v3 5/5] raid5: only wakeup necessary threads

[NeilBrown <neilb@suse.de>]

[-- Attachment #1: Type: text/plain, Size: 2761 bytes --]

On Wed, 28 Aug 2013 14:31:59 +0800 Shaohua Li <shli@kernel.org> wrote:

> On Wed, Aug 28, 2013 at 02:13:04PM +1000, NeilBrown wrote:
> > On Tue, 27 Aug 2013 17:50:43 +0800 Shaohua Li <shli@kernel.org> wrote:
> > 
> > 
> > > @@ -229,8 +233,26 @@ static void raid5_wakeup_stripe_thread(s
> > >  
> > >  	group = conf->worker_groups + cpu_to_group(sh->cpu);
> > >  
> > > -	for (i = 0; i < conf->worker_cnt_per_group; i++)
> > > -		queue_work_on(sh->cpu, raid5_wq, &group->workers[i].work);
> > > +	group->workers[0].working = true;
> > > +	/* at least one worker should run to avoid race */
> > > +	queue_work_on(sh->cpu, raid5_wq, &group->workers[0].work);
> > > +
> > > +	thread_cnt = group->stripes_cnt / MAX_STRIPE_BATCH - 1;
> > > +	/* wakeup more workers */
> > > +	for (i = 1; i < conf->worker_cnt_per_group && thread_cnt > 0; i++) {
> > > +		if (group->workers[i].working == false) {
> > > +			group->workers[i].working = true;
> > > +			queue_work_on(sh->cpu, raid5_wq,
> > > +				      &group->workers[i].work);
> > > +			thread_cnt--;
> > > +		} else if (group->workers[i].working_cnt <=
> > > +			   MAX_STRIPE_BATCH / 2)
> > > +			/*
> > > +			 * If a worker has no enough stripes handling, assume
> > > +			 * it will fetch more stripes soon.
> > > +			 */
> > > +			thread_cnt--;
> > > +	}
> > >  }
> > 
> > I don't really understand this  "working_cnt <= MAX_STRIPE_BATCH / 2"
> > heuristic.  It is at best a very coarse estimate of how long until the worker
> > will get some more stripes to work on.
> > I think I would simply not count any thread that is already working (except
> > the first, which is always counted whether it is working or not)
> > Do you see some particular gain from the counting?
> > 
> > 
> > 
> > > -#define MAX_STRIPE_BATCH 8
> > > -static int handle_active_stripes(struct r5conf *conf, int group)
> > > +static int handle_active_stripes(struct r5conf *conf, int group,
> > > +		struct r5worker *worker)
> > >  {
> > >  	struct stripe_head *batch[MAX_STRIPE_BATCH], *sh;
> > >  	int i, batch_size = 0;
> > > @@ -4921,6 +4955,9 @@ static int handle_active_stripes(struct
> > >  			(sh = __get_priority_stripe(conf, group)) != NULL)
> > >  		batch[batch_size++] = sh;
> > >  
> > > +	if (worker)
> > > +		worker->working_cnt = batch_size;
> > > +
> > >  	if (batch_size == 0)
> > >  		return batch_size;
> > 
> > I think this could possibly return with ->working still 'true'.
> > I think it is safest to clear it on every exit from the function
> 
> I need do more tests on this one. Could you please apply other patches to your
> tree, then I can rebase this patch against it when I'm done.

Other patches are now applied and pushed out.

NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]