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:34:37 -0500
Vivek Goyal <vgoyal@redhat.com> wrote:

> On Tue, Mar 06, 2012 at 01:20:34PM -0800, Andrew Morton wrote:
> 
> > > +	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.

On a non-preemptible unprocessor kernel it's game over, isn't it? 
Unless someone frees some memory from interrupt context it is time for
the Big Red Button.

I'm not sure what to suggest, really - if an allocation failed then
there's nothing the caller can reliably do to fix that.  The best
approach is to fail all the way back to userspace with -ENOMEM.

In this context I suppose you could drop a warning into the logs then
bale out and retry on the next IO attempt.


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

Oh, is that what alloc_list does ;)


btw, speaking of uniprocessor: please do perform a uniprocessor build
and see what impact the patch has upon the size(1) output for the .o
files.  We should try to minimize the pointless bloat for the UP
kernel.