On 12/10/2015 06:29 PM, Christoph Hellwig wrote: > On Thu, Dec 10, 2015 at 12:46:54PM -0500, Doug Ledford wrote: >> Organization. Let's be fair, the totally flat namespace you are >> preferring is the equivalent of a teenager that is completely incapable >> of picking thier dirty laundry up off the floor. It is sloppy, >> disorganized, often full of old cruft that you don't know if you can get >> rid of or not, often so disorganized you might have three similarly >> named items that you can't figure out which one should be used in which >> circumstances, etc. > > The most cruft I've found in major subsystems in years has been > in the RDMA code, so I'm not sure where that argument comes from. From reading struct netdevice and trying to work out what all of the items do. The portion I didn't like before is gone now, so it doesn't do any good for me to reference it (it was related to the address lists for the device). > We're pretty good at garbage collecting cruft in Linux, and the > typical counter examples are arbitrarily split structures where it's > easy to hide things. > >>> And looking at the existing members of >>> struct ib_device what determines if it goes straight into the device >>> or the attribute? >> >> Organization. What goes where depends on what makes sense according to >> the organization you are doing. > > So what makes num_comp_vectors or phys_port_cnt fit into ib_device, > while max_qp or max_cq are in struct ib_device_attr? Historically, the latter two were part of the device query and represented device maximums (and almost all of ib_device_attr is this sort of thing). The first depends on the initialization results for the PCI dev and is not a device limit that can be queried. The second is a high level device property that allows you to know how many devices need queried. If you were to ever have per-port struct for device capabilities, the port count would necessarily need to be outside of that area. So, from a logical perspective, ib_device_attr contents can be thought of as those things you get when you query the card, and only things you get by querying the card (although not all cards have all items in this struct), while the others in ib_device are part of other initialization steps. > I really like clean data structures, but keeping structures that > have 1:1 relationships and sit in the same module separate never > has been a good idea. Looking at struct netdevice, it has the sort of organization I would call reasonable. Things like struct tx_stats is a struct, even though it's embedded in the parent struct and not a pointer and there is exactly and only one copy, so it could be flat. >>> There is a reason why we don't do this weird >>> attr split in other Linux subsystems, and making IB follow this pattern >>> makes everyone feel right at home instead of wondering about the >>> weird attribute. >> >> Being organized is not "weird". Let's not wax poetic about sloppy, >> disorganized structures. Let's be honest about what they are so we >> don't feel like we need to take a shower every time we talk about them >> to purge us of the sins of our lies. > > I call that utter BS. Being organized is exactly not having multiple > structures that have the same scope or life time, it's actually what > I call disorganzied. While I didn't make a huge search for anything, at least the tx stats in struct netdevice are a direct contradiction to this statement. They are something I would call well organized. And speaking of that, in struct netdevice, all of the various ops elements are handled as structs, including the base netdevice ops, whereas struct ib_device puts the base ops flat in the struct. So if we wanted to be more like struct netdevice, we would move the base ops out of struct ib_device. > There is a lot to be said about grouping the > fields in the structure, and that's how sensible subsystems handle it: > > stuct foo_bar { > /* read/write in the hot path, keep together and tightly packed: */ > ... > > /* read-only in the hot path */ > ... > > /* random members: */ > ... > > /* properties here, immutable after setup: */ > ... > }; > > but that's completely inverse to what we're having with ib_device > currently. Well, the current struct ib_device is not well laid out. That I'll grant. But it's not because ib_device_attr is a struct. It simply isn't well laid out. Among other things, for the example you give above, struct ib_device is the wrong layer to be trying to lay things out like this. An ib_device can be multi-port, and each port can be a different RDMA device type. I would argue then that the hotpath items need to be done at the port level, not the parent device level. We started this with the port_immutable array. I would argue that continuing with that work and moving further down that path is the right way to go. -- Doug Ledford GPG KeyID: 0E572FDD