> -----Original Message----- > From: Jason Gunthorpe <jgg@xxxxxxxx> > Sent: Wednesday, November 7, 2018 2:08 PM > To: Parav Pandit <parav@xxxxxxxxxxxx> > Cc: Leon Romanovsky <leon@xxxxxxxxxx>; 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:55:25PM +0000, Parav Pandit wrote: > > > 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. > > The size of the struct doesn't matter, the cacheline layout does, and as long > as the user of the ops can reasonably be expected to be touching cachelines > in the ib_device, it doesn't really matter. Adding more indirection to outside > tables is more likely to make things slower, as now we have to touch more > cachelines to get the same thing. > Indirection table only for non-data-path functions, rest will continue to be part of _ops. > I don't think any real performance claim can be made either way. > > > > > 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. > > This series already has const structs.. > > > 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. > > Yes, that is unfortunate, but that is still less structs over all then if there > were micro structs. > > > 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. > > And only hns really had this problem badly, other drivers had largely a single > structs. But yes, this waste is something I didn't like too much. > > Again, the value here is to make the drivers simpler, making the drivers > complicated again to juggle these structs seems bad. > Ok. thanks.
