On Tue, 5 Jun 2018 21:35:26 +0300 "Michael S. Tsirkin" <m...@redhat.com> wrote:
> Thanks, I think this is nice patch but I wonder whether it can be split > up somewhat. Not all of it is uncontroversial. I started that way, but then I was fixing code that was later deleted. The big change was eliminating the callbacks. > > On Mon, Jun 04, 2018 at 08:42:31PM -0700, Stephen Hemminger wrote: > > * The matching of secondary device to primary device policy > > is up to the network device. Both net_failover and netvsc > > will use MAC for now but can change separately. > > I actually suspect both will change to a serial number > down the road. > > > * The match policy is only used during initial discovery; after > > that the secondary device knows what the upper device is because > > of the parent/child relationship; no searching is required. > > That would obviously be an improvement - does it have to be tied with > rest of changes? This was not possible with the version of the common code that is in net now. > > > * Now, netvsc and net_failover use the same delayed work type > > mechanism for setup. Previously, net_failover code was triggering off > > name change but a similar policy was rejected for netvsc. > > "what is good for the goose is good for the gander" > > I don't really understand what you are saying here. I think the delayed > hack is kind of ugly and seems racy. Current failover code was rejected > by whom? Why is new one good and for whom? Did you want to do a name > change in netvsc but it was rejected? Could you clarify please? See: https://patchwork.ozlabs.org/patch/851711/ > > > * The net_failover private device info 'struct net_failover_info' > > should have been private to the driver file, not a visible > > API. > > > > * The net_failover device should use SET_NETDEV_DEV > > that is intended only for physical devices not virtual devices. > > You mean should not. Yes. Virtual device should not set device parent. > > > * No point in having DocBook style comments on a driver file. > > They only make sense on an external exposed API. > > > > * net_failover only supports Ethernet, so use ether_addr_copy. > > It is since you need to know about all the things you need to copy, and > because of mac matching. But it isn't too much effort to add more > transports and I don't see value in going in the reverse direction and > making it more ethernet specific that it already is. Sure, then do memcpy base on addr_len > > > * Set permanent and current address of net_failover device > > to match the primary. > > > > * Carrier should be marked off before registering device > > the net_failover device. > > Are above two bugfixes? Yes. > > > * Use netdev_XXX for log messages, in net_failover (not dev_xxx) > > > > * Since failover infrastructure is about linking devices just > > use RTNL no need for other locking in init and teardown. > > > > * Don't bother with ERR_PTR() style return if only possible > > return is success or no memory. > > > > * As much as possible, the terms master and slave should be avoided > > because of their cultural connotations. > > Also for consistency, failover is calling these primary and standby now. Good, let's standardize on that. > > > Note; this code has been tested on Hyper-V > > but is compile tested only on virtio. > > > > Fixes: 30c8bd5aa8b2 ("net: Introduce generic failover module") > > Signed-off-by: Stephen Hemminger <sthem...@microsoft.com> > > --- > > > > Although this patch needs to go into 4.18 (linux-net), > > I'd rather we focused on fixing bugs in 4.18, and left refactoring to > 4.19. > Either we fix or revert the current code in 4.18. Sorry, I am not having callback hell code in any vendor or upstream kernel.
