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

On Mon, 11 Apr 2011 12:59:23 -0400 "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

--
To unsubscribe from this list: send the line "unsubscribe linux-raid" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

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

On Tue, Apr 12, 2011 at 07:14:28AM +1000, NeilBrown wrote:
>
> 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.

Your explanation does make sense to be now that you explain it. I
didn't even thing of that variant before.

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

On 2011-04-11 23:14, NeilBrown wrote:
> On Mon, 11 Apr 2011 12:59:23 -0400 "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.

Yep that is correct, it's to avoid juggling irq on and off for multiple
queues. I will put a comment there.


--
Jens Axboe