* [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
* [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
* [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 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
* 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
* 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
* 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
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).