[PATCH] null_blk: add unlikely for REQ_OP_DISACRD handling

[PATCH] null_blk: add unlikely for REQ_OP_DISACRD handling

[Chaitanya Kulkarni]

Since discard requests are not as common as read and write requests
mark REQ_OP_DISCARD condition unlikely in the null_handle_rq() and
null_handle_bio().

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
---
 drivers/block/null_blk_main.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/block/null_blk_main.c b/drivers/block/null_blk_main.c
index 99328ded60d1..cbbbb89e89ab 100644
--- a/drivers/block/null_blk_main.c
+++ b/drivers/block/null_blk_main.c
@@ -1061,7 +1061,7 @@ static int null_handle_rq(struct nullb_cmd *cmd)
 
 	sector = blk_rq_pos(rq);
 
-	if (req_op(rq) == REQ_OP_DISCARD) {
+	if (unlikely(req_op(rq) == REQ_OP_DISCARD)) {
 		null_handle_discard(nullb, sector, blk_rq_bytes(rq));
 		return 0;
 	}
@@ -1095,7 +1095,7 @@ static int null_handle_bio(struct nullb_cmd *cmd)
 
 	sector = bio->bi_iter.bi_sector;
 
-	if (bio_op(bio) == REQ_OP_DISCARD) {
+	if (unlikely(bio_op(bio) == REQ_OP_DISCARD)) {
 		null_handle_discard(nullb, sector,
 			bio_sectors(bio) << SECTOR_SHIFT);
 		return 0;
-- 
2.17.0

Re: [PATCH] null_blk: add unlikely for REQ_OP_DISACRD handling

[Minwoo Im]

On 19-07-01 20:20:27, Chaitanya Kulkarni wrote:
> Since discard requests are not as common as read and write requests
> mark REQ_OP_DISCARD condition unlikely in the null_handle_rq() and
> null_handle_bio().
> 
> Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>

Chaitanya,

Just a simple doubt here.  I just have tested this patch with 'dd' to
see any performance benefit when queue_mode == 0 || 1.  But I don't
think it's worth it for the performance of read/write OR I have an wrong
way to test it.  you have never mentioned performance in this patch,
though.

But, I do like this patch because it will indicate what you have
mentioned in the commit message in the code level.

Reviewed-by: Minwoo Im <minwoo.im.dev@gmail.com>

Thanks!

Re: [PATCH] null_blk: add unlikely for REQ_OP_DISACRD handling

[Jens Axboe]

On 7/1/19 9:20 PM, Chaitanya Kulkarni wrote:
> Since discard requests are not as common as read and write requests
> mark REQ_OP_DISCARD condition unlikely in the null_handle_rq() and
> null_handle_bio().

We should let normal branch prediction handle this. What if you
are running a pure discard workload?

In general I'm not a huge fan of likely/unlikely annotations,
they only tend to make sense when it's an unlikely() for an
error case, not for something that could potentially be quite
the opposite of an unlikely case.

-- 
Jens Axboe

Re: [PATCH] null_blk: add unlikely for REQ_OP_DISACRD handling

[Chaitanya Kulkarni]

On 7/3/19 6:39 AM, Jens Axboe wrote:
> We should let normal branch prediction handle this. What if you
> are running a pure discard workload?

Hmm, I'm wasn't aware of such workload especially for null_blk where
disacard

is being used with memory back-end, I'll keep in mind from next time.

> In general I'm not a huge fan of likely/unlikely annotations,
> they only tend to make sense when it's an unlikely() for an
> error case, not for something that could potentially be quite
> the opposite of an unlikely case.

Make sense to drop this patch and future such usage of likely()/unlikely()

usage. Thanks for the clarification.

>
> -- Jens Axboe