* [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 a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).