* [PATCH for-next 0/4] Misc update for RNBD @ 2021-04-28 6:13 Gioh Kim 2021-04-28 6:13 ` [PATCH for-next 1/4] block/rnbd-clt: Change queue_depth type in rnbd_clt_session to size_t Gioh Kim ` (4 more replies) 0 siblings, 5 replies; 12+ messages in thread From: Gioh Kim @ 2021-04-28 6:13 UTC (permalink / raw) To: linux-block Cc: axboe, hch, sagi, bvanassche, haris.iqbal, jinpu.wang, Gioh Kim Some misc updates for RNBD: * Remove unnecessary likely/unlikely macro from if-else statement * Fix coding style issues reported by checkpatch.pl * Add error check Dima Stepanov (1): block/rnbd: Fix style issues Gioh Kim (1): block/rnbd: Remove all likely and unlikely Md Haris Iqbal (2): block/rnbd-clt: Change queue_depth type in rnbd_clt_session to size_t block/rnbd-clt: Check the return value of the function rtrs_clt_query drivers/block/rnbd/rnbd-clt.c | 46 ++++++++++++++++++++--------------- drivers/block/rnbd/rnbd-clt.h | 2 +- drivers/block/rnbd/rnbd-srv.c | 2 +- 3 files changed, 29 insertions(+), 21 deletions(-) -- 2.25.1 ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH for-next 1/4] block/rnbd-clt: Change queue_depth type in rnbd_clt_session to size_t 2021-04-28 6:13 [PATCH for-next 0/4] Misc update for RNBD Gioh Kim @ 2021-04-28 6:13 ` Gioh Kim 2021-04-28 18:27 ` Chaitanya Kulkarni 2021-04-28 6:13 ` [PATCH for-next 2/4] block/rnbd: Fix style issues Gioh Kim ` (3 subsequent siblings) 4 siblings, 1 reply; 12+ messages in thread From: Gioh Kim @ 2021-04-28 6:13 UTC (permalink / raw) To: linux-block Cc: axboe, hch, sagi, bvanassche, haris.iqbal, jinpu.wang, Md Haris Iqbal, Guoqing Jiang, Gioh Kim From: Md Haris Iqbal <haris.iqbal@cloud.ionos.com> The member queue_depth in the structure rnbd_clt_session is read from the rtrs client side using the function rtrs_clt_query, which in turn is read from the rtrs_clt structure. It should really be of type size_t. Fixes: 90426e89f54db ("block/rnbd: client: private header with client structs and functions") Signed-off-by: Md Haris Iqbal <haris.iqbal@ionos.com> Reviewed-by: Guoqing Jiang <guoqing.jiang@ionos.com> Signed-off-by: Gioh Kim <gi-oh.kim@ionos.com> --- drivers/block/rnbd/rnbd-clt.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/block/rnbd/rnbd-clt.h b/drivers/block/rnbd/rnbd-clt.h index 451e7383738f..b5322c5aaac0 100644 --- a/drivers/block/rnbd/rnbd-clt.h +++ b/drivers/block/rnbd/rnbd-clt.h @@ -87,7 +87,7 @@ struct rnbd_clt_session { DECLARE_BITMAP(cpu_queues_bm, NR_CPUS); int __percpu *cpu_rr; /* per-cpu var for CPU round-robin */ atomic_t busy; - int queue_depth; + size_t queue_depth; u32 max_io_size; struct blk_mq_tag_set tag_set; u32 nr_poll_queues; -- 2.25.1 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH for-next 1/4] block/rnbd-clt: Change queue_depth type in rnbd_clt_session to size_t 2021-04-28 6:13 ` [PATCH for-next 1/4] block/rnbd-clt: Change queue_depth type in rnbd_clt_session to size_t Gioh Kim @ 2021-04-28 18:27 ` Chaitanya Kulkarni 0 siblings, 0 replies; 12+ messages in thread From: Chaitanya Kulkarni @ 2021-04-28 18:27 UTC (permalink / raw) To: Gioh Kim, linux-block Cc: axboe, hch, sagi, bvanassche, haris.iqbal, jinpu.wang, Md Haris Iqbal, Guoqing Jiang On 4/27/21 23:14, Gioh Kim wrote: > From: Md Haris Iqbal <haris.iqbal@cloud.ionos.com> > > The member queue_depth in the structure rnbd_clt_session is read from the > rtrs client side using the function rtrs_clt_query, which in turn is read > from the rtrs_clt structure. It should really be of type size_t. > > Fixes: 90426e89f54db ("block/rnbd: client: private header with client structs and functions") > Signed-off-by: Md Haris Iqbal <haris.iqbal@ionos.com> > Reviewed-by: Guoqing Jiang <guoqing.jiang@ionos.com> > Signed-off-by: Gioh Kim <gi-oh.kim@ionos.com> > --- Looks good. Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com> ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH for-next 2/4] block/rnbd: Fix style issues 2021-04-28 6:13 [PATCH for-next 0/4] Misc update for RNBD Gioh Kim 2021-04-28 6:13 ` [PATCH for-next 1/4] block/rnbd-clt: Change queue_depth type in rnbd_clt_session to size_t Gioh Kim @ 2021-04-28 6:13 ` Gioh Kim 2021-04-28 18:27 ` Chaitanya Kulkarni 2021-04-28 6:13 ` [PATCH for-next 3/4] block/rnbd-clt: Check the return value of the function rtrs_clt_query Gioh Kim ` (2 subsequent siblings) 4 siblings, 1 reply; 12+ messages in thread From: Gioh Kim @ 2021-04-28 6:13 UTC (permalink / raw) To: linux-block Cc: axboe, hch, sagi, bvanassche, haris.iqbal, jinpu.wang, Dima Stepanov, Dima Stepanov, Gioh Kim From: Dima Stepanov <dmitrii.stepanov@cloud.ionos.com> This patch fixes some style issues detected by scripts/checkpatch.pl * Resolve spacing and tab issues * Remove extra braces in rnbd_get_iu * Use num_possible_cpus() instead of NR_CPUS in alloc_sess * Fix the comments styling in rnbd_queue_rq Signed-off-by: Dima Stepanov <dmitrii.stepanov@ionos.com> Signed-off-by: Gioh Kim <gi-oh.kim@ionos.com> Signed-off-by: Md Haris Iqbal <haris.iqbal@ionos.com> Signed-off-by: Jack Wang <jinpu.wang@ionos.com> --- drivers/block/rnbd/rnbd-clt.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/drivers/block/rnbd/rnbd-clt.c b/drivers/block/rnbd/rnbd-clt.c index c01786afe1b1..15f831159c31 100644 --- a/drivers/block/rnbd/rnbd-clt.c +++ b/drivers/block/rnbd/rnbd-clt.c @@ -88,7 +88,7 @@ static int rnbd_clt_set_dev_attr(struct rnbd_clt_dev *dev, dev->discard_alignment = le32_to_cpu(rsp->discard_alignment); dev->secure_discard = le16_to_cpu(rsp->secure_discard); dev->rotational = rsp->rotational; - dev->wc = !!(rsp->cache_policy & RNBD_WRITEBACK); + dev->wc = !!(rsp->cache_policy & RNBD_WRITEBACK); dev->fua = !!(rsp->cache_policy & RNBD_FUA); dev->max_hw_sectors = sess->max_io_size / SECTOR_SIZE; @@ -351,9 +351,8 @@ static struct rnbd_iu *rnbd_get_iu(struct rnbd_clt_session *sess, struct rtrs_permit *permit; iu = kzalloc(sizeof(*iu), GFP_KERNEL); - if (!iu) { + if (!iu) return NULL; - } permit = rnbd_get_permit(sess, con_type, wait); if (unlikely(!permit)) { @@ -805,7 +804,7 @@ static struct rnbd_clt_session *alloc_sess(const char *sessname) mutex_init(&sess->lock); INIT_LIST_HEAD(&sess->devs_list); INIT_LIST_HEAD(&sess->list); - bitmap_zero(sess->cpu_queues_bm, NR_CPUS); + bitmap_zero(sess->cpu_queues_bm, num_possible_cpus()); init_waitqueue_head(&sess->rtrs_waitq); refcount_set(&sess->refcount, 1); @@ -1148,7 +1147,8 @@ static blk_status_t rnbd_queue_rq(struct blk_mq_hw_ctx *hctx, iu->sgt.sgl = iu->first_sgl; err = sg_alloc_table_chained(&iu->sgt, /* Even-if the request has no segment, - * sglist must have one entry at least */ + * sglist must have one entry at least. + */ blk_rq_nr_phys_segments(rq) ? : 1, iu->sgt.sgl, RNBD_INLINE_SG_CNT); -- 2.25.1 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH for-next 2/4] block/rnbd: Fix style issues 2021-04-28 6:13 ` [PATCH for-next 2/4] block/rnbd: Fix style issues Gioh Kim @ 2021-04-28 18:27 ` Chaitanya Kulkarni 0 siblings, 0 replies; 12+ messages in thread From: Chaitanya Kulkarni @ 2021-04-28 18:27 UTC (permalink / raw) To: Gioh Kim, linux-block Cc: axboe, hch, sagi, bvanassche, haris.iqbal, jinpu.wang, Dima Stepanov, Dima Stepanov On 4/27/21 23:14, Gioh Kim wrote: > From: Dima Stepanov <dmitrii.stepanov@cloud.ionos.com> > > This patch fixes some style issues detected by scripts/checkpatch.pl > * Resolve spacing and tab issues > * Remove extra braces in rnbd_get_iu > * Use num_possible_cpus() instead of NR_CPUS in alloc_sess > * Fix the comments styling in rnbd_queue_rq > > Signed-off-by: Dima Stepanov <dmitrii.stepanov@ionos.com> > Signed-off-by: Gioh Kim <gi-oh.kim@ionos.com> > Signed-off-by: Md Haris Iqbal <haris.iqbal@ionos.com> > Signed-off-by: Jack Wang <jinpu.wang@ionos.com> Looks good. Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com> ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH for-next 3/4] block/rnbd-clt: Check the return value of the function rtrs_clt_query 2021-04-28 6:13 [PATCH for-next 0/4] Misc update for RNBD Gioh Kim 2021-04-28 6:13 ` [PATCH for-next 1/4] block/rnbd-clt: Change queue_depth type in rnbd_clt_session to size_t Gioh Kim 2021-04-28 6:13 ` [PATCH for-next 2/4] block/rnbd: Fix style issues Gioh Kim @ 2021-04-28 6:13 ` Gioh Kim 2021-04-28 6:13 ` [PATCH for-next 4/4] block/rnbd: Remove all likely and unlikely Gioh Kim 2021-04-28 14:03 ` [PATCH for-next 0/4] Misc update for RNBD Jens Axboe 4 siblings, 0 replies; 12+ messages in thread From: Gioh Kim @ 2021-04-28 6:13 UTC (permalink / raw) To: linux-block Cc: axboe, hch, sagi, bvanassche, haris.iqbal, jinpu.wang, Md Haris Iqbal, Guoqing Jiang, Gioh Kim From: Md Haris Iqbal <haris.iqbal@cloud.ionos.com> In case none of the paths are in connected state, the function rtrs_clt_query returns an error. In such a case, error out since the values in the rtrs_attrs structure would be garbage. Fixes: f7a7a5c228d45 ("block/rnbd: client: main functionality") Signed-off-by: Md Haris Iqbal <haris.iqbal@ionos.com> Reviewed-by: Guoqing Jiang <guoqing.jiang@ionos.com> Signed-off-by: Jack Wang <jinpu.wang@ionos.com> Signed-off-by: Gioh Kim <gi-oh.kim@ionos.com> --- drivers/block/rnbd/rnbd-clt.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/drivers/block/rnbd/rnbd-clt.c b/drivers/block/rnbd/rnbd-clt.c index 15f831159c31..f855bf1fa8d5 100644 --- a/drivers/block/rnbd/rnbd-clt.c +++ b/drivers/block/rnbd/rnbd-clt.c @@ -691,7 +691,11 @@ static void remap_devs(struct rnbd_clt_session *sess) return; } - rtrs_clt_query(sess->rtrs, &attrs); + err = rtrs_clt_query(sess->rtrs, &attrs); + if (err) { + pr_err("rtrs_clt_query(\"%s\"): %d\n", sess->sessname, err); + return; + } mutex_lock(&sess->lock); sess->max_io_size = attrs.max_io_size; @@ -1294,7 +1298,11 @@ find_and_get_or_create_sess(const char *sessname, err = PTR_ERR(sess->rtrs); goto wake_up_and_put; } - rtrs_clt_query(sess->rtrs, &attrs); + + err = rtrs_clt_query(sess->rtrs, &attrs); + if (err) + goto close_rtrs; + sess->max_io_size = attrs.max_io_size; sess->queue_depth = attrs.queue_depth; sess->nr_poll_queues = nr_poll_queues; -- 2.25.1 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH for-next 4/4] block/rnbd: Remove all likely and unlikely 2021-04-28 6:13 [PATCH for-next 0/4] Misc update for RNBD Gioh Kim ` (2 preceding siblings ...) 2021-04-28 6:13 ` [PATCH for-next 3/4] block/rnbd-clt: Check the return value of the function rtrs_clt_query Gioh Kim @ 2021-04-28 6:13 ` Gioh Kim 2021-04-28 18:33 ` Chaitanya Kulkarni 2021-04-28 14:03 ` [PATCH for-next 0/4] Misc update for RNBD Jens Axboe 4 siblings, 1 reply; 12+ messages in thread From: Gioh Kim @ 2021-04-28 6:13 UTC (permalink / raw) To: linux-block Cc: axboe, hch, sagi, bvanassche, haris.iqbal, jinpu.wang, Gioh Kim The IO performance test with fio after removing the likely and unlikely macros in all if-statement shows no performance drop. They do not help for the performance of rnbd. The fio test did random read on 32 rnbd devices and 64 processes. Test environment: - AMD Opteron(tm) Processor 6386 SE - 125G memory - kernel version: 5.4.86 - gcc version: gcc (Debian 8.3.0-6) 8.3.0 - Infiniband controller: InfiniBand: Mellanox Technologies MT26428 [ConnectX VPI PCIe 2.0 5GT/s - IB QDR / 10GigE] (rev b0) before read: IOPS=549k, BW=2146MiB/s read: IOPS=544k, BW=2125MiB/s read: IOPS=553k, BW=2158MiB/s read: IOPS=535k, BW=2089MiB/s read: IOPS=543k, BW=2122MiB/s read: IOPS=552k, BW=2154MiB/s average: IOPS=546k, BW=2132MiB/s after read: IOPS=556k, BW=2172MiB/s read: IOPS=561k, BW=2191MiB/s read: IOPS=552k, BW=2156MiB/s read: IOPS=551k, BW=2154MiB/s read: IOPS=562k, BW=2194MiB/s ----------- average: IOPS=556k, BW=2173MiB/s The IOPS and bandwidth got better slightly after removing likely/unlikely. (IOPS= +1.8% BW= +1.9%) But we cannot make sure that removing the likely/unlikely help the performance because it depends on various situations. We only make sure that removing the likely/unlikely does not drop the performance. Signed-off-by: Gioh Kim <gi-oh.kim@ionos.com> Reviewed-by: Md Haris Iqbal <haris.iqbal@ionos.com> --- drivers/block/rnbd/rnbd-clt.c | 24 ++++++++++++------------ drivers/block/rnbd/rnbd-srv.c | 2 +- 2 files changed, 13 insertions(+), 13 deletions(-) diff --git a/drivers/block/rnbd/rnbd-clt.c b/drivers/block/rnbd/rnbd-clt.c index f855bf1fa8d5..c604a402cd5c 100644 --- a/drivers/block/rnbd/rnbd-clt.c +++ b/drivers/block/rnbd/rnbd-clt.c @@ -241,7 +241,7 @@ static bool rnbd_rerun_if_needed(struct rnbd_clt_session *sess) cpu_q = rnbd_get_cpu_qlist(sess, nxt_cpu(cpu_q->cpu))) { if (!spin_trylock_irqsave(&cpu_q->requeue_lock, flags)) continue; - if (unlikely(!test_bit(cpu_q->cpu, sess->cpu_queues_bm))) + if (!test_bit(cpu_q->cpu, sess->cpu_queues_bm)) goto unlock; q = list_first_entry_or_null(&cpu_q->requeue_list, typeof(*q), requeue_list); @@ -320,7 +320,7 @@ static struct rtrs_permit *rnbd_get_permit(struct rnbd_clt_session *sess, struct rtrs_permit *permit; permit = rtrs_clt_get_permit(sess->rtrs, con_type, wait); - if (likely(permit)) + if (permit) /* We have a subtle rare case here, when all permits can be * consumed before busy counter increased. This is safe, * because loser will get NULL as a permit, observe 0 busy @@ -355,7 +355,7 @@ static struct rnbd_iu *rnbd_get_iu(struct rnbd_clt_session *sess, return NULL; permit = rnbd_get_permit(sess, con_type, wait); - if (unlikely(!permit)) { + if (!permit) { kfree(iu); return NULL; } @@ -1050,7 +1050,7 @@ static int rnbd_client_xfer_request(struct rnbd_clt_dev *dev, }; err = rtrs_clt_request(rq_data_dir(rq), &req_ops, rtrs, permit, &vec, 1, size, iu->sgt.sgl, sg_cnt); - if (unlikely(err)) { + if (err) { rnbd_clt_err_rl(dev, "RTRS failed to transfer IO, err: %d\n", err); return err; @@ -1081,7 +1081,7 @@ static bool rnbd_clt_dev_add_to_requeue(struct rnbd_clt_dev *dev, cpu_q = get_cpu_ptr(sess->cpu_queues); spin_lock_irqsave(&cpu_q->requeue_lock, flags); - if (likely(!test_and_set_bit_lock(0, &q->in_list))) { + if (!test_and_set_bit_lock(0, &q->in_list)) { if (WARN_ON(!list_empty(&q->requeue_list))) goto unlock; @@ -1093,7 +1093,7 @@ static bool rnbd_clt_dev_add_to_requeue(struct rnbd_clt_dev *dev, */ smp_mb__before_atomic(); } - if (likely(atomic_read(&sess->busy))) { + if (atomic_read(&sess->busy)) { list_add_tail(&q->requeue_list, &cpu_q->requeue_list); } else { /* Very unlikely, but possible: busy counter was @@ -1121,7 +1121,7 @@ static void rnbd_clt_dev_kick_mq_queue(struct rnbd_clt_dev *dev, if (delay != RNBD_DELAY_IFBUSY) blk_mq_delay_run_hw_queue(hctx, delay); - else if (unlikely(!rnbd_clt_dev_add_to_requeue(dev, q))) + else if (!rnbd_clt_dev_add_to_requeue(dev, q)) /* * If session is not busy we have to restart * the queue ourselves. @@ -1138,12 +1138,12 @@ static blk_status_t rnbd_queue_rq(struct blk_mq_hw_ctx *hctx, int err; blk_status_t ret = BLK_STS_IOERR; - if (unlikely(dev->dev_state != DEV_STATE_MAPPED)) + if (dev->dev_state != DEV_STATE_MAPPED) return BLK_STS_IOERR; iu->permit = rnbd_get_permit(dev->sess, RTRS_IO_CON, RTRS_PERMIT_NOWAIT); - if (unlikely(!iu->permit)) { + if (!iu->permit) { rnbd_clt_dev_kick_mq_queue(dev, hctx, RNBD_DELAY_IFBUSY); return BLK_STS_RESOURCE; } @@ -1165,9 +1165,9 @@ static blk_status_t rnbd_queue_rq(struct blk_mq_hw_ctx *hctx, blk_mq_start_request(rq); err = rnbd_client_xfer_request(dev, rq, iu); - if (likely(err == 0)) + if (err == 0) return BLK_STS_OK; - if (unlikely(err == -EAGAIN || err == -ENOMEM)) { + if (err == -EAGAIN || err == -ENOMEM) { rnbd_clt_dev_kick_mq_queue(dev, hctx, 10/*ms*/); ret = BLK_STS_RESOURCE; } @@ -1584,7 +1584,7 @@ struct rnbd_clt_dev *rnbd_clt_map_device(const char *sessname, struct rnbd_clt_dev *dev; int ret; - if (unlikely(exists_devpath(pathname, sessname))) + if (exists_devpath(pathname, sessname)) return ERR_PTR(-EEXIST); sess = find_and_get_or_create_sess(sessname, paths, path_cnt, port_nr, nr_poll_queues); diff --git a/drivers/block/rnbd/rnbd-srv.c b/drivers/block/rnbd/rnbd-srv.c index 899dd9d7c10b..aafecfe97055 100644 --- a/drivers/block/rnbd/rnbd-srv.c +++ b/drivers/block/rnbd/rnbd-srv.c @@ -104,7 +104,7 @@ rnbd_get_sess_dev(int dev_id, struct rnbd_srv_session *srv_sess) rcu_read_lock(); sess_dev = xa_load(&srv_sess->index_idr, dev_id); - if (likely(sess_dev)) + if (sess_dev) ret = kref_get_unless_zero(&sess_dev->kref); rcu_read_unlock(); -- 2.25.1 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH for-next 4/4] block/rnbd: Remove all likely and unlikely 2021-04-28 6:13 ` [PATCH for-next 4/4] block/rnbd: Remove all likely and unlikely Gioh Kim @ 2021-04-28 18:33 ` Chaitanya Kulkarni 2021-04-29 7:14 ` Gioh Kim 0 siblings, 1 reply; 12+ messages in thread From: Chaitanya Kulkarni @ 2021-04-28 18:33 UTC (permalink / raw) To: Gioh Kim, linux-block Cc: axboe, hch, sagi, bvanassche, haris.iqbal, jinpu.wang On 4/27/21 23:14, Gioh Kim wrote: > The IO performance test with fio after removing the likely and > unlikely macros in all if-statement shows no performance drop. > They do not help for the performance of rnbd. > > The fio test did random read on 32 rnbd devices and 64 processes. > Test environment: > - AMD Opteron(tm) Processor 6386 SE > - 125G memory > - kernel version: 5.4.86 why 5.4 and not linux-block/for-next ? > - gcc version: gcc (Debian 8.3.0-6) 8.3.0 > - Infiniband controller: InfiniBand: Mellanox Technologies MT26428 > [ConnectX VPI PCIe 2.0 5GT/s - IB QDR / 10GigE] (rev b0) > > before > read: IOPS=549k, BW=2146MiB/s > read: IOPS=544k, BW=2125MiB/s > read: IOPS=553k, BW=2158MiB/s > read: IOPS=535k, BW=2089MiB/s > read: IOPS=543k, BW=2122MiB/s > read: IOPS=552k, BW=2154MiB/s > average: IOPS=546k, BW=2132MiB/s > > after > read: IOPS=556k, BW=2172MiB/s > read: IOPS=561k, BW=2191MiB/s > read: IOPS=552k, BW=2156MiB/s > read: IOPS=551k, BW=2154MiB/s > read: IOPS=562k, BW=2194MiB/s > ----------- > average: IOPS=556k, BW=2173MiB/s > > The IOPS and bandwidth got better slightly after removing > likely/unlikely. (IOPS= +1.8% BW= +1.9%) But we cannot make sure > that removing the likely/unlikely help the performance because it > depends on various situations. We only make sure that removing the > likely/unlikely does not drop the performance. Did you get a chance to collect perf numbers to see which functions are getting faster ? ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH for-next 4/4] block/rnbd: Remove all likely and unlikely 2021-04-28 18:33 ` Chaitanya Kulkarni @ 2021-04-29 7:14 ` Gioh Kim 2021-05-04 13:04 ` Gioh Kim 0 siblings, 1 reply; 12+ messages in thread From: Gioh Kim @ 2021-04-29 7:14 UTC (permalink / raw) To: Chaitanya Kulkarni Cc: linux-block, axboe, hch, sagi, bvanassche, haris.iqbal, jinpu.wang On Wed, Apr 28, 2021 at 8:33 PM Chaitanya Kulkarni <Chaitanya.Kulkarni@wdc.com> wrote: > > On 4/27/21 23:14, Gioh Kim wrote: > > The IO performance test with fio after removing the likely and > > unlikely macros in all if-statement shows no performance drop. > > They do not help for the performance of rnbd. > > > > The fio test did random read on 32 rnbd devices and 64 processes. > > Test environment: > > - AMD Opteron(tm) Processor 6386 SE > > - 125G memory > > - kernel version: 5.4.86 > > why 5.4 and not linux-block/for-next ? We have done porting only 5.4 for the server machine yet. > > > - gcc version: gcc (Debian 8.3.0-6) 8.3.0 > > - Infiniband controller: InfiniBand: Mellanox Technologies MT26428 > > [ConnectX VPI PCIe 2.0 5GT/s - IB QDR / 10GigE] (rev b0) > > > > before > > read: IOPS=549k, BW=2146MiB/s > > read: IOPS=544k, BW=2125MiB/s > > read: IOPS=553k, BW=2158MiB/s > > read: IOPS=535k, BW=2089MiB/s > > read: IOPS=543k, BW=2122MiB/s > > read: IOPS=552k, BW=2154MiB/s > > average: IOPS=546k, BW=2132MiB/s > > > > after > > read: IOPS=556k, BW=2172MiB/s > > read: IOPS=561k, BW=2191MiB/s > > read: IOPS=552k, BW=2156MiB/s > > read: IOPS=551k, BW=2154MiB/s > > read: IOPS=562k, BW=2194MiB/s > > ----------- > > average: IOPS=556k, BW=2173MiB/s > > > > The IOPS and bandwidth got better slightly after removing > > likely/unlikely. (IOPS= +1.8% BW= +1.9%) But we cannot make sure > > that removing the likely/unlikely help the performance because it > > depends on various situations. We only make sure that removing the > > likely/unlikely does not drop the performance. > > Did you get a chance to collect perf numbers to see which functions are > getting faster ? I knew somebody would ask for it ;-) No, I didn't because I have been occupied with another task. But I will check it soon in a few weeks. Thank you for the review. > > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH for-next 4/4] block/rnbd: Remove all likely and unlikely 2021-04-29 7:14 ` Gioh Kim @ 2021-05-04 13:04 ` Gioh Kim 2021-05-05 13:12 ` Gioh Kim 0 siblings, 1 reply; 12+ messages in thread From: Gioh Kim @ 2021-05-04 13:04 UTC (permalink / raw) To: Chaitanya Kulkarni Cc: linux-block, axboe, hch, sagi, bvanassche, haris.iqbal, jinpu.wang On Thu, Apr 29, 2021 at 9:14 AM Gioh Kim <gi-oh.kim@ionos.com> wrote: > > On Wed, Apr 28, 2021 at 8:33 PM Chaitanya Kulkarni > <Chaitanya.Kulkarni@wdc.com> wrote: > > > > On 4/27/21 23:14, Gioh Kim wrote: > > > The IO performance test with fio after removing the likely and > > > unlikely macros in all if-statement shows no performance drop. > > > They do not help for the performance of rnbd. > > > > > > The fio test did random read on 32 rnbd devices and 64 processes. > > > Test environment: > > > - AMD Opteron(tm) Processor 6386 SE > > > - 125G memory > > > - kernel version: 5.4.86 > > > > why 5.4 and not linux-block/for-next ? > > We have done porting only 5.4 for the server machine yet. > > > > > > - gcc version: gcc (Debian 8.3.0-6) 8.3.0 > > > - Infiniband controller: InfiniBand: Mellanox Technologies MT26428 > > > [ConnectX VPI PCIe 2.0 5GT/s - IB QDR / 10GigE] (rev b0) > > > > > > before > > > read: IOPS=549k, BW=2146MiB/s > > > read: IOPS=544k, BW=2125MiB/s > > > read: IOPS=553k, BW=2158MiB/s > > > read: IOPS=535k, BW=2089MiB/s > > > read: IOPS=543k, BW=2122MiB/s > > > read: IOPS=552k, BW=2154MiB/s > > > average: IOPS=546k, BW=2132MiB/s > > > > > > after > > > read: IOPS=556k, BW=2172MiB/s > > > read: IOPS=561k, BW=2191MiB/s > > > read: IOPS=552k, BW=2156MiB/s > > > read: IOPS=551k, BW=2154MiB/s > > > read: IOPS=562k, BW=2194MiB/s > > > ----------- > > > average: IOPS=556k, BW=2173MiB/s > > > > > > The IOPS and bandwidth got better slightly after removing > > > likely/unlikely. (IOPS= +1.8% BW= +1.9%) But we cannot make sure > > > that removing the likely/unlikely help the performance because it > > > depends on various situations. We only make sure that removing the > > > likely/unlikely does not drop the performance. > > > > Did you get a chance to collect perf numbers to see which functions are > > getting faster ? Hi Chaitanya, I ran the perf tool to find out which functions are getting faster. But I was not able to find it. Could you please suggest a tool or anything to check it out? For your information, below is what I got with 'perf record fio <options:8-device, 64-job, 60-second>' The result before/after removing likely/unlikely looks the same. 4.15% fio [kernel.kallsyms] [k] _raw_spin_lock_irqsave 3.19% fio [kernel.kallsyms] [k] x86_pmu_disable_all 2.98% fio [rnbd_client] [k] rnbd_put_permit 2.77% fio [kernel.kallsyms] [k] find_first_zero_bit 2.49% fio [kernel.kallsyms] [k] __x86_indirect_thunk_rax 2.21% fio [kernel.kallsyms] [k] psi_task_change 2.00% fio [kernel.kallsyms] [k] gup_pgd_range 1.83% fio fio [.] 0x0000000000029048 1.78% fio [rnbd_client] [k] rnbd_get_permit 1.78% fio fio [.] axmap_isset 1.63% fio [kernel.kallsyms] [k] _raw_spin_lock 1.58% fio fio [.] fio_gettime 1.53% fio [rtrs_client] [k] __rtrs_get_permit 1.51% fio [rnbd_client] [k] rnbd_queue_rq 1.51% fio [rtrs_client] [k] rtrs_clt_put_permit 1.47% fio [kernel.kallsyms] [k] try_to_wake_up 1.31% fio [kernel.kallsyms] [k] kmem_cache_alloc 1.22% fio libc-2.28.so [.] 0x00000000000a2547 1.17% fio [mlx4_ib] [k] _mlx4_ib_post_send 1.14% fio [kernel.kallsyms] [k] blkdev_direct_IO 1.14% fio [kernel.kallsyms] [k] read_tsc 1.02% fio [rtrs_client] [k] rtrs_clt_read_req 0.92% fio [rtrs_client] [k] get_next_path_min_inflight 0.92% fio [kernel.kallsyms] [k] sched_clock 0.91% fio [kernel.kallsyms] [k] blk_mq_get_request 0.87% fio [kernel.kallsyms] [k] x86_pmu_enable_all 0.87% fio [kernel.kallsyms] [k] __sched_text_start 0.84% fio [kernel.kallsyms] [k] insert_work 0.82% fio [kernel.kallsyms] [k] copy_user_generic_string 0.80% fio [kernel.kallsyms] [k] blk_attempt_plug_merge 0.73% fio [rtrs_client] [k] rtrs_clt_update_all_stats > > I knew somebody would ask for it ;-) > No, I didn't because I have been occupied with another task. > But I will check it soon in a few weeks. > > Thank you for the review. > > > > > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH for-next 4/4] block/rnbd: Remove all likely and unlikely 2021-05-04 13:04 ` Gioh Kim @ 2021-05-05 13:12 ` Gioh Kim 0 siblings, 0 replies; 12+ messages in thread From: Gioh Kim @ 2021-05-05 13:12 UTC (permalink / raw) To: Chaitanya Kulkarni Cc: linux-block, axboe, hch, sagi, bvanassche, haris.iqbal, jinpu.wang On Tue, May 4, 2021 at 3:04 PM Gioh Kim <gi-oh.kim@ionos.com> wrote: > > On Thu, Apr 29, 2021 at 9:14 AM Gioh Kim <gi-oh.kim@ionos.com> wrote: > > > > On Wed, Apr 28, 2021 at 8:33 PM Chaitanya Kulkarni > > <Chaitanya.Kulkarni@wdc.com> wrote: > > > > > > On 4/27/21 23:14, Gioh Kim wrote: > > > > The IO performance test with fio after removing the likely and > > > > unlikely macros in all if-statement shows no performance drop. > > > > They do not help for the performance of rnbd. > > > > > > > > The fio test did random read on 32 rnbd devices and 64 processes. > > > > Test environment: > > > > - AMD Opteron(tm) Processor 6386 SE > > > > - 125G memory > > > > - kernel version: 5.4.86 > > > > > > why 5.4 and not linux-block/for-next ? > > > > We have done porting only 5.4 for the server machine yet. > > > > > > > > > - gcc version: gcc (Debian 8.3.0-6) 8.3.0 > > > > - Infiniband controller: InfiniBand: Mellanox Technologies MT26428 > > > > [ConnectX VPI PCIe 2.0 5GT/s - IB QDR / 10GigE] (rev b0) > > > > > > > > before > > > > read: IOPS=549k, BW=2146MiB/s > > > > read: IOPS=544k, BW=2125MiB/s > > > > read: IOPS=553k, BW=2158MiB/s > > > > read: IOPS=535k, BW=2089MiB/s > > > > read: IOPS=543k, BW=2122MiB/s > > > > read: IOPS=552k, BW=2154MiB/s > > > > average: IOPS=546k, BW=2132MiB/s > > > > > > > > after > > > > read: IOPS=556k, BW=2172MiB/s > > > > read: IOPS=561k, BW=2191MiB/s > > > > read: IOPS=552k, BW=2156MiB/s > > > > read: IOPS=551k, BW=2154MiB/s > > > > read: IOPS=562k, BW=2194MiB/s > > > > ----------- > > > > average: IOPS=556k, BW=2173MiB/s > > > > > > > > The IOPS and bandwidth got better slightly after removing > > > > likely/unlikely. (IOPS= +1.8% BW= +1.9%) But we cannot make sure > > > > that removing the likely/unlikely help the performance because it > > > > depends on various situations. We only make sure that removing the > > > > likely/unlikely does not drop the performance. > > > > > > Did you get a chance to collect perf numbers to see which functions are > > > getting faster ? > > Hi Chaitanya, > > I ran the perf tool to find out which functions are getting faster. > But I was not able to find it. > Could you please suggest a tool or anything to check it out? > > For your information, below is what I got with 'perf record fio > <options:8-device, 64-job, 60-second>' > The result before/after removing likely/unlikely looks the same. > > 4.15% fio [kernel.kallsyms] [k] _raw_spin_lock_irqsave > 3.19% fio [kernel.kallsyms] [k] x86_pmu_disable_all > 2.98% fio [rnbd_client] [k] rnbd_put_permit > 2.77% fio [kernel.kallsyms] [k] find_first_zero_bit > 2.49% fio [kernel.kallsyms] [k] __x86_indirect_thunk_rax > 2.21% fio [kernel.kallsyms] [k] psi_task_change > 2.00% fio [kernel.kallsyms] [k] gup_pgd_range > 1.83% fio fio [.] 0x0000000000029048 > 1.78% fio [rnbd_client] [k] rnbd_get_permit > 1.78% fio fio [.] axmap_isset > 1.63% fio [kernel.kallsyms] [k] _raw_spin_lock > 1.58% fio fio [.] fio_gettime > 1.53% fio [rtrs_client] [k] __rtrs_get_permit > 1.51% fio [rnbd_client] [k] rnbd_queue_rq > 1.51% fio [rtrs_client] [k] rtrs_clt_put_permit > 1.47% fio [kernel.kallsyms] [k] try_to_wake_up > 1.31% fio [kernel.kallsyms] [k] kmem_cache_alloc > 1.22% fio libc-2.28.so [.] 0x00000000000a2547 > 1.17% fio [mlx4_ib] [k] _mlx4_ib_post_send > 1.14% fio [kernel.kallsyms] [k] blkdev_direct_IO > 1.14% fio [kernel.kallsyms] [k] read_tsc > 1.02% fio [rtrs_client] [k] rtrs_clt_read_req > 0.92% fio [rtrs_client] [k] get_next_path_min_inflight > 0.92% fio [kernel.kallsyms] [k] sched_clock > 0.91% fio [kernel.kallsyms] [k] blk_mq_get_request > 0.87% fio [kernel.kallsyms] [k] x86_pmu_enable_all > 0.87% fio [kernel.kallsyms] [k] __sched_text_start > 0.84% fio [kernel.kallsyms] [k] insert_work > 0.82% fio [kernel.kallsyms] [k] copy_user_generic_string > 0.80% fio [kernel.kallsyms] [k] blk_attempt_plug_merge > 0.73% fio [rtrs_client] [k] rtrs_clt_update_all_stats > Hi Chaitanya, I think likely/unlikely macros are related to cache and branch prediction. So I checked cache and branch misses with perf tool. The result are same before/after removing likely/unlikely - cache misses: after 5,452%, before 5,443% - branch misses: after 2.08%, before 2.09% I would appreciate it if you would suggest something else for me to check. Below is the raw data that I got from the perf tool. after removing likely: Performance counter stats for 'fio --direct=1 --rw=randread --time_based=1 --group_reporting --ioengine=libaio --iodepth=128 --name=fiotest --fadvise_hint=0 --iodepth_batch_submit=128 --iodepth_batch_complete=128 --invalidate=0 --runtime=180 --numjobs=64 --filename=/dev/rnbd0 --filename=/dev/rnbd1 --filename=/dev/rnbd2 --filename=/dev/rnbd3 --filename=/dev/rnbd4 --filename=/dev/rnbd5 --filename=/dev/rnbd6 --filename=/dev/rnbd7 --filename=/dev/rnbd8 --filename=/dev/rnbd9 --filename=/dev/rnbd10 --filename=/dev/rnbd11 --filename=/dev/rnbd12 --filename=/dev/rnbd13 --filename=/dev/rnbd14 --filename=/dev/rnbd15 --filename=/dev/rnbd16 --filename=/dev/rnbd17 --filename=/dev/rnbd18 --filename=/dev/rnbd19 --filename=/dev/rnbd20 --filename=/dev/rnbd21 --filename=/dev/rnbd22 --filename=/dev/rnbd23 --filename=/dev/rnbd24 --filename=/dev/rnbd25 --filename=/dev/rnbd26 --filename=/dev/rnbd27 --filename=/dev/rnbd28 --filename=/dev/rnbd29 --filename=/dev/rnbd30 --filename=/dev/rnbd31': 1.834.487,82 msec task-clock # 9,986 CPUs utilized 3.128.339.845.336 cycles # 1,705 GHz (66,53%) 1.110.316.024.909 instructions # 0,35 insn per cycle (83,27%) 76.626.760.535 cache-references # 41,770 M/sec (83,26%) 4.177.366.104 cache-misses # 5,452 % of all cache refs (50,21%) 224.055.600.184 branches # 122,135 M/sec (66,85%) 4.669.404.288 branch-misses # 2,08% of all branches (83,38%) 183,707988693 seconds time elapsed 185,630125000 seconds user 1590,286666000 seconds sys before removing: Performance counter stats for 'fio --direct=1 --rw=randread --time_based=1 --group_reporting --ioengine=libaio --iodepth=128 --name=fiotest --fadvise_hint=0 --iodepth_batch_submit=128 --iodepth_batch_complete=128 --invalidate=0 --runtime=180 --numjobs=64 --filename=/dev/rnbd0 --filename=/dev/rnbd1 --filename=/dev/rnbd2 --filename=/dev/rnbd3 --filename=/dev/rnbd4 --filename=/dev/rnbd5 --filename=/dev/rnbd6 --filename=/dev/rnbd7 --filename=/dev/rnbd8 --filename=/dev/rnbd9 --filename=/dev/rnbd10 --filename=/dev/rnbd11 --filename=/dev/rnbd12 --filename=/dev/rnbd13 --filename=/dev/rnbd14 --filename=/dev/rnbd15 --filename=/dev/rnbd16 --filename=/dev/rnbd17 --filename=/dev/rnbd18 --filename=/dev/rnbd19 --filename=/dev/rnbd20 --filename=/dev/rnbd21 --filename=/dev/rnbd22 --filename=/dev/rnbd23 --filename=/dev/rnbd24 --filename=/dev/rnbd25 --filename=/dev/rnbd26 --filename=/dev/rnbd27 --filename=/dev/rnbd28 --filename=/dev/rnbd29 --filename=/dev/rnbd30 --filename=/dev/rnbd31': 1.841.874,78 msec task-clock # 10,039 CPUs utilized 3.157.131.978.349 cycles # 1,714 GHz (66,48%) 1.115.369.402.018 instructions # 0,35 insn per cycle (83,27%) 77.060.091.803 cache-references # 41,838 M/sec (83,39%) 4.194.110.754 cache-misses # 5,443 % of all cache refs (50,13%) 225.304.135.864 branches # 122,323 M/sec (66,83%) 4.716.162.562 branch-misses # 2,09% of all branches (83,42%) 183,476417386 seconds time elapsed 185,356439000 seconds user 1596,787284000 seconds sys > > > > > I knew somebody would ask for it ;-) > > No, I didn't because I have been occupied with another task. > > But I will check it soon in a few weeks. > > > > Thank you for the review. > > > > > > > > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH for-next 0/4] Misc update for RNBD 2021-04-28 6:13 [PATCH for-next 0/4] Misc update for RNBD Gioh Kim ` (3 preceding siblings ...) 2021-04-28 6:13 ` [PATCH for-next 4/4] block/rnbd: Remove all likely and unlikely Gioh Kim @ 2021-04-28 14:03 ` Jens Axboe 4 siblings, 0 replies; 12+ messages in thread From: Jens Axboe @ 2021-04-28 14:03 UTC (permalink / raw) To: Gioh Kim, linux-block; +Cc: hch, sagi, bvanassche, haris.iqbal, jinpu.wang On 4/28/21 12:13 AM, Gioh Kim wrote: > Some misc updates for RNBD: > * Remove unnecessary likely/unlikely macro from if-else statement > * Fix coding style issues reported by checkpatch.pl > * Add error check Applied for post initial merge, thanks. -- Jens Axboe ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2021-05-05 13:13 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-04-28 6:13 [PATCH for-next 0/4] Misc update for RNBD Gioh Kim 2021-04-28 6:13 ` [PATCH for-next 1/4] block/rnbd-clt: Change queue_depth type in rnbd_clt_session to size_t Gioh Kim 2021-04-28 18:27 ` Chaitanya Kulkarni 2021-04-28 6:13 ` [PATCH for-next 2/4] block/rnbd: Fix style issues Gioh Kim 2021-04-28 18:27 ` Chaitanya Kulkarni 2021-04-28 6:13 ` [PATCH for-next 3/4] block/rnbd-clt: Check the return value of the function rtrs_clt_query Gioh Kim 2021-04-28 6:13 ` [PATCH for-next 4/4] block/rnbd: Remove all likely and unlikely Gioh Kim 2021-04-28 18:33 ` Chaitanya Kulkarni 2021-04-29 7:14 ` Gioh Kim 2021-05-04 13:04 ` Gioh Kim 2021-05-05 13:12 ` Gioh Kim 2021-04-28 14:03 ` [PATCH for-next 0/4] Misc update for RNBD Jens Axboe
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.