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

[NeilBrown <neilb@suse.de>]

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

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


Thanks,
NeilBrown

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