> -----Original Message----- > From: linux-rdma-owner@xxxxxxxxxxxxxxx <linux-rdma- > owner@xxxxxxxxxxxxxxx> On Behalf Of Jason Gunthorpe > Sent: Tuesday, November 6, 2018 1:37 PM > To: Leon Romanovsky <leon@xxxxxxxxxx> > Cc: Parav Pandit <parav@xxxxxxxxxxxx>; Kamal Heib > <kamalheib1@xxxxxxxxx>; Doug Ledford <dledford@xxxxxxxxxx>; linux- > rdma@xxxxxxxxxxxxxxx > Subject: Re: [PATCH rdma-next v3 00/20] RDMA: Add support for > ib_device_ops > > On Tue, Nov 06, 2018 at 08:41:54PM +0200, Leon Romanovsky wrote: > > > > Sorry for the late comment for Jason's point in v2 series to have > > > one giant structure. > > > > > > With one giant structure, fill_res_entry() provider callback > > > function of rdma_restrack_root doesn't deserve to be inside a > > > logically well separate structure rdma_restrack_root. We should > > > take out fill_res_entry() and put into ib_device_ops. > > This is possibly true.. It is cleaner for drivers if they don't have to reach into > all sorts of different places to put function pointers. > :-) those all sorts of places are logical places. > > > But I like the approach of rdma_restrack_root which keeps all > > > logical entities together. And with that good example I continue to > > > propose same logical separate for rest of the callbacks too. > > > With that non hot control path routines can possibly stay outside of > > > ib_device apart from code readability. > > > > Second for that, > > It will also give us a chance to optimize for cache alignment for such > > paths. > > As long as the function pointers are stored in memory allocated as part of > struct ib_device, adding more structs will not help performance. The usual > approach of sorting hot data together into common cache lines is > appropriate here. That is the point, the whole structure doesn't stay inside the ib_device. It should be something similar to netdev's, const struct dcbnl_rtnl_ops *dcbnl_ops; const struct rtnl_link_ops *rtnl_link_ops; Fairly large number of function pointers will move out for vfs, stats, gids etc. > > Kamal sorted by functionality, which is not necessarily great, it would be > better to sort based on some profiling, but our prior list wasn't optimized > either.. > > > Plus review will be much cleaner, we will be able to easily spot the > > need of performance numbers for changes in those flows. > > I think having micro structures would make a huge mess of the drivers. > > Every driver would have to individually split all the ops (across all the > variations) into a huge number of micro structs and juggle all of that. So > unnecessary and complicated.. > Most of them will go as const. For hns driver there are several instances of ib_device_ops created just for 2 functions in it to make use of this new API. Micro structures would be equally clean though after looking at hns driver multiple structures, but I am fine if you want to continue with just one structure. > The only value I see in this series is that it makes the drivers much simpler. I > would not throw that way for micro-structures.. > > Jason
