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

Jason Gunthorpe
2015-06-11T03:57:27Z

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