Re: [dm-devel] [PATCH V5 7/7] blk-mq-sched: don't dequeue request until all in ->dispatch are flushed

[Ming Lei]

On Tue, Oct 03, 2017 at 02:11:28AM -0700, Christoph Hellwig wrote:
> This looks good in general:
> 
> Reviewed-by: Christoph Hellwig 
> 
> Minor nitpicks below:
> 
> > const bool has_sched_dispatch = e && e->type->ops.mq.dispatch_request;
> 
> This is now only tested once, so you can remove the local variable
> for it.

There are still two users of the local variable, so I suggest to
keep it.

> 
> > +   /*
> > +* We may clear DISPATCH_BUSY just after it
> > +* is set from another context, the only cost
> > +* is that one request is dequeued a bit early,
> > +* we can survive that. Given the window is
> > +* small enough, no need to worry about performance
> > +* effect.
> > +*/
> 
> Use your 80 line real estate for comments please.

OK.

> 
> > if (!has_sched_dispatch)
> > +   if (!q->queue_depth) {
> > +   blk_mq_flush_busy_ctxs(hctx, _list);
> > +   blk_mq_dispatch_rq_list(q, _list);
> > +   } else {
> > +   blk_mq_do_dispatch_ctx(q, hctx);
> > +   }
> > +   } else {
> > blk_mq_do_dispatch_sched(q, e, hctx);
> > +   }
> 
> Maybe flatten this out to:
> 
>   if (e && e->type->ops.mq.dispatch_request) {
>   blk_mq_do_dispatch_sched(q, e, hctx);
>   } else if (q->queue_depth) {
>   blk_mq_do_dispatch_ctx(q, hctx);
>   } else {
>   blk_mq_flush_busy_ctxs(hctx, _list);
>   blk_mq_dispatch_rq_list(q, _list);
>   }
> 

OK.


-- 
Ming

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH V5 7/7] blk-mq-sched: don't dequeue request until all in ->dispatch are flushed

[Christoph Hellwig]

This looks good in general:

Reviewed-by: Christoph Hellwig 

Minor nitpicks below:

>   const bool has_sched_dispatch = e && e->type->ops.mq.dispatch_request;

This is now only tested once, so you can remove the local variable
for it.

> + /*
> +  * We may clear DISPATCH_BUSY just after it
> +  * is set from another context, the only cost
> +  * is that one request is dequeued a bit early,
> +  * we can survive that. Given the window is
> +  * small enough, no need to worry about performance
> +  * effect.
> +  */

Use your 80 line real estate for comments please.

>   if (!has_sched_dispatch)
> + if (!q->queue_depth) {
> + blk_mq_flush_busy_ctxs(hctx, _list);
> + blk_mq_dispatch_rq_list(q, _list);
> + } else {
> + blk_mq_do_dispatch_ctx(q, hctx);
> + }
> + } else {
>   blk_mq_do_dispatch_sched(q, e, hctx);
> + }

Maybe flatten this out to:

if (e && e->type->ops.mq.dispatch_request) {
blk_mq_do_dispatch_sched(q, e, hctx);
} else if (q->queue_depth) {
blk_mq_do_dispatch_ctx(q, hctx);
} else {
blk_mq_flush_busy_ctxs(hctx, _list);
blk_mq_dispatch_rq_list(q, _list);
}

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel