On Thu, Jun 11, 2015 at 6:57 AM, Jason Gunthorpe <[email protected]> wrote: > 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). >
Correct, get_netdev is assumed to be called under rcu. We'll switch to a version where the vendor calls dev_hold before it returns. > .. 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... > Correct, thanks. >> > 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... > Functionality wise, it's on par with the mlx4 implementation - supports regular Ethernet device, bonding, vlans and default GIDs. > Jason > -- > To unsubscribe from this list: send the line "unsubscribe linux-rdma" in > the body of a message to [email protected] > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
