Re: [PATCH net] failover: eliminate callback hell

[Stephen Hemminger <stephen@networkplumber.org>]

On Tue, 5 Jun 2018 21:35:26 +0300
"Michael S. Tsirkin" <mst@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 <sthemmin@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.