On Mon, 2018-04-30 at 09:27 +0200, Roman Pen wrote:
> From: Roman Pen <r.peniaev@xxxxxxxxx>
>
> Hi all,
>
> This is an attempt to make a device pool API out of a common code,
> which caches pair of ib_device and ib_pd pointers. I found 4 places,
> where this common functionality can be replaced by some lib calls:
> nvme, nvmet, iser and isert. Total deduplication gain in loc is not
> quite significant, but eventually new ULP IB code can also require
> the same device/pd pair cache, e.g. in our IBTRS module the same
> code has to be repeated again, which was observed by Sagi and he
> suggested to make a common helper function instead of producing
> another copy.
I was hoping for more dedup of LoC. Oh well. It still has value in
that it puts the locking and such in one place so we don't have as many
potential spots for bugs to creep in, but one way you usually measure
that impact is when you remove 100 lines of complex code and replace it
with 20 lines of simpler library calls. Because of the need for
callbacks, you don't get 20 lines of simpler library calls, you get 20
lines of simpler calls and then 60 lines of callbacks that reintroduce
complexity into that which was supposed to be simplified.
Anyway, I'm ambivalent on this one. If it simplified things more, I'd
be much more enthusiastic. In the end, I'll leave the decision up to
Sagi and Christoph. It's their code that this is modifying mainly. If
they think this qualifies as a worthwhile cleanup in their code, then
I'll take it in.
> Diff v1..v2:
> 1. Add pool->deinit callback in order to make code symmetric with
> init procedure and to handle error paths correctly.
> 2. Avoid BUG_ON, replaced with WARN_ON.
> 3. Rename ib_dev_pool_find_get_or_create() on ib_dev_pool_find_or_add()
> to make the name a bit shorter and nicer.
> 4. Return proper value from ib_pool_dev_exists().
>
> Roman Pen (5):
> IB/core: introduce an IB device pool
> nvme-rdma: switch on IB pool device interface
> nvmet-rdma: switch on IB pool device interface
> IB/iser: switch on IB pool device interface
> IB/isert: switch on IB pool device interface
>
> drivers/infiniband/core/Makefile | 2 +-
> drivers/infiniband/core/dev_pool.c | 131 ++++++++++++++++++
> drivers/infiniband/ulp/iser/iscsi_iser.c | 68 ++++++++--
> drivers/infiniband/ulp/iser/iscsi_iser.h | 21 ++-
> drivers/infiniband/ulp/iser/iser_initiator.c | 42 +++---
> drivers/infiniband/ulp/iser/iser_memory.c | 18 +--
> drivers/infiniband/ulp/iser/iser_verbs.c | 102 +++-----------
> drivers/infiniband/ulp/isert/ib_isert.c | 185 ++++++++++++-------------
> drivers/infiniband/ulp/isert/ib_isert.h | 7 +-
> drivers/nvme/host/rdma.c | 187 +++++++++----------------
> drivers/nvme/target/rdma.c | 196 ++++++++++++---------------
> include/rdma/dev_pool.h | 58 ++++++++
> 12 files changed, 547 insertions(+), 470 deletions(-)
> create mode 100644 drivers/infiniband/core/dev_pool.c
> create mode 100644 include/rdma/dev_pool.h
>
> Signed-off-by: Roman Pen <roman.penyaev@xxxxxxxxxxxxxxxx>
> Cc: Keith Busch <keith.busch@xxxxxxxxx>
> Cc: Jens Axboe <axboe@xxxxxx>
> Cc: Christoph Hellwig <hch@xxxxxx>
> Cc: Steve Wise <swise@xxxxxxxxxxxxxxxxxxxxx>
> Cc: Bart Van Assche <bart.vanassche@xxxxxxxxxxx>
> Cc: Sagi Grimberg <sagi@xxxxxxxxxxx>
> Cc: Doug Ledford <dledford@xxxxxxxxxx>
> Cc: Or Gerlitz <ogerlitz@xxxxxxxxxxxx>
>
--
Doug Ledford <dledford@xxxxxxxxxx>
GPG KeyID: B826A3330E572FDD
Key fingerprint = AE6B 1BDA 122B 23B4 265B 1274 B826 A333 0E57 2FDDAttachment:
signature.asc
Description: This is a digitally signed message part
