Re: [PATCH 05/10] block: remove per-queue plugging

[NeilBrown <neilb@suse.de>]

On Mon, 11 Apr 2011 12:59:23 -0400 "hch@infradead.org" <hch@infradead.org>
wrote:

> On Mon, Apr 11, 2011 at 02:50:22PM +1000, NeilBrown wrote:
> > diff --git a/block/blk-core.c b/block/blk-core.c
> > index 273d60b..903ce8d 100644
> > --- a/block/blk-core.c
> > +++ b/block/blk-core.c
> > @@ -2674,19 +2674,23 @@ static void flush_plug_list(struct blk_plug *plug)
> >  	struct request_queue *q;
> >  	unsigned long flags;
> >  	struct request *rq;
> > +	struct list_head head;
> >  
> >  	BUG_ON(plug->magic != PLUG_MAGIC);
> >  
> >  	if (list_empty(&plug->list))
> >  		return;
> > +	list_add(&head, &plug->list);
> > +	list_del_init(&plug->list);
> >  
> >  	if (plug->should_sort)
> > -		list_sort(NULL, &plug->list, plug_rq_cmp);
> > +		list_sort(NULL, &head, plug_rq_cmp);
> > +	plug->should_sort = 0;
> 
> As Jens mentioned this should be list_splice_init.  But looking over
> flush_plug_list the code there seems strange to me.
> 
> What does the local_irq_save in flush_plug_list protect?  Why don't
> we need it over the list_sort?  And do we still need it when first
> splicing the list to a local one?
> 
> It's one of these cases where I'd really like to see more comments
> explaining why the code is doing what it's doing.

My understanding of that was that the calling requirement of
__elv_add_request is that the queue spinlock is held and that interrupts are
disabled.
So rather than possible enabling and disabling interrupts several times as
different queue are handled, the code just disabled interrupts once, and
then just take the spinlock once for each different queue.

The whole point of the change to plugging was to take locks less often.
Disabling interrupts less often is presumably an analogous goal.

Though I agree that a comment would help.

	q = NULL;
+	/* Disable interrupts just once rather than using spin_lock_irq/sin_unlock_irq
	 * variants
	 */
	local_irq_save(flags);


assuming my analysis is correct.

NeilBrown