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

[Vivek Goyal <vgoyal@redhat.com>]

On Tue, Mar 06, 2012 at 01:20:34PM -0800, Andrew Morton wrote:

[..]
> > @@ -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..
> 

Will do. This basically says that if stats are not allocated yet, return
and don't try to account anything.

[..]
> > +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.

Will add some comments. Once we create a new group, we add it to the 
linked list and wake up the worker thread. This is the work function which
is run by worker thread. Once it finds that list is empty, it knows there
is no group waiting for allocation and worker exits.

> > +	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?

If allocation fails, I am trying to allocate it again in infinite loop.
What should I do? Try it after sleeping a bit? Or give up after certain
number of tries? This is in worker thread context though, so main IO path
is not impacted.

[..]
> > +		}
> > +		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.

Yes. Once alloc_list is empty, we know there are no groups needing 
per cpu stat allocation and worker exits. Once a new group is created
it will be added to the list and work will be scheduled again.

Thanks
Vivek