Re: [PATCHSET] mempool, percpu, blkcg: fix percpu stat allocation and remove stats_lock

[Andrew Morton <akpm@linux-foundation.org>]

On Tue, 6 Mar 2012 16:09:55 -0500
Vivek Goyal <vgoyal@redhat.com> wrote:

>
> ...
>
> blk-cgroup: Alloc per cpu stats from worker thread in a delayed manner
> 
> Current per cpu stat allocation assumes GFP_KERNEL allocation flag. But in
> IO path there are times when we want GFP_NOIO semantics. As there is no
> way to pass the allocation flags to alloc_percpu(), this patch delays the
> allocation of stats using a worker thread.
> 
> v2-> tejun suggested following changes. Changed the patch accordingly.
> 	- move alloc_node location in structure
> 	- reduce the size of names of some of the fields
> 	- Reduce the scope of locking of alloc_list_lock
> 	- Simplified stat_alloc_fn() by allocating stats for all
> 	  policies in one go and then assigning these to a group.

<takes a look to see if he can understand some block stuff>

<decides he can't>

>
> ...
>
> @@ -30,6 +30,15 @@ static LIST_HEAD(blkio_list);
>  static DEFINE_MUTEX(all_q_mutex);
>  static LIST_HEAD(all_q_list);
>  
> +/* List of groups pending per cpu stats allocation */
> +static DEFINE_SPINLOCK(alloc_list_lock);
> +static LIST_HEAD(alloc_list);
> +
> +/* Array of per cpu stat pointers allocated for blk groups */
> +static void *pcpu_stats[BLKIO_NR_POLICIES];
> +static void blkio_stat_alloc_fn(struct work_struct *);
> +static DECLARE_WORK(blkio_stat_alloc_work, blkio_stat_alloc_fn);
> +
>  struct blkio_cgroup blkio_root_cgroup = { .weight = 2*BLKIO_WEIGHT_DEFAULT };
>  EXPORT_SYMBOL_GPL(blkio_root_cgroup);
>  
> @@ -391,6 +400,9 @@ void blkiocg_update_dispatch_stats(struc
>  	struct blkio_group_stats_cpu *stats_cpu;
>  	unsigned long flags;
>  
> +	if (pd->stats_cpu == NULL)
> +		return;

Maybe add a comment explaining how this comes about?  It isn't very
obvious..

>  	/*
>  	 * Disabling interrupts to provide mutual exclusion between two
>  	 * writes on same cpu. It probably is not needed for 64bit. Not
> @@ -443,6 +455,9 @@ void blkiocg_update_io_merged_stats(stru
>  	struct blkio_group_stats_cpu *stats_cpu;
>  	unsigned long flags;
>  
> +	if (pd->stats_cpu == NULL)
> +		return;
> +
>  	/*
>  	 * Disabling interrupts to provide mutual exclusion between two
>  	 * writes on same cpu. It probably is not needed for 64bit. Not
> @@ -460,6 +475,59 @@ void blkiocg_update_io_merged_stats(stru
>  }
>  EXPORT_SYMBOL_GPL(blkiocg_update_io_merged_stats);
>  
> +static void blkio_stat_alloc_fn(struct work_struct *work)
> +{
> +
> +	struct blkio_group *blkg, *n;
> +	int i;
> +
> +alloc_stats:
> +	spin_lock_irq(&alloc_list_lock);
> +		if (list_empty(&alloc_list)) {
> +			/* Nothing to do */

That's not a very helpful comment, given that we weren't told what the
function is supposed to do in the first place.

> +			spin_unlock_irq(&alloc_list_lock);
> +			return;
> +		}
> +	spin_unlock_irq(&alloc_list_lock);

Interesting code layout - I rather like it!

> +	for (i = 0; i < BLKIO_NR_POLICIES; i++) {
> +		if (pcpu_stats[i] != NULL)
> +			continue;
> +
> +		pcpu_stats[i] = alloc_percpu(struct blkio_group_stats_cpu);
> +		if (pcpu_stats[i] == NULL)
> +			goto alloc_stats;

hoo boy that looks like an infinite loop.  What's going on here?

> +	}
> +
> +	spin_lock_irq(&blkio_list_lock);
> +	spin_lock(&alloc_list_lock);
> +
> +	list_for_each_entry_safe(blkg, n, &alloc_list, alloc_node) {
> +		for (i = 0; i < BLKIO_NR_POLICIES; i++) {
> +			struct blkio_policy_type *pol = blkio_policy[i];
> +			struct blkg_policy_data *pd;
> +
> +			if (!pol)
> +				continue;
> +
> +			if (!blkg->pd[i])
> +				continue;
> +
> +			pd = blkg->pd[i];
> +			if (pd->stats_cpu)
> +				continue;
> +
> +			pd->stats_cpu = pcpu_stats[i];
> +			pcpu_stats[i] = NULL;
> +		}
> +		list_del_init(&blkg->alloc_node);
> +		break;
> +	}
> +	spin_unlock(&alloc_list_lock);
> +	spin_unlock_irq(&blkio_list_lock);
> +	goto alloc_stats;
> +}

So the function runs until alloc_list is empty.  Very mysterious.

>
> ...
>