Re: [PATCH net-next v12 1/5] net: Introduce generic failover module

[Samudrala, Sridhar]

On 5/30/2018 7:52 PM, Stephen Hemminger wrote:

On Fri, 25 May 2018 16:06:58 -0700
"Samudrala, Sridhar"  wrote:


On 5/25/2018 3:38 PM, Stephen Hemminger wrote:

On Thu, 24 May 2018 09:55:13 -0700
Sridhar Samudrala  wrote:
  

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 03ed492c4e14..0f4ba52b641d 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -1421,6 +1421,8 @@ struct net_device_ops {
*   entity (i.e. the master device for bridged veth)
* @IFF_MACSEC: device is a MACsec device
* @IFF_NO_RX_HANDLER: device doesn't support the rx_handler hook
+ * @IFF_FAILOVER: device is a failover master device
+ * @IFF_FAILOVER_SLAVE: device is lower dev of a failover master device
*/
   enum netdev_priv_flags {
IFF_802_1Q_VLAN = 1<<0,
@@ -1450,6 +1452,8 @@ enum netdev_priv_flags {
IFF_PHONY_HEADROOM  = 1<<24,
IFF_MACSEC  = 1<<25,
IFF_NO_RX_HANDLER   = 1<<26,
+   IFF_FAILOVER= 1<<27,
+   IFF_FAILOVER_SLAVE  = 1<<28,
   };

Why is FAILOVER any different than other master/slave relationships.
I don't think you need to take up precious netdev flag bits for this.

These are netdev priv flags.
Jiri says that IFF_MASTER/IFF_SLAVE are bonding specific flags and cannot be 
used
with other failover mechanisms. Team also doesn't use this flags and it has its 
own
priv_flags.


This change breaks userspace.
We already have worked with partners to ignore devices marked as IFF_SLAVE,
and IFF_SLAVE is visible to user space API's.


I specifically made sure not to remove IFF_SLAVE in the netvsc patch.




NAK


___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH net-next v12 1/5] net: Introduce generic failover module

[Stephen Hemminger]

On Fri, 25 May 2018 16:06:58 -0700
"Samudrala, Sridhar"  wrote:

> On 5/25/2018 3:38 PM, Stephen Hemminger wrote:
> > On Thu, 24 May 2018 09:55:13 -0700
> > Sridhar Samudrala  wrote:
> >  
> >> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> >> index 03ed492c4e14..0f4ba52b641d 100644
> >> --- a/include/linux/netdevice.h
> >> +++ b/include/linux/netdevice.h
> >> @@ -1421,6 +1421,8 @@ struct net_device_ops {
> >>*   entity (i.e. the master device for bridged veth)
> >>* @IFF_MACSEC: device is a MACsec device
> >>* @IFF_NO_RX_HANDLER: device doesn't support the rx_handler hook
> >> + * @IFF_FAILOVER: device is a failover master device
> >> + * @IFF_FAILOVER_SLAVE: device is lower dev of a failover master device
> >>*/
> >>   enum netdev_priv_flags {
> >>IFF_802_1Q_VLAN = 1<<0,
> >> @@ -1450,6 +1452,8 @@ enum netdev_priv_flags {
> >>IFF_PHONY_HEADROOM  = 1<<24,
> >>IFF_MACSEC  = 1<<25,
> >>IFF_NO_RX_HANDLER   = 1<<26,
> >> +  IFF_FAILOVER= 1<<27,
> >> +  IFF_FAILOVER_SLAVE  = 1<<28,
> >>   };  
> > Why is FAILOVER any different than other master/slave relationships.
> > I don't think you need to take up precious netdev flag bits for this.  
> 
> These are netdev priv flags.
> Jiri says that IFF_MASTER/IFF_SLAVE are bonding specific flags and cannot be 
> used
> with other failover mechanisms. Team also doesn't use this flags and it has 
> its own
> priv_flags.
> 

This change breaks userspace.
We already have worked with partners to ignore devices marked as IFF_SLAVE,
and IFF_SLAVE is visible to user space API's.

NAK
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH net-next v12 1/5] net: Introduce generic failover module

[Stephen Hemminger]

On Fri, 25 May 2018 16:06:58 -0700
"Samudrala, Sridhar"  wrote:

> On 5/25/2018 3:38 PM, Stephen Hemminger wrote:
> > On Thu, 24 May 2018 09:55:13 -0700
> > Sridhar Samudrala  wrote:
> >  
> >> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> >> index 03ed492c4e14..0f4ba52b641d 100644
> >> --- a/include/linux/netdevice.h
> >> +++ b/include/linux/netdevice.h
> >> @@ -1421,6 +1421,8 @@ struct net_device_ops {
> >>*   entity (i.e. the master device for bridged veth)
> >>* @IFF_MACSEC: device is a MACsec device
> >>* @IFF_NO_RX_HANDLER: device doesn't support the rx_handler hook
> >> + * @IFF_FAILOVER: device is a failover master device
> >> + * @IFF_FAILOVER_SLAVE: device is lower dev of a failover master device
> >>*/
> >>   enum netdev_priv_flags {
> >>IFF_802_1Q_VLAN = 1<<0,
> >> @@ -1450,6 +1452,8 @@ enum netdev_priv_flags {
> >>IFF_PHONY_HEADROOM  = 1<<24,
> >>IFF_MACSEC  = 1<<25,
> >>IFF_NO_RX_HANDLER   = 1<<26,
> >> +  IFF_FAILOVER= 1<<27,
> >> +  IFF_FAILOVER_SLAVE  = 1<<28,
> >>   };  
> > Why is FAILOVER any different than other master/slave relationships.
> > I don't think you need to take up precious netdev flag bits for this.  
> 
> These are netdev priv flags.
> Jiri says that IFF_MASTER/IFF_SLAVE are bonding specific flags and cannot be 
> used
> with other failover mechanisms. Team also doesn't use this flags and it has 
> its own
> priv_flags.
> 

They are already used by bonding and team.
I don't see why this can't reuse them.
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH net-next v12 1/5] net: Introduce generic failover module

[Jiri Pirko]

Sat, May 26, 2018 at 12:37:44AM CEST, step...@networkplumber.org wrote:
>On Thu, 24 May 2018 09:55:13 -0700
>Sridhar Samudrala  wrote:
>
>
>> +spin_lock(_lock);
>
>Since register is not in fast path, this should be a mutex?

I don't get it. Why would you prefer mutex over spinlock here?
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH net-next v12 1/5] net: Introduce generic failover module

[Samudrala, Sridhar]

On 5/25/2018 3:38 PM, Stephen Hemminger wrote:

On Thu, 24 May 2018 09:55:13 -0700
Sridhar Samudrala  wrote:


diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 03ed492c4e14..0f4ba52b641d 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -1421,6 +1421,8 @@ struct net_device_ops {
   *entity (i.e. the master device for bridged veth)
   * @IFF_MACSEC: device is a MACsec device
   * @IFF_NO_RX_HANDLER: device doesn't support the rx_handler hook
+ * @IFF_FAILOVER: device is a failover master device
+ * @IFF_FAILOVER_SLAVE: device is lower dev of a failover master device
   */
  enum netdev_priv_flags {
IFF_802_1Q_VLAN = 1<<0,
@@ -1450,6 +1452,8 @@ enum netdev_priv_flags {
IFF_PHONY_HEADROOM  = 1<<24,
IFF_MACSEC  = 1<<25,
IFF_NO_RX_HANDLER   = 1<<26,
+   IFF_FAILOVER= 1<<27,
+   IFF_FAILOVER_SLAVE  = 1<<28,
  };

Why is FAILOVER any different than other master/slave relationships.
I don't think you need to take up precious netdev flag bits for this.


These are netdev priv flags.
Jiri says that IFF_MASTER/IFF_SLAVE are bonding specific flags and cannot be 
used
with other failover mechanisms. Team also doesn't use this flags and it has its 
own
priv_flags.

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH net-next v12 1/5] net: Introduce generic failover module

[Samudrala, Sridhar]

On 5/25/2018 3:37 PM, Stephen Hemminger wrote:

On Thu, 24 May 2018 09:55:13 -0700
Sridhar Samudrala  wrote:



+   spin_lock(_lock);

Since register is not in fast path, this should be a mutex?


This is Jiri's comment which made me to switch to spinlock from mutex

  >> Why mutex? Apparently you don't need to sleep while holding a lock.
  >> Simple spinlock would do.





+int failover_slave_unregister(struct net_device *slave_dev)
+{
+   struct net_device *failover_dev;
+   struct failover_ops *fops;
+
+   if (!netif_is_failover_slave(slave_dev))
+   goto done;
+
+   ASSERT_RTNL();
+
+   failover_dev = failover_get_bymac(slave_dev->perm_addr, );
+   if (!failover_dev)
+   goto done;

Since the slave device must have a master device set already, why not use
that instead of searching by MAC address on unregister or link change.


We also need to get the fops(failover_ops)

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH net-next v12 1/5] net: Introduce generic failover module

[Stephen Hemminger]

On Thu, 24 May 2018 09:55:13 -0700
Sridhar Samudrala  wrote:

> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 03ed492c4e14..0f4ba52b641d 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -1421,6 +1421,8 @@ struct net_device_ops {
>   *   entity (i.e. the master device for bridged veth)
>   * @IFF_MACSEC: device is a MACsec device
>   * @IFF_NO_RX_HANDLER: device doesn't support the rx_handler hook
> + * @IFF_FAILOVER: device is a failover master device
> + * @IFF_FAILOVER_SLAVE: device is lower dev of a failover master device
>   */
>  enum netdev_priv_flags {
>   IFF_802_1Q_VLAN = 1<<0,
> @@ -1450,6 +1452,8 @@ enum netdev_priv_flags {
>   IFF_PHONY_HEADROOM  = 1<<24,
>   IFF_MACSEC  = 1<<25,
>   IFF_NO_RX_HANDLER   = 1<<26,
> + IFF_FAILOVER= 1<<27,
> + IFF_FAILOVER_SLAVE  = 1<<28,
>  };

Why is FAILOVER any different than other master/slave relationships.
I don't think you need to take up precious netdev flag bits for this.
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH net-next v12 1/5] net: Introduce generic failover module

[Stephen Hemminger]

On Thu, 24 May 2018 09:55:13 -0700
Sridhar Samudrala  wrote:


> + spin_lock(_lock);

Since register is not in fast path, this should be a mutex?


> +int failover_slave_unregister(struct net_device *slave_dev)
> +{
> + struct net_device *failover_dev;
> + struct failover_ops *fops;
> +
> + if (!netif_is_failover_slave(slave_dev))
> + goto done;
> +
> + ASSERT_RTNL();
> +
> + failover_dev = failover_get_bymac(slave_dev->perm_addr, );
> + if (!failover_dev)
> + goto done;

Since the slave device must have a master device set already, why not use
that instead of searching by MAC address on unregister or link change.

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization