On 2011-04-11 23:14, NeilBrown wrote: > On Mon, 11 Apr 2011 12:59:23 -0400 "hch@xxxxxxxxxxxxx" <hch@xxxxxxxxxxxxx> > 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. Yep that is correct, it's to avoid juggling irq on and off for multiple queues. I will put a comment there. -- Jens Axboe -- To unsubscribe from this list: send the line "unsubscribe linux-raid" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
