null_blk: add unlikely for REQ_OP_DISACRD handling
diff mbox series

Message ID 20190702032027.24066-1-chaitanya.kulkarni@wdc.com
State New
Headers show
Series
  • null_blk: add unlikely for REQ_OP_DISACRD handling
Related show

Commit Message

Chaitanya Kulkarni July 2, 2019, 3:20 a.m. UTC
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(-)

Comments

Minwoo Im July 3, 2019, 7:17 a.m. UTC | #1
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!
Jens Axboe July 3, 2019, 1:39 p.m. UTC | #2
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.
Chaitanya Kulkarni July 4, 2019, 12:14 a.m. UTC | #3
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

Patch
diff mbox series

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;