On Wed, May 2, 2018 at 12:10 PM, Max Gurtovoy <maxg@xxxxxxxxxxxx> wrote: > > > On 5/1/2018 7:20 PM, Doug Ledford 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. > > > I'm sorry, I also don't think it saves us a lot. I guess each ULP that is > using ib_device will always wrap it with some ulp_device structure with it's > own functionality. > > -Max. Not all ulp devices need their own device structures, e.g. NVMe host can simply reuses this ib_pool_device. Our ULP IBTRS module (both sides: host, target) also does not create new uld_device structure (moreover, I even do not need to provide any callbacks on target side, which of course saves some locs), so it depends on the task, of course. -- 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
