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. 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. Jason
