Messages in this thread | | | Date | Tue, 6 Mar 2012 16:34:37 -0500 | From | Vivek Goyal <> | 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
| |