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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
