linux-kernel - Re: [PATCHSET] mempool, percpu, blkcg: fix percpu stat allocation and remove stats

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 6 Mar 2012 13:55:31 -0800
From:	Andrew Morton <akpm@...ux-foundation.org>
To:	Vivek Goyal <vgoyal@...hat.com>
Cc:	Tejun Heo <tj@...nel.org>, axboe@...nel.dk, hughd@...gle.com,
	avi@...hat.com, nate@...nel.net, cl@...ux-foundation.org,
	linux-kernel@...r.kernel.org, dpshah@...gle.com,
	ctalbott@...gle.com, rni@...gle.com
Subject: Re: [PATCHSET] mempool, percpu, blkcg: fix percpu stat allocation
 and remove stats_lock

On Tue, 6 Mar 2012 16:34:37 -0500
Vivek Goyal <vgoyal@...hat.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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/