Re: [PATCH for-next V5 00/12] Move RoCE GID management to IB/Core

[Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>]

On Wed, Jun 10, 2015 at 09:06:28PM -0400, Doug Ledford wrote:

> People tend to push the "patches should be small, self contained,
> incremental" ideal.  In some cases, that gets carried to an extreme.  In
> this case, patch 1 introduces one side of the locking and patch 3 and 5
> introduce the other halves.

I already did spot check patches 3 and 5 for exactly that. They add
other uses of RCU, but they appear to be totally different -
objectional for style reasons, but probably not incorrect.

For instance the rcu lock grabs in patch 3 and 5 are protecting the
call to netdev_master_upper_dev_get_rcu in patch 10. 'get_netdev' is
more correctly called 'get_netdev_rcu' in this design. (as I said, this
placement of rcu_read_lock is ugly).

.. and just searching through the patches for 'rcu' to write this, I
noticed this:

+void ib_enum_roce_ports_of_netdev(roce_netdev_filter filter,
[..]
+	down_read(&lists_rwsem);
+	list_for_each_entry_rcu(dev, &device_list, core_list)
+	     ib_dev_roce_ports_of_netdev(dev, filter, filter_cookie, cb,
+					    cookie);
+	up_read(&lists_rwsem);

Should't call list_for_each_entry_rcu under a rwsem, this is just left over
from the old locking regime...

> > I think you've got the right basic idea for a cleanup series here. It
> > is time to buckle down and execute it well.
> 
> Except that this isn't really a cleanup, and calling it that clouds the
> issue.

Well, I've been asking for a cleanup .. The entire goal is to make
things more reviewable and a no-functional-change cleanup would sure
help that..

> it. But that's not the case.  The new core code implements everything
> that the two drivers do, and then some more.

I'd be interested to see a list of the 'some more' included in the
patch comments, I didn't look with a fine toothed comb, but not much
functional stood out to me...

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