Re: device attr cleanup (was: Handle mlx4 max_sge_rd correctly)

[Christoph Hellwig <hch-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org>]

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

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.

> >  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.  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.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html