Re: [PATCH V2 10/20] blk-mq-sched: introduce helpers for query, change busy state

[Ming Lei]

On Wed, Aug 23, 2017 at 02:02:20PM -0600, Jens Axboe wrote:
> On Tue, Aug 22 2017, Bart Van Assche wrote:
> > On Sat, 2017-08-05 at 14:56 +0800, Ming Lei wrote:
> > > +static inline bool blk_mq_hctx_is_dispatch_busy(struct blk_mq_hw_ctx 
> > > *hctx)
> > > +{
> > > + return test_bit(BLK_MQ_S_DISPATCH_BUSY, >state);
> > > +}
> > > +
> > > +static inline void blk_mq_hctx_set_dispatch_busy(struct blk_mq_hw_ctx 
> > > *hctx)
> > > +{
> > > + set_bit(BLK_MQ_S_DISPATCH_BUSY, >state);
> > > +}
> > > +
> > > +static inline void blk_mq_hctx_clear_dispatch_busy(struct blk_mq_hw_ctx 
> > > *hctx)
> > > +{
> > > + clear_bit(BLK_MQ_S_DISPATCH_BUSY, >state);
> > > +}
> > 
> > Hello Ming,
> > 
> > Are these helper functions modified in a later patch? If not, please drop
> > this patch. One line helper functions are not useful and do not improve
> > readability of source code.
> 
> Agree, they just obfuscate the code. Only reason to do this is to do
> things like:

If you look at the following patch, these introduced functions are
modified a lot.

> 
> static inline void blk_mq_hctx_clear_dispatch_busy(struct blk_mq_hw_ctx *hctx)
> {
> if (test_bit(BLK_MQ_S_DISPATCH_BUSY, >state))
>   clear_bit(BLK_MQ_S_DISPATCH_BUSY, >state);
> }
> 
> to avoid unecessary RMW (and locked instruction). At least then you can
> add a single comment as to why it's done that way. Apart from that, I
> prefer to open-code it so people don't have to grep to figure out wtf
> blk_mq_hctx_clear_dispatch_busy() does.

Ok, will do that in this way.



-- 
Ming

Re: [PATCH V2 10/20] blk-mq-sched: introduce helpers for query, change busy state

[Ming Lei]

On Tue, Aug 22, 2017 at 08:41:14PM +, Bart Van Assche wrote:
> On Sat, 2017-08-05 at 14:56 +0800, Ming Lei wrote:
> > +static inline bool blk_mq_hctx_is_dispatch_busy(struct blk_mq_hw_ctx *hctx)
> > +{
> > +   return test_bit(BLK_MQ_S_DISPATCH_BUSY, >state);
> > +}
> > +
> > +static inline void blk_mq_hctx_set_dispatch_busy(struct blk_mq_hw_ctx 
> > *hctx)
> > +{
> > +   set_bit(BLK_MQ_S_DISPATCH_BUSY, >state);
> > +}
> > +
> > +static inline void blk_mq_hctx_clear_dispatch_busy(struct blk_mq_hw_ctx 
> > *hctx)
> > +{
> > +   clear_bit(BLK_MQ_S_DISPATCH_BUSY, >state);
> > +}
> 
> Hello Ming,
> 
> Are these helper functions modified in a later patch? If not, please drop
> this patch. One line helper functions are not useful and do not improve
> readability of source code.

It is definitely modified in later patch, :-)

-- 
Ming

Re: [PATCH V2 10/20] blk-mq-sched: introduce helpers for query, change busy state

[Bart Van Assche]

On Sat, 2017-08-05 at 14:56 +0800, Ming Lei wrote:
> +static inline bool blk_mq_hctx_is_dispatch_busy(struct blk_mq_hw_ctx *hctx)
> +{
> + return test_bit(BLK_MQ_S_DISPATCH_BUSY, >state);
> +}
> +
> +static inline void blk_mq_hctx_set_dispatch_busy(struct blk_mq_hw_ctx *hctx)
> +{
> + set_bit(BLK_MQ_S_DISPATCH_BUSY, >state);
> +}
> +
> +static inline void blk_mq_hctx_clear_dispatch_busy(struct blk_mq_hw_ctx 
> *hctx)
> +{
> + clear_bit(BLK_MQ_S_DISPATCH_BUSY, >state);
> +}

Hello Ming,

Are these helper functions modified in a later patch? If not, please drop
this patch. One line helper functions are not useful and do not improve
readability of source code.

Thanks,

Bart.

[PATCH V2 10/20] blk-mq-sched: introduce helpers for query, change busy state

[Ming Lei]

Signed-off-by: Ming Lei 
---
 block/blk-mq-sched.c |  6 +++---
 block/blk-mq.c   |  4 ++--
 block/blk-mq.h   | 15 +++
 3 files changed, 20 insertions(+), 5 deletions(-)

diff --git a/block/blk-mq-sched.c b/block/blk-mq-sched.c
index 18d997679d7d..9fae76275acf 100644
--- a/block/blk-mq-sched.c
+++ b/block/blk-mq-sched.c
@@ -181,7 +181,7 @@ void blk_mq_sched_dispatch_requests(struct blk_mq_hw_ctx 
*hctx)
 * effect.
 */
if (list_empty_careful(>dispatch))
-   clear_bit(BLK_MQ_S_DISPATCH_BUSY, >state);
+   blk_mq_hctx_clear_dispatch_busy(hctx);
}
 
/*
@@ -189,7 +189,7 @@ void blk_mq_sched_dispatch_requests(struct blk_mq_hw_ctx 
*hctx)
 * will be run to try to make progress, so it is always
 * safe to check the state here.
 */
-   if (test_bit(BLK_MQ_S_DISPATCH_BUSY, >state))
+   if (blk_mq_hctx_is_dispatch_busy(hctx))
return;
 
if (!has_sched_dispatch) {
@@ -342,7 +342,7 @@ static bool blk_mq_sched_bypass_insert(struct blk_mq_hw_ctx 
*hctx,
 * the dispatch list.
 */
spin_lock(>lock);
-   set_bit(BLK_MQ_S_DISPATCH_BUSY, >state);
+   blk_mq_hctx_set_dispatch_busy(hctx);
list_add(>queuelist, >dispatch);
spin_unlock(>lock);
return true;
diff --git a/block/blk-mq.c b/block/blk-mq.c
index b535587570b8..11042aa0501d 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -1105,7 +1105,7 @@ bool blk_mq_dispatch_rq_list(struct request_queue *q, 
struct list_head *list)
 * DISPATCH_BUSY won't be cleared until all requests
 * in hctx->dispatch are dispatched successfully
 */
-   set_bit(BLK_MQ_S_DISPATCH_BUSY, >state);
+   blk_mq_hctx_set_dispatch_busy(hctx);
list_splice_init(list, >dispatch);
spin_unlock(>lock);
 
@@ -1883,7 +1883,7 @@ static int blk_mq_hctx_notify_dead(unsigned int cpu, 
struct hlist_node *node)
return 0;
 
spin_lock(>lock);
-   set_bit(BLK_MQ_S_DISPATCH_BUSY, >state);
+   blk_mq_hctx_set_dispatch_busy(hctx);
list_splice_tail_init(, >dispatch);
spin_unlock(>lock);
 
diff --git a/block/blk-mq.h b/block/blk-mq.h
index 260b608af336..cadc0c83a140 100644
--- a/block/blk-mq.h
+++ b/block/blk-mq.h
@@ -136,4 +136,19 @@ static inline bool blk_mq_hw_queue_mapped(struct 
blk_mq_hw_ctx *hctx)
return hctx->nr_ctx && hctx->tags;
 }
 
+static inline bool blk_mq_hctx_is_dispatch_busy(struct blk_mq_hw_ctx *hctx)
+{
+   return test_bit(BLK_MQ_S_DISPATCH_BUSY, >state);
+}
+
+static inline void blk_mq_hctx_set_dispatch_busy(struct blk_mq_hw_ctx *hctx)
+{
+   set_bit(BLK_MQ_S_DISPATCH_BUSY, >state);
+}
+
+static inline void blk_mq_hctx_clear_dispatch_busy(struct blk_mq_hw_ctx *hctx)
+{
+   clear_bit(BLK_MQ_S_DISPATCH_BUSY, >state);
+}
+
 #endif
-- 
2.9.4