On Wed, May 9, 2018 at 4:20 PM, Sagi Grimberg <sagi@xxxxxxxxxxx> wrote: > >>> 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/. > > > Hi Roman, > > Thanks for taking a stab at it! > > I was hoping this would simplify things even further but looking at this > I'm not sure it makes things any better. > > What I had in mind was something different. I would try and tie this > to ib_client interface. In this case, a ULP would create its device > representation in .add() and destroy it in .remove() callouts. For the > binding of a ULP device represntation to a ULP queue I would offer a > single generic lookup that takes cm_id->device and return the ib_client > class device (pointer to the ULP device). > > This way, no need for refcounting (comes for free) nor optional callouts... > Thoughts? Hi Sagi, Ah, right, I see. That is how is already implemented (e.g. rds/ib.c) using ib_client 'add|remove' and 'ib_(get|set)_client_data' calls. Nice, I've not noticed that before, that seems indeed much better for wrapping ib_device around. I will try to prepare some patches next week. -- 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
