| Message ID | 20190702032027.24066-1-chaitanya.kulkarni@wdc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | null_blk: add unlikely for REQ_OP_DISACRD handling | expand |
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!
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.
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
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;
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(-)