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 16:34:37 -0500
From:	Vivek Goyal <vgoyal@...hat.com>
To:	Andrew Morton <akpm@...ux-foundation.org>
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, 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
--
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/