On Tue, May 1, 2018 at 6:20 PM, Doug Ledford <dledford@xxxxxxxxxx> wrote: > 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. Hi Sagi and Christoph, Guys, could you please comment on this patch set? These patches were created because of some later Sagi comments on IBTRS code, where I repeat the same dev_find_or_add() pattern, which is common for nvme and iser. So this is an attempt to deduplicate a bit and then reuse the API. Indeed, this change does not bring a lot in terms of LoC, but e.g. for IBTRS it simplifies the stuff and I just want to decide can I use this API or I should move those functions back to ibtrs-core module, without touching infiniband/core/. Thanks. -- Roman -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
