[patch net-next 0/6] rocker: make master change handling nicer

[patch net-next 0/6] rocker: make master change handling nicer

[Jiri Pirko]

From: Jiri Pirko <jiri@mellanox.com>

Jiri Pirko (6):
  net: introduce change upper device notifier change info
  net: add netif_is_bridge_master helper
  net: add netif_is_ovs_master helper with IFF_OPENVSWITCH private flag
  net: kill long time unused bonding private flags
  rocker: use new helper to figure out master kind
  rocker: use change upper info

 drivers/net/ethernet/rocker/rocker.c | 74 ++++++++++++++++++++---------------
 include/linux/netdevice.h            | 75 +++++++++++++++++++-----------------
 net/core/dev.c                       | 16 +++++++-
 net/openvswitch/vport-internal_dev.c |  2 +-
 4 files changed, 97 insertions(+), 70 deletions(-)

-- 
1.9.3

[patch net-next 1/6] net: introduce change upper device notifier change info

[Jiri Pirko]

From: Jiri Pirko <jiri@mellanox.com>

Add info that is passed along with NETDEV_CHANGEUPPER event.

Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
 include/linux/netdevice.h |  7 +++++++
 net/core/dev.c            | 16 ++++++++++++++--
 2 files changed, 21 insertions(+), 2 deletions(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 6abe0d6..39f30da 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -2127,6 +2127,13 @@ struct netdev_notifier_change_info {
 	unsigned int flags_changed;
 };
 
+struct netdev_notifier_changeupper_info {
+	struct netdev_notifier_info info; /* must be first */
+	struct net_device *upper_dev; /* new upper dev */
+	bool master; /* is upper dev master */
+	bool linking; /* is the nofication for link or unlink */
+};
+
 static inline void netdev_notifier_info_init(struct netdev_notifier_info *info,
 					     struct net_device *dev)
 {
diff --git a/net/core/dev.c b/net/core/dev.c
index b1f3f48..e7ef971 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -5311,6 +5311,7 @@ static int __netdev_upper_dev_link(struct net_device *dev,
 				   struct net_device *upper_dev, bool master,
 				   void *private)
 {
+	struct netdev_notifier_changeupper_info changeupper_info;
 	struct netdev_adjacent *i, *j, *to_i, *to_j;
 	int ret = 0;
 
@@ -5329,6 +5330,10 @@ static int __netdev_upper_dev_link(struct net_device *dev,
 	if (master && netdev_master_upper_dev_get(dev))
 		return -EBUSY;
 
+	changeupper_info.upper_dev = upper_dev;
+	changeupper_info.master = master;
+	changeupper_info.linking = true;
+
 	ret = __netdev_adjacent_dev_link_neighbour(dev, upper_dev, private,
 						   master);
 	if (ret)
@@ -5367,7 +5372,8 @@ static int __netdev_upper_dev_link(struct net_device *dev,
 			goto rollback_lower_mesh;
 	}
 
-	call_netdevice_notifiers(NETDEV_CHANGEUPPER, dev);
+	call_netdevice_notifiers_info(NETDEV_CHANGEUPPER, dev,
+				      &changeupper_info.info);
 	return 0;
 
 rollback_lower_mesh:
@@ -5462,9 +5468,14 @@ EXPORT_SYMBOL(netdev_master_upper_dev_link_private);
 void netdev_upper_dev_unlink(struct net_device *dev,
 			     struct net_device *upper_dev)
 {
+	struct netdev_notifier_changeupper_info changeupper_info;
 	struct netdev_adjacent *i, *j;
 	ASSERT_RTNL();
 
+	changeupper_info.upper_dev = upper_dev;
+	changeupper_info.master = netdev_master_upper_dev_get(dev) == upper_dev;
+	changeupper_info.linking = false;
+
 	__netdev_adjacent_dev_unlink_neighbour(dev, upper_dev);
 
 	/* Here is the tricky part. We must remove all dev's lower
@@ -5484,7 +5495,8 @@ void netdev_upper_dev_unlink(struct net_device *dev,
 	list_for_each_entry(i, &upper_dev->all_adj_list.upper, list)
 		__netdev_adjacent_dev_unlink(dev, i->dev);
 
-	call_netdevice_notifiers(NETDEV_CHANGEUPPER, dev);
+	call_netdevice_notifiers_info(NETDEV_CHANGEUPPER, dev,
+				      &changeupper_info.info);
 }
 EXPORT_SYMBOL(netdev_upper_dev_unlink);
 
-- 
1.9.3

[patch net-next 2/6] net: add netif_is_bridge_master helper

[Jiri Pirko]

From: Jiri Pirko <jiri@mellanox.com>

Add this helper so code can easily figure out if netdev is a bridge.

Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
 include/linux/netdevice.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 39f30da..be625f4 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -3848,6 +3848,11 @@ static inline bool netif_is_vrf(const struct net_device *dev)
 	return dev->priv_flags & IFF_VRF_MASTER;
 }
 
+static inline bool netif_is_bridge_master(const struct net_device *dev)
+{
+	return dev->priv_flags & IFF_EBRIDGE;
+}
+
 static inline bool netif_index_is_vrf(struct net *net, int ifindex)
 {
 	bool rc = false;
-- 
1.9.3

[patch net-next 3/6] net: add netif_is_ovs_master helper with IFF_OPENVSWITCH private flag

[Jiri Pirko]

From: Jiri Pirko <jiri@mellanox.com>

Add this helper so code can easily figure out if netdev is openswitch.

Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
 include/linux/netdevice.h            | 8 ++++++++
 net/openvswitch/vport-internal_dev.c | 2 +-
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index be625f4..0a884e6 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -1264,6 +1264,7 @@ struct net_device_ops {
  * @IFF_MACVLAN: Macvlan device
  * @IFF_VRF_MASTER: device is a VRF master
  * @IFF_NO_QUEUE: device can run without qdisc attached
+ * @IFF_VRF_OPENVSWITCH: device is a Open vSwitch master
  */
 enum netdev_priv_flags {
 	IFF_802_1Q_VLAN			= 1<<0,
@@ -1293,6 +1294,7 @@ enum netdev_priv_flags {
 	IFF_IPVLAN_SLAVE		= 1<<24,
 	IFF_VRF_MASTER			= 1<<25,
 	IFF_NO_QUEUE			= 1<<26,
+	IFF_OPENVSWITCH			= 1<<27,
 };
 
 #define IFF_802_1Q_VLAN			IFF_802_1Q_VLAN
@@ -1322,6 +1324,7 @@ enum netdev_priv_flags {
 #define IFF_IPVLAN_SLAVE		IFF_IPVLAN_SLAVE
 #define IFF_VRF_MASTER			IFF_VRF_MASTER
 #define IFF_NO_QUEUE			IFF_NO_QUEUE
+#define IFF_OPENVSWITCH			IFF_OPENVSWITCH
 
 /**
  *	struct net_device - The DEVICE structure.
@@ -3853,6 +3856,11 @@ static inline bool netif_is_bridge_master(const struct net_device *dev)
 	return dev->priv_flags & IFF_EBRIDGE;
 }
 
+static inline bool netif_is_ovs_master(const struct net_device *dev)
+{
+	return dev->priv_flags & IFF_OPENVSWITCH;
+}
+
 static inline bool netif_index_is_vrf(struct net *net, int ifindex)
 {
 	bool rc = false;
diff --git a/net/openvswitch/vport-internal_dev.c b/net/openvswitch/vport-internal_dev.c
index c058bbf..80b3e12 100644
--- a/net/openvswitch/vport-internal_dev.c
+++ b/net/openvswitch/vport-internal_dev.c
@@ -135,7 +135,7 @@ static void do_setup(struct net_device *netdev)
 	netdev->netdev_ops = &internal_dev_netdev_ops;
 
 	netdev->priv_flags &= ~IFF_TX_SKB_SHARING;
-	netdev->priv_flags |= IFF_LIVE_ADDR_CHANGE;
+	netdev->priv_flags |= IFF_LIVE_ADDR_CHANGE | IFF_OPENVSWITCH;
 	netdev->destructor = internal_dev_destructor;
 	netdev->ethtool_ops = &internal_dev_ethtool_ops;
 	netdev->rtnl_link_ops = &internal_dev_link_ops;
-- 
1.9.3

[patch net-next 4/6] net: kill long time unused bonding private flags

[Jiri Pirko]

From: Jiri Pirko <jiri@mellanox.com>

We don't use them for years, just kill them now.

Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
 include/linux/netdevice.h | 57 +++++++++++++++++------------------------------
 1 file changed, 21 insertions(+), 36 deletions(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 0a884e6..d895b17 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -1240,13 +1240,8 @@ struct net_device_ops {
  *
  * @IFF_802_1Q_VLAN: 802.1Q VLAN device
  * @IFF_EBRIDGE: Ethernet bridging device
- * @IFF_SLAVE_INACTIVE: bonding slave not the curr. active
- * @IFF_MASTER_8023AD: bonding master, 802.3ad
- * @IFF_MASTER_ALB: bonding master, balance-alb
  * @IFF_BONDING: bonding master or slave
- * @IFF_SLAVE_NEEDARP: need ARPs for validation
  * @IFF_ISATAP: ISATAP interface (RFC4214)
- * @IFF_MASTER_ARPMON: bonding master, ARP mon in use
  * @IFF_WAN_HDLC: WAN HDLC device
  * @IFF_XMIT_DST_RELEASE: dev_hard_start_xmit() is allowed to
  *	release skb->dst
@@ -1269,43 +1264,33 @@ struct net_device_ops {
 enum netdev_priv_flags {
 	IFF_802_1Q_VLAN			= 1<<0,
 	IFF_EBRIDGE			= 1<<1,
-	IFF_SLAVE_INACTIVE		= 1<<2,
-	IFF_MASTER_8023AD		= 1<<3,
-	IFF_MASTER_ALB			= 1<<4,
-	IFF_BONDING			= 1<<5,
-	IFF_SLAVE_NEEDARP		= 1<<6,
-	IFF_ISATAP			= 1<<7,
-	IFF_MASTER_ARPMON		= 1<<8,
-	IFF_WAN_HDLC			= 1<<9,
-	IFF_XMIT_DST_RELEASE		= 1<<10,
-	IFF_DONT_BRIDGE			= 1<<11,
-	IFF_DISABLE_NETPOLL		= 1<<12,
-	IFF_MACVLAN_PORT		= 1<<13,
-	IFF_BRIDGE_PORT			= 1<<14,
-	IFF_OVS_DATAPATH		= 1<<15,
-	IFF_TX_SKB_SHARING		= 1<<16,
-	IFF_UNICAST_FLT			= 1<<17,
-	IFF_TEAM_PORT			= 1<<18,
-	IFF_SUPP_NOFCS			= 1<<19,
-	IFF_LIVE_ADDR_CHANGE		= 1<<20,
-	IFF_MACVLAN			= 1<<21,
-	IFF_XMIT_DST_RELEASE_PERM	= 1<<22,
-	IFF_IPVLAN_MASTER		= 1<<23,
-	IFF_IPVLAN_SLAVE		= 1<<24,
-	IFF_VRF_MASTER			= 1<<25,
-	IFF_NO_QUEUE			= 1<<26,
-	IFF_OPENVSWITCH			= 1<<27,
+	IFF_BONDING			= 1<<2,
+	IFF_ISATAP			= 1<<3,
+	IFF_WAN_HDLC			= 1<<4,
+	IFF_XMIT_DST_RELEASE		= 1<<5,
+	IFF_DONT_BRIDGE			= 1<<6,
+	IFF_DISABLE_NETPOLL		= 1<<7,
+	IFF_MACVLAN_PORT		= 1<<8,
+	IFF_BRIDGE_PORT			= 1<<9,
+	IFF_OVS_DATAPATH		= 1<<10,
+	IFF_TX_SKB_SHARING		= 1<<11,
+	IFF_UNICAST_FLT			= 1<<12,
+	IFF_TEAM_PORT			= 1<<13,
+	IFF_SUPP_NOFCS			= 1<<14,
+	IFF_LIVE_ADDR_CHANGE		= 1<<15,
+	IFF_MACVLAN			= 1<<16,
+	IFF_XMIT_DST_RELEASE_PERM	= 1<<17,
+	IFF_IPVLAN_MASTER		= 1<<18,
+	IFF_IPVLAN_SLAVE		= 1<<19,
+	IFF_VRF_MASTER			= 1<<20,
+	IFF_NO_QUEUE			= 1<<21,
+	IFF_OPENVSWITCH			= 1<<22,
 };
 
 #define IFF_802_1Q_VLAN			IFF_802_1Q_VLAN
 #define IFF_EBRIDGE			IFF_EBRIDGE
-#define IFF_SLAVE_INACTIVE		IFF_SLAVE_INACTIVE
-#define IFF_MASTER_8023AD		IFF_MASTER_8023AD
-#define IFF_MASTER_ALB			IFF_MASTER_ALB
 #define IFF_BONDING			IFF_BONDING
-#define IFF_SLAVE_NEEDARP		IFF_SLAVE_NEEDARP
 #define IFF_ISATAP			IFF_ISATAP
-#define IFF_MASTER_ARPMON		IFF_MASTER_ARPMON
 #define IFF_WAN_HDLC			IFF_WAN_HDLC
 #define IFF_XMIT_DST_RELEASE		IFF_XMIT_DST_RELEASE
 #define IFF_DONT_BRIDGE			IFF_DONT_BRIDGE
-- 
1.9.3

[patch net-next 5/6] rocker: use new helper to figure out master kind

[Jiri Pirko]

From: Jiri Pirko <jiri@mellanox.com>

Looking at rtnl kind string is kind of ugly. So use new helpers to do
this in nicer way.

Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
 drivers/net/ethernet/rocker/rocker.c | 19 +++++++------------
 1 file changed, 7 insertions(+), 12 deletions(-)

diff --git a/drivers/net/ethernet/rocker/rocker.c b/drivers/net/ethernet/rocker/rocker.c
index a7cb74a..62f383c 100644
--- a/drivers/net/ethernet/rocker/rocker.c
+++ b/drivers/net/ethernet/rocker/rocker.c
@@ -322,21 +322,16 @@ static u16 rocker_port_vlan_to_vid(const struct rocker_port *rocker_port,
 	return ntohs(vlan_id);
 }
 
-static bool rocker_port_is_slave(const struct rocker_port *rocker_port,
-				   const char *kind)
-{
-	return rocker_port->bridge_dev &&
-		!strcmp(rocker_port->bridge_dev->rtnl_link_ops->kind, kind);
-}
-
 static bool rocker_port_is_bridged(const struct rocker_port *rocker_port)
 {
-	return rocker_port_is_slave(rocker_port, "bridge");
+	return rocker_port->bridge_dev &&
+	       netif_is_bridge_master(rocker_port->bridge_dev);
 }
 
 static bool rocker_port_is_ovsed(const struct rocker_port *rocker_port)
 {
-	return rocker_port_is_slave(rocker_port, "openvswitch");
+	return rocker_port->bridge_dev &&
+	       netif_is_ovs_master(rocker_port->bridge_dev);
 }
 
 #define ROCKER_OP_FLAG_REMOVE		BIT(0)
@@ -5338,10 +5333,10 @@ static int rocker_port_master_changed(struct net_device *dev)
 	int err = 0;
 
 	/* N.B: Do nothing if the type of master is not supported */
-	if (master && master->rtnl_link_ops) {
-		if (!strcmp(master->rtnl_link_ops->kind, "bridge"))
+	if (master) {
+		if (netif_is_bridge_master(master))
 			err = rocker_port_bridge_join(rocker_port, master);
-		else if (!strcmp(master->rtnl_link_ops->kind, "openvswitch"))
+		else if (netif_is_ovs_master(master))
 			err = rocker_port_ovs_changed(rocker_port, master);
 	} else if (rocker_port_is_bridged(rocker_port)) {
 		err = rocker_port_bridge_leave(rocker_port);
-- 
1.9.3

[patch net-next 6/6] rocker: use change upper info

[Jiri Pirko]

From: Jiri Pirko <jiri@mellanox.com>

Since now information about changed upper is passed along, benefit from
that and use this info directly.

This also fixes possible issues that could happen when non-master device
is added (current code does not distinguish between master and non-master
upper device).

Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
 drivers/net/ethernet/rocker/rocker.c | 61 ++++++++++++++++++++++--------------
 1 file changed, 38 insertions(+), 23 deletions(-)

diff --git a/drivers/net/ethernet/rocker/rocker.c b/drivers/net/ethernet/rocker/rocker.c
index 62f383c..34ac41a 100644
--- a/drivers/net/ethernet/rocker/rocker.c
+++ b/drivers/net/ethernet/rocker/rocker.c
@@ -5326,46 +5326,61 @@ static int rocker_port_ovs_changed(struct rocker_port *rocker_port,
 	return err;
 }
 
-static int rocker_port_master_changed(struct net_device *dev)
+static int rocker_port_master_linked(struct rocker_port *rocker_port,
+				     struct net_device *master)
+{
+	int err = 0;
+
+	if (netif_is_bridge_master(master))
+		err = rocker_port_bridge_join(rocker_port, master);
+	else if (netif_is_ovs_master(master))
+		err = rocker_port_ovs_changed(rocker_port, master);
+	return err;
+}
+
+static int rocker_port_master_unlinked(struct rocker_port *rocker_port)
 {
-	struct rocker_port *rocker_port = netdev_priv(dev);
-	struct net_device *master = netdev_master_upper_dev_get(dev);
 	int err = 0;
 
-	/* N.B: Do nothing if the type of master is not supported */
-	if (master) {
-		if (netif_is_bridge_master(master))
-			err = rocker_port_bridge_join(rocker_port, master);
-		else if (netif_is_ovs_master(master))
-			err = rocker_port_ovs_changed(rocker_port, master);
-	} else if (rocker_port_is_bridged(rocker_port)) {
+	if (rocker_port_is_bridged(rocker_port))
 		err = rocker_port_bridge_leave(rocker_port);
-	} else if (rocker_port_is_ovsed(rocker_port)) {
+	else if (rocker_port_is_ovsed(rocker_port))
 		err = rocker_port_ovs_changed(rocker_port, NULL);
-	}
-
 	return err;
 }
 
 static int rocker_netdevice_event(struct notifier_block *unused,
 				  unsigned long event, void *ptr)
 {
-	struct net_device *dev;
+	struct net_device *dev = netdev_notifier_info_to_dev(ptr);
+	struct netdev_notifier_changeupper_info *info;
+	struct rocker_port *rocker_port;
 	int err;
 
+	if (!rocker_port_dev_check(dev))
+		return NOTIFY_DONE;
+
 	switch (event) {
 	case NETDEV_CHANGEUPPER:
-		dev = netdev_notifier_info_to_dev(ptr);
-		if (!rocker_port_dev_check(dev))
-			return NOTIFY_DONE;
-		err = rocker_port_master_changed(dev);
-		if (err)
-			netdev_warn(dev,
-				    "failed to reflect master change (err %d)\n",
-				    err);
+		info = ptr;
+		if (!info->master)
+			goto out;
+		rocker_port = netdev_priv(dev);
+		if (info->linking) {
+			err = rocker_port_master_linked(rocker_port,
+							info->upper_dev);
+			if (err)
+				netdev_warn(dev, "failed to reflect master linked (err %d)\n",
+					    err);
+		} else {
+			err = rocker_port_master_unlinked(rocker_port);
+			if (err)
+				netdev_warn(dev, "failed to reflect master unlinked (err %d)\n",
+					    err);
+		}
 		break;
 	}
-
+out:
 	return NOTIFY_DONE;
 }
 
-- 
1.9.3

Re: [patch net-next 3/6] net: add netif_is_ovs_master helper with IFF_OPENVSWITCH private flag

[Florian Fainelli]

On 26/08/15 09:36, Jiri Pirko wrote:
> From: Jiri Pirko <jiri@mellanox.com>
> 
> Add this helper so code can easily figure out if netdev is openswitch.
> 
> Signed-off-by: Jiri Pirko <jiri@mellanox.com>
> ---
>  include/linux/netdevice.h            | 8 ++++++++
>  net/openvswitch/vport-internal_dev.c | 2 +-
>  2 files changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index be625f4..0a884e6 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -1264,6 +1264,7 @@ struct net_device_ops {
>   * @IFF_MACVLAN: Macvlan device
>   * @IFF_VRF_MASTER: device is a VRF master
>   * @IFF_NO_QUEUE: device can run without qdisc attached
> + * @IFF_VRF_OPENVSWITCH: device is a Open vSwitch master

Typo, the flag you introduced is named IFF_OPENVSWITCH, not VFR_OPENSWITCH.

>   */
>  enum netdev_priv_flags {
>  	IFF_802_1Q_VLAN			= 1<<0,
> @@ -1293,6 +1294,7 @@ enum netdev_priv_flags {
>  	IFF_IPVLAN_SLAVE		= 1<<24,
>  	IFF_VRF_MASTER			= 1<<25,
>  	IFF_NO_QUEUE			= 1<<26,
> +	IFF_OPENVSWITCH			= 1<<27,
>  };
>  
>  #define IFF_802_1Q_VLAN			IFF_802_1Q_VLAN
> @@ -1322,6 +1324,7 @@ enum netdev_priv_flags {
>  #define IFF_IPVLAN_SLAVE		IFF_IPVLAN_SLAVE
>  #define IFF_VRF_MASTER			IFF_VRF_MASTER
>  #define IFF_NO_QUEUE			IFF_NO_QUEUE
> +#define IFF_OPENVSWITCH			IFF_OPENVSWITCH
>  
>  /**
>   *	struct net_device - The DEVICE structure.
> @@ -3853,6 +3856,11 @@ static inline bool netif_is_bridge_master(const struct net_device *dev)
>  	return dev->priv_flags & IFF_EBRIDGE;
>  }
>  
> +static inline bool netif_is_ovs_master(const struct net_device *dev)
> +{
> +	return dev->priv_flags & IFF_OPENVSWITCH;
> +}
> +
>  static inline bool netif_index_is_vrf(struct net *net, int ifindex)
>  {
>  	bool rc = false;
> diff --git a/net/openvswitch/vport-internal_dev.c b/net/openvswitch/vport-internal_dev.c
> index c058bbf..80b3e12 100644
> --- a/net/openvswitch/vport-internal_dev.c
> +++ b/net/openvswitch/vport-internal_dev.c
> @@ -135,7 +135,7 @@ static void do_setup(struct net_device *netdev)
>  	netdev->netdev_ops = &internal_dev_netdev_ops;
>  
>  	netdev->priv_flags &= ~IFF_TX_SKB_SHARING;
> -	netdev->priv_flags |= IFF_LIVE_ADDR_CHANGE;
> +	netdev->priv_flags |= IFF_LIVE_ADDR_CHANGE | IFF_OPENVSWITCH;
>  	netdev->destructor = internal_dev_destructor;
>  	netdev->ethtool_ops = &internal_dev_ethtool_ops;
>  	netdev->rtnl_link_ops = &internal_dev_link_ops;
> 


-- 
Florian

Re: [patch net-next 3/6] net: add netif_is_ovs_master helper with IFF_OPENVSWITCH private flag

[Scott Feldman]

On Wed, Aug 26, 2015 at 9:36 AM, Jiri Pirko <jiri@resnulli.us> wrote:
> From: Jiri Pirko <jiri@mellanox.com>
>
> Add this helper so code can easily figure out if netdev is openswitch.
>
> Signed-off-by: Jiri Pirko <jiri@mellanox.com>
> ---
>  include/linux/netdevice.h            | 8 ++++++++
>  net/openvswitch/vport-internal_dev.c | 2 +-
>  2 files changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index be625f4..0a884e6 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -1264,6 +1264,7 @@ struct net_device_ops {
>   * @IFF_MACVLAN: Macvlan device
>   * @IFF_VRF_MASTER: device is a VRF master
>   * @IFF_NO_QUEUE: device can run without qdisc attached
> + * @IFF_VRF_OPENVSWITCH: device is a Open vSwitch master
>   */
>  enum netdev_priv_flags {
>         IFF_802_1Q_VLAN                 = 1<<0,
> @@ -1293,6 +1294,7 @@ enum netdev_priv_flags {
>         IFF_IPVLAN_SLAVE                = 1<<24,
>         IFF_VRF_MASTER                  = 1<<25,
>         IFF_NO_QUEUE                    = 1<<26,
> +       IFF_OPENVSWITCH                 = 1<<27,
>  };
>
>  #define IFF_802_1Q_VLAN                        IFF_802_1Q_VLAN
> @@ -1322,6 +1324,7 @@ enum netdev_priv_flags {
>  #define IFF_IPVLAN_SLAVE               IFF_IPVLAN_SLAVE
>  #define IFF_VRF_MASTER                 IFF_VRF_MASTER
>  #define IFF_NO_QUEUE                   IFF_NO_QUEUE
> +#define IFF_OPENVSWITCH                        IFF_OPENVSWITCH
>
>  /**
>   *     struct net_device - The DEVICE structure.
> @@ -3853,6 +3856,11 @@ static inline bool netif_is_bridge_master(const struct net_device *dev)
>         return dev->priv_flags & IFF_EBRIDGE;
>  }
>
> +static inline bool netif_is_ovs_master(const struct net_device *dev)
> +{
> +       return dev->priv_flags & IFF_OPENVSWITCH;
> +}

We're going to run out of priv_flags bits.  This flag doesn't seem
like something that will be checked lots of places.  How about using
rtnl_link_ops->kind to save a bit in priv_flags?

static inline bool netif_is_ovs_master(const struct net_device *dev)
{
    return !strcmp(dev->rtnl_link_ops->kind, "openvswitch"));
}

> +
>  static inline bool netif_index_is_vrf(struct net *net, int ifindex)
>  {
>         bool rc = false;
> diff --git a/net/openvswitch/vport-internal_dev.c b/net/openvswitch/vport-internal_dev.c
> index c058bbf..80b3e12 100644
> --- a/net/openvswitch/vport-internal_dev.c
> +++ b/net/openvswitch/vport-internal_dev.c
> @@ -135,7 +135,7 @@ static void do_setup(struct net_device *netdev)
>         netdev->netdev_ops = &internal_dev_netdev_ops;
>
>         netdev->priv_flags &= ~IFF_TX_SKB_SHARING;
> -       netdev->priv_flags |= IFF_LIVE_ADDR_CHANGE;
> +       netdev->priv_flags |= IFF_LIVE_ADDR_CHANGE | IFF_OPENVSWITCH;
>         netdev->destructor = internal_dev_destructor;
>         netdev->ethtool_ops = &internal_dev_ethtool_ops;
>         netdev->rtnl_link_ops = &internal_dev_link_ops;
> --
> 1.9.3
>

Re: [patch net-next 6/6] rocker: use change upper info

[Scott Feldman]

On Wed, Aug 26, 2015 at 9:36 AM, Jiri Pirko <jiri@resnulli.us> wrote:
> From: Jiri Pirko <jiri@mellanox.com>
>
> Since now information about changed upper is passed along, benefit from
> that and use this info directly.
>
> This also fixes possible issues that could happen when non-master device
> is added (current code does not distinguish between master and non-master
> upper device).
>
> Signed-off-by: Jiri Pirko <jiri@mellanox.com>

Acked-by: Scott Feldman <sfeldma@gmail.com>

Re: [patch net-next 5/6] rocker: use new helper to figure out master kind

[Scott Feldman]

On Wed, Aug 26, 2015 at 9:36 AM, Jiri Pirko <jiri@resnulli.us> wrote:
> From: Jiri Pirko <jiri@mellanox.com>
>
> Looking at rtnl kind string is kind of ugly. So use new helpers to do
> this in nicer way.
>
> Signed-off-by: Jiri Pirko <jiri@mellanox.com>

Acked-by: Scott Feldman <sfeldma@gmail.com>

Re: [patch net-next 0/6] rocker: make master change handling nicer

[Scott Feldman]

On Wed, Aug 26, 2015 at 9:36 AM, Jiri Pirko <jiri@resnulli.us> wrote:
> From: Jiri Pirko <jiri@mellanox.com>
>
> Jiri Pirko (6):
>   net: introduce change upper device notifier change info
>   net: add netif_is_bridge_master helper
>   net: add netif_is_ovs_master helper with IFF_OPENVSWITCH private flag
>   net: kill long time unused bonding private flags
>   rocker: use new helper to figure out master kind
>   rocker: use change upper info

Looks good Jiri, thanks for cleaning this up.  Only nit was about
conserving netdev->priv_flags.   Using rtnl_link_ops->kind to
determine netdev type scales better.

Re: [patch net-next 3/6] net: add netif_is_ovs_master helper with IFF_OPENVSWITCH private flag

[Jiri Pirko]

Wed, Aug 26, 2015 at 07:24:55PM CEST, f.fainelli@gmail.com wrote:
>On 26/08/15 09:36, Jiri Pirko wrote:
>> From: Jiri Pirko <jiri@mellanox.com>
>> 
>> Add this helper so code can easily figure out if netdev is openswitch.
>> 
>> Signed-off-by: Jiri Pirko <jiri@mellanox.com>
>> ---
>>  include/linux/netdevice.h            | 8 ++++++++
>>  net/openvswitch/vport-internal_dev.c | 2 +-
>>  2 files changed, 9 insertions(+), 1 deletion(-)
>> 
>> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
>> index be625f4..0a884e6 100644
>> --- a/include/linux/netdevice.h
>> +++ b/include/linux/netdevice.h
>> @@ -1264,6 +1264,7 @@ struct net_device_ops {
>>   * @IFF_MACVLAN: Macvlan device
>>   * @IFF_VRF_MASTER: device is a VRF master
>>   * @IFF_NO_QUEUE: device can run without qdisc attached
>> + * @IFF_VRF_OPENVSWITCH: device is a Open vSwitch master
>
>Typo, the flag you introduced is named IFF_OPENVSWITCH, not VFR_OPENSWITCH.

Oups, will fix. Thanks

Re: [patch net-next 3/6] net: add netif_is_ovs_master helper with IFF_OPENVSWITCH private flag

[Jiri Pirko]

Wed, Aug 26, 2015 at 07:43:18PM CEST, sfeldma@gmail.com wrote:
>On Wed, Aug 26, 2015 at 9:36 AM, Jiri Pirko <jiri@resnulli.us> wrote:
>> From: Jiri Pirko <jiri@mellanox.com>
>>
>> Add this helper so code can easily figure out if netdev is openswitch.
>>
>> Signed-off-by: Jiri Pirko <jiri@mellanox.com>
>> ---
>>  include/linux/netdevice.h            | 8 ++++++++
>>  net/openvswitch/vport-internal_dev.c | 2 +-
>>  2 files changed, 9 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
>> index be625f4..0a884e6 100644
>> --- a/include/linux/netdevice.h
>> +++ b/include/linux/netdevice.h
>> @@ -1264,6 +1264,7 @@ struct net_device_ops {
>>   * @IFF_MACVLAN: Macvlan device
>>   * @IFF_VRF_MASTER: device is a VRF master
>>   * @IFF_NO_QUEUE: device can run without qdisc attached
>> + * @IFF_VRF_OPENVSWITCH: device is a Open vSwitch master
>>   */
>>  enum netdev_priv_flags {
>>         IFF_802_1Q_VLAN                 = 1<<0,
>> @@ -1293,6 +1294,7 @@ enum netdev_priv_flags {
>>         IFF_IPVLAN_SLAVE                = 1<<24,
>>         IFF_VRF_MASTER                  = 1<<25,
>>         IFF_NO_QUEUE                    = 1<<26,
>> +       IFF_OPENVSWITCH                 = 1<<27,
>>  };
>>
>>  #define IFF_802_1Q_VLAN                        IFF_802_1Q_VLAN
>> @@ -1322,6 +1324,7 @@ enum netdev_priv_flags {
>>  #define IFF_IPVLAN_SLAVE               IFF_IPVLAN_SLAVE
>>  #define IFF_VRF_MASTER                 IFF_VRF_MASTER
>>  #define IFF_NO_QUEUE                   IFF_NO_QUEUE
>> +#define IFF_OPENVSWITCH                        IFF_OPENVSWITCH
>>
>>  /**
>>   *     struct net_device - The DEVICE structure.
>> @@ -3853,6 +3856,11 @@ static inline bool netif_is_bridge_master(const struct net_device *dev)
>>         return dev->priv_flags & IFF_EBRIDGE;
>>  }
>>
>> +static inline bool netif_is_ovs_master(const struct net_device *dev)
>> +{
>> +       return dev->priv_flags & IFF_OPENVSWITCH;
>> +}
>
>We're going to run out of priv_flags bits.  This flag doesn't seem
>like something that will be checked lots of places.  How about using
>rtnl_link_ops->kind to save a bit in priv_flags?
>
>static inline bool netif_is_ovs_master(const struct net_device *dev)
>{
>    return !strcmp(dev->rtnl_link_ops->kind, "openvswitch"));
>}

There are lot of helpers like this for other soft-devices. I think that
is okay to have it this way. The thing is that sometimes you need to use
thi helper in fast path and in that case, you do not want to strcmp.

There is plenty of priv_flags bits for now when I killed the bonding
stuff.

Re: [patch net-next 3/6] net: add netif_is_ovs_master helper with IFF_OPENVSWITCH private flag

[Scott Feldman]

On Wed, Aug 26, 2015 at 10:43 PM, Jiri Pirko <jiri@resnulli.us> wrote:
> Wed, Aug 26, 2015 at 07:43:18PM CEST, sfeldma@gmail.com wrote:
>>On Wed, Aug 26, 2015 at 9:36 AM, Jiri Pirko <jiri@resnulli.us> wrote:
>>> From: Jiri Pirko <jiri@mellanox.com>
>>>
>>> Add this helper so code can easily figure out if netdev is openswitch.
>>>
>>> Signed-off-by: Jiri Pirko <jiri@mellanox.com>
>>> ---
>>>  include/linux/netdevice.h            | 8 ++++++++
>>>  net/openvswitch/vport-internal_dev.c | 2 +-
>>>  2 files changed, 9 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
>>> index be625f4..0a884e6 100644
>>> --- a/include/linux/netdevice.h
>>> +++ b/include/linux/netdevice.h
>>> @@ -1264,6 +1264,7 @@ struct net_device_ops {
>>>   * @IFF_MACVLAN: Macvlan device
>>>   * @IFF_VRF_MASTER: device is a VRF master
>>>   * @IFF_NO_QUEUE: device can run without qdisc attached
>>> + * @IFF_VRF_OPENVSWITCH: device is a Open vSwitch master
>>>   */
>>>  enum netdev_priv_flags {
>>>         IFF_802_1Q_VLAN                 = 1<<0,
>>> @@ -1293,6 +1294,7 @@ enum netdev_priv_flags {
>>>         IFF_IPVLAN_SLAVE                = 1<<24,
>>>         IFF_VRF_MASTER                  = 1<<25,
>>>         IFF_NO_QUEUE                    = 1<<26,
>>> +       IFF_OPENVSWITCH                 = 1<<27,
>>>  };
>>>
>>>  #define IFF_802_1Q_VLAN                        IFF_802_1Q_VLAN
>>> @@ -1322,6 +1324,7 @@ enum netdev_priv_flags {
>>>  #define IFF_IPVLAN_SLAVE               IFF_IPVLAN_SLAVE
>>>  #define IFF_VRF_MASTER                 IFF_VRF_MASTER
>>>  #define IFF_NO_QUEUE                   IFF_NO_QUEUE
>>> +#define IFF_OPENVSWITCH                        IFF_OPENVSWITCH
>>>
>>>  /**
>>>   *     struct net_device - The DEVICE structure.
>>> @@ -3853,6 +3856,11 @@ static inline bool netif_is_bridge_master(const struct net_device *dev)
>>>         return dev->priv_flags & IFF_EBRIDGE;
>>>  }
>>>
>>> +static inline bool netif_is_ovs_master(const struct net_device *dev)
>>> +{
>>> +       return dev->priv_flags & IFF_OPENVSWITCH;
>>> +}
>>
>>We're going to run out of priv_flags bits.  This flag doesn't seem
>>like something that will be checked lots of places.  How about using
>>rtnl_link_ops->kind to save a bit in priv_flags?
>>
>>static inline bool netif_is_ovs_master(const struct net_device *dev)
>>{
>>    return !strcmp(dev->rtnl_link_ops->kind, "openvswitch"));
>>}
>
> There are lot of helpers like this for other soft-devices. I think that
> is okay to have it this way. The thing is that sometimes you need to use
> thi helper in fast path and in that case, you do not want to strcmp.
>
> There is plenty of priv_flags bits for now when I killed the bonding
> stuff.

Ya, but think about the bit: you (and others) used a bit in priv_flags
to indicate the netdev type.  Can you add an enum field to
rtnl_link_ops->type to indicate link type?  Then it's not a strcmp.
You can write your helper using strcmp first, and then later migrate
to using rtnl_link_ops->type.

Re: [patch net-next 3/6] net: add netif_is_ovs_master helper with IFF_OPENVSWITCH private flag

[Jiri Pirko]

Thu, Aug 27, 2015 at 08:23:13AM CEST, sfeldma@gmail.com wrote:
>On Wed, Aug 26, 2015 at 10:43 PM, Jiri Pirko <jiri@resnulli.us> wrote:
>> Wed, Aug 26, 2015 at 07:43:18PM CEST, sfeldma@gmail.com wrote:
>>>On Wed, Aug 26, 2015 at 9:36 AM, Jiri Pirko <jiri@resnulli.us> wrote:
>>>> From: Jiri Pirko <jiri@mellanox.com>
>>>>
>>>> Add this helper so code can easily figure out if netdev is openswitch.
>>>>
>>>> Signed-off-by: Jiri Pirko <jiri@mellanox.com>
>>>> ---
>>>>  include/linux/netdevice.h            | 8 ++++++++
>>>>  net/openvswitch/vport-internal_dev.c | 2 +-
>>>>  2 files changed, 9 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
>>>> index be625f4..0a884e6 100644
>>>> --- a/include/linux/netdevice.h
>>>> +++ b/include/linux/netdevice.h
>>>> @@ -1264,6 +1264,7 @@ struct net_device_ops {
>>>>   * @IFF_MACVLAN: Macvlan device
>>>>   * @IFF_VRF_MASTER: device is a VRF master
>>>>   * @IFF_NO_QUEUE: device can run without qdisc attached
>>>> + * @IFF_VRF_OPENVSWITCH: device is a Open vSwitch master
>>>>   */
>>>>  enum netdev_priv_flags {
>>>>         IFF_802_1Q_VLAN                 = 1<<0,
>>>> @@ -1293,6 +1294,7 @@ enum netdev_priv_flags {
>>>>         IFF_IPVLAN_SLAVE                = 1<<24,
>>>>         IFF_VRF_MASTER                  = 1<<25,
>>>>         IFF_NO_QUEUE                    = 1<<26,
>>>> +       IFF_OPENVSWITCH                 = 1<<27,
>>>>  };
>>>>
>>>>  #define IFF_802_1Q_VLAN                        IFF_802_1Q_VLAN
>>>> @@ -1322,6 +1324,7 @@ enum netdev_priv_flags {
>>>>  #define IFF_IPVLAN_SLAVE               IFF_IPVLAN_SLAVE
>>>>  #define IFF_VRF_MASTER                 IFF_VRF_MASTER
>>>>  #define IFF_NO_QUEUE                   IFF_NO_QUEUE
>>>> +#define IFF_OPENVSWITCH                        IFF_OPENVSWITCH
>>>>
>>>>  /**
>>>>   *     struct net_device - The DEVICE structure.
>>>> @@ -3853,6 +3856,11 @@ static inline bool netif_is_bridge_master(const struct net_device *dev)
>>>>         return dev->priv_flags & IFF_EBRIDGE;
>>>>  }
>>>>
>>>> +static inline bool netif_is_ovs_master(const struct net_device *dev)
>>>> +{
>>>> +       return dev->priv_flags & IFF_OPENVSWITCH;
>>>> +}
>>>
>>>We're going to run out of priv_flags bits.  This flag doesn't seem
>>>like something that will be checked lots of places.  How about using
>>>rtnl_link_ops->kind to save a bit in priv_flags?
>>>
>>>static inline bool netif_is_ovs_master(const struct net_device *dev)
>>>{
>>>    return !strcmp(dev->rtnl_link_ops->kind, "openvswitch"));
>>>}
>>
>> There are lot of helpers like this for other soft-devices. I think that
>> is okay to have it this way. The thing is that sometimes you need to use
>> thi helper in fast path and in that case, you do not want to strcmp.
>>
>> There is plenty of priv_flags bits for now when I killed the bonding
>> stuff.
>
>Ya, but think about the bit: you (and others) used a bit in priv_flags
>to indicate the netdev type.  Can you add an enum field to
>rtnl_link_ops->type to indicate link type?  Then it's not a strcmp.
>You can write your helper using strcmp first, and then later migrate
>to using rtnl_link_ops->type.

But how different this would be to the priv_flags? You have to have
"central storage" for these types anyway, same as for flags. + it's one
more pointer dereference and null check on fastpath.

Re: [patch net-next 3/6] net: add netif_is_ovs_master helper with IFF_OPENVSWITCH private flag

[Jiri Pirko]

Thu, Aug 27, 2015 at 08:23:13AM CEST, sfeldma@gmail.com wrote:
>On Wed, Aug 26, 2015 at 10:43 PM, Jiri Pirko <jiri@resnulli.us> wrote:
>> Wed, Aug 26, 2015 at 07:43:18PM CEST, sfeldma@gmail.com wrote:
>>>On Wed, Aug 26, 2015 at 9:36 AM, Jiri Pirko <jiri@resnulli.us> wrote:
>>>> From: Jiri Pirko <jiri@mellanox.com>
>>>>
>>>> Add this helper so code can easily figure out if netdev is openswitch.
>>>>
>>>> Signed-off-by: Jiri Pirko <jiri@mellanox.com>
>>>> ---
>>>>  include/linux/netdevice.h            | 8 ++++++++
>>>>  net/openvswitch/vport-internal_dev.c | 2 +-
>>>>  2 files changed, 9 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
>>>> index be625f4..0a884e6 100644
>>>> --- a/include/linux/netdevice.h
>>>> +++ b/include/linux/netdevice.h
>>>> @@ -1264,6 +1264,7 @@ struct net_device_ops {
>>>>   * @IFF_MACVLAN: Macvlan device
>>>>   * @IFF_VRF_MASTER: device is a VRF master
>>>>   * @IFF_NO_QUEUE: device can run without qdisc attached
>>>> + * @IFF_VRF_OPENVSWITCH: device is a Open vSwitch master
>>>>   */
>>>>  enum netdev_priv_flags {
>>>>         IFF_802_1Q_VLAN                 = 1<<0,
>>>> @@ -1293,6 +1294,7 @@ enum netdev_priv_flags {
>>>>         IFF_IPVLAN_SLAVE                = 1<<24,
>>>>         IFF_VRF_MASTER                  = 1<<25,
>>>>         IFF_NO_QUEUE                    = 1<<26,
>>>> +       IFF_OPENVSWITCH                 = 1<<27,
>>>>  };
>>>>
>>>>  #define IFF_802_1Q_VLAN                        IFF_802_1Q_VLAN
>>>> @@ -1322,6 +1324,7 @@ enum netdev_priv_flags {
>>>>  #define IFF_IPVLAN_SLAVE               IFF_IPVLAN_SLAVE
>>>>  #define IFF_VRF_MASTER                 IFF_VRF_MASTER
>>>>  #define IFF_NO_QUEUE                   IFF_NO_QUEUE
>>>> +#define IFF_OPENVSWITCH                        IFF_OPENVSWITCH
>>>>
>>>>  /**
>>>>   *     struct net_device - The DEVICE structure.
>>>> @@ -3853,6 +3856,11 @@ static inline bool netif_is_bridge_master(const struct net_device *dev)
>>>>         return dev->priv_flags & IFF_EBRIDGE;
>>>>  }
>>>>
>>>> +static inline bool netif_is_ovs_master(const struct net_device *dev)
>>>> +{
>>>> +       return dev->priv_flags & IFF_OPENVSWITCH;
>>>> +}
>>>
>>>We're going to run out of priv_flags bits.  This flag doesn't seem
>>>like something that will be checked lots of places.  How about using
>>>rtnl_link_ops->kind to save a bit in priv_flags?
>>>
>>>static inline bool netif_is_ovs_master(const struct net_device *dev)
>>>{
>>>    return !strcmp(dev->rtnl_link_ops->kind, "openvswitch"));
>>>}
>>
>> There are lot of helpers like this for other soft-devices. I think that
>> is okay to have it this way. The thing is that sometimes you need to use
>> thi helper in fast path and in that case, you do not want to strcmp.
>>
>> There is plenty of priv_flags bits for now when I killed the bonding
>> stuff.
>
>Ya, but think about the bit: you (and others) used a bit in priv_flags
>to indicate the netdev type.  Can you add an enum field to
>rtnl_link_ops->type to indicate link type?  Then it's not a strcmp.
>You can write your helper using strcmp first, and then later migrate
>to using rtnl_link_ops->type.


Also, dev can be multiple things, it can be bridge port and vlan dev at
the same time. Flags are good for this.

Re: [patch net-next 3/6] net: add netif_is_ovs_master helper with IFF_OPENVSWITCH private flag

[Scott Feldman]

On Wed, Aug 26, 2015 at 11:30 PM, Jiri Pirko <jiri@resnulli.us> wrote:
> Thu, Aug 27, 2015 at 08:23:13AM CEST, sfeldma@gmail.com wrote:
>>On Wed, Aug 26, 2015 at 10:43 PM, Jiri Pirko <jiri@resnulli.us> wrote:
>>> Wed, Aug 26, 2015 at 07:43:18PM CEST, sfeldma@gmail.com wrote:
>>>>On Wed, Aug 26, 2015 at 9:36 AM, Jiri Pirko <jiri@resnulli.us> wrote:
>>>>> From: Jiri Pirko <jiri@mellanox.com>

>>>>We're going to run out of priv_flags bits.  This flag doesn't seem
>>>>like something that will be checked lots of places.  How about using
>>>>rtnl_link_ops->kind to save a bit in priv_flags?
>>>>
>>>>static inline bool netif_is_ovs_master(const struct net_device *dev)
>>>>{
>>>>    return !strcmp(dev->rtnl_link_ops->kind, "openvswitch"));
>>>>}
>>>
>>> There are lot of helpers like this for other soft-devices. I think that
>>> is okay to have it this way. The thing is that sometimes you need to use
>>> thi helper in fast path and in that case, you do not want to strcmp.
>>>
>>> There is plenty of priv_flags bits for now when I killed the bonding
>>> stuff.
>>
>>Ya, but think about the bit: you (and others) used a bit in priv_flags
>>to indicate the netdev type.  Can you add an enum field to
>>rtnl_link_ops->type to indicate link type?  Then it's not a strcmp.
>>You can write your helper using strcmp first, and then later migrate
>>to using rtnl_link_ops->type.
>
>
> Also, dev can be multiple things, it can be bridge port and vlan dev at
> the same time. Flags are good for this.

priv_flags bits are three types:

1) dev attribute (IFF_XMIT_DST_RELEASE, IFF_DISABLE_NETPOLL, etc)
2) dev type (IFF_802_1Q_VLAN, IFF_EBRIDGE, etc)
3) and dev membership (IFF_BRIDGE_PORT, IFF_TEAM_PORT, etc)

Are there types 2 or 3 in any fast paths?  Type 2 can move to enum;
they're mutually exclusive.  Type 3 is the dev's master's type 2, and
since dev can have only one master, no flag is needed: just look up
master type to know if dev is bridged or ovs'ed.

Re: [patch net-next 3/6] net: add netif_is_ovs_master helper with IFF_OPENVSWITCH private flag

[Jiri Pirko]

Thu, Aug 27, 2015 at 09:51:52AM CEST, sfeldma@gmail.com wrote:
>On Wed, Aug 26, 2015 at 11:30 PM, Jiri Pirko <jiri@resnulli.us> wrote:
>> Thu, Aug 27, 2015 at 08:23:13AM CEST, sfeldma@gmail.com wrote:
>>>On Wed, Aug 26, 2015 at 10:43 PM, Jiri Pirko <jiri@resnulli.us> wrote:
>>>> Wed, Aug 26, 2015 at 07:43:18PM CEST, sfeldma@gmail.com wrote:
>>>>>On Wed, Aug 26, 2015 at 9:36 AM, Jiri Pirko <jiri@resnulli.us> wrote:
>>>>>> From: Jiri Pirko <jiri@mellanox.com>
>
>>>>>We're going to run out of priv_flags bits.  This flag doesn't seem
>>>>>like something that will be checked lots of places.  How about using
>>>>>rtnl_link_ops->kind to save a bit in priv_flags?
>>>>>
>>>>>static inline bool netif_is_ovs_master(const struct net_device *dev)
>>>>>{
>>>>>    return !strcmp(dev->rtnl_link_ops->kind, "openvswitch"));
>>>>>}
>>>>
>>>> There are lot of helpers like this for other soft-devices. I think that
>>>> is okay to have it this way. The thing is that sometimes you need to use
>>>> thi helper in fast path and in that case, you do not want to strcmp.
>>>>
>>>> There is plenty of priv_flags bits for now when I killed the bonding
>>>> stuff.
>>>
>>>Ya, but think about the bit: you (and others) used a bit in priv_flags
>>>to indicate the netdev type.  Can you add an enum field to
>>>rtnl_link_ops->type to indicate link type?  Then it's not a strcmp.
>>>You can write your helper using strcmp first, and then later migrate
>>>to using rtnl_link_ops->type.
>>
>>
>> Also, dev can be multiple things, it can be bridge port and vlan dev at
>> the same time. Flags are good for this.
>
>priv_flags bits are three types:
>
>1) dev attribute (IFF_XMIT_DST_RELEASE, IFF_DISABLE_NETPOLL, etc)
>2) dev type (IFF_802_1Q_VLAN, IFF_EBRIDGE, etc)
>3) and dev membership (IFF_BRIDGE_PORT, IFF_TEAM_PORT, etc)
>
>Are there types 2 or 3 in any fast paths?  Type 2 can move to enum;
>they're mutually exclusive.  Type 3 is the dev's master's type 2, and
>since dev can have only one master, no flag is needed: just look up
>master type to know if dev is bridged or ovs'ed.

3 is certainly used in fast-path.

This priv_flags stuff could certainly get better somehow. I don't think
that using rtnl_link_ops is the answer here. It is certainly out of scope
of this patchset.