[PATCH RFC 0/2] Control VF link state

[PATCH RFC 0/2] Control VF link state

[Or Gerlitz]

Here's a suggestion for API and implementation that lets the admin to configure
the link state of the VF / SRIOV eSwitch vPORT. Basically, it has three states

Auto -	the VF link state will reflect the PF link state

Enable - VF link stat will be always up, traffic from VF to VF can 
	 work even if PF link is down.

Disable - VF link state is down and the VF can't send/recv, e.g can be 
  	  used to suspend the link while configuring the VF.

Rony and Or.

Rony Efraim (2):
  net/core: Add netlink directives to control VF link state
  net/mlx4: Add vf link state support

 drivers/net/ethernet/mellanox/mlx4/cmd.c       |   47 ++++++++++++++++++++++++
 drivers/net/ethernet/mellanox/mlx4/en_netdev.c |    8 ++++
 drivers/net/ethernet/mellanox/mlx4/eq.c        |    9 ++++-
 drivers/net/ethernet/mellanox/mlx4/fw.c        |    8 ++++
 drivers/net/ethernet/mellanox/mlx4/mlx4.h      |    1 +
 include/linux/mlx4/cmd.h                       |    2 +-
 include/linux/netdevice.h                      |    3 ++
 include/uapi/linux/if_link.h                   |   13 +++++++
 net/core/rtnetlink.c                           |    9 +++++
 9 files changed, 97 insertions(+), 3 deletions(-)

Rony Efraim (1):
  Add VF link state control 

iproute2 diffstat:

 include/linux/if_link.h |   13 +++++++++++++
 ip/iplink.c             |   14 ++++++++++++++
 2 files changed, 27 insertions(+), 0 deletions(-)

[PATCH RFC 1/2] net/core: Add netlink directives to control VF link state

[Or Gerlitz]

From: Rony Efraim <ronye@mellanox.com>

Auto -	the VF link state will reflect the PF link state

Enable - VF link stat will be always up, traffic from VF to VF can 
	 work even if PF link is down.

Disable - VF link state is down and the VF can't send/recv, e.g can be 
  	  used to suspend the link while configuring the VF.

Signed-off-by: Rony Efraim <ronye@mellanox.com>
---
 include/linux/netdevice.h    |    3 +++
 include/uapi/linux/if_link.h |   13 +++++++++++++
 net/core/rtnetlink.c         |    9 +++++++++
 3 files changed, 25 insertions(+), 0 deletions(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index f8898a4..e609474 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -801,6 +801,7 @@ struct netdev_fcoe_hbainfo {
  * int (*ndo_set_vf_spoofchk)(struct net_device *dev, int vf, bool setting);
  * int (*ndo_get_vf_config)(struct net_device *dev,
  *			    int vf, struct ifla_vf_info *ivf);
+ * int (*ndo_set_vf_link_state)(struct net_device *dev, int vf, int link_state);
  * int (*ndo_set_vf_port)(struct net_device *dev, int vf,
  *			  struct nlattr *port[]);
  * int (*ndo_get_vf_port)(struct net_device *dev, int vf, struct sk_buff *skb);
@@ -955,6 +956,8 @@ struct net_device_ops {
 	int			(*ndo_get_vf_config)(struct net_device *dev,
 						     int vf,
 						     struct ifla_vf_info *ivf);
+	int			(*ndo_set_vf_link_state)(struct net_device *dev,
+						       int vf, int link_state);
 	int			(*ndo_set_vf_port)(struct net_device *dev,
 						   int vf,
 						   struct nlattr *port[]);
diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
index b05823c..a923efd 100644
--- a/include/uapi/linux/if_link.h
+++ b/include/uapi/linux/if_link.h
@@ -336,6 +336,7 @@ enum {
 	IFLA_VF_VLAN,
 	IFLA_VF_TX_RATE,	/* TX Bandwidth Allocation */
 	IFLA_VF_SPOOFCHK,	/* Spoof Checking on/off switch */
+	IFLA_VF_LINK_STATE,	/* link state enable/disable/auto switch */
 	__IFLA_VF_MAX,
 };
 
@@ -362,6 +363,18 @@ struct ifla_vf_spoofchk {
 	__u32 setting;
 };
 
+enum {
+	IFLA_VF_LINK_STATE_AUTO,	/* link state of the uplink */
+	IFLA_VF_LINK_STATE_ENABLE,	/* link always up */
+	IFLA_VF_LINK_STATE_DISABLE,	/* link always down */
+	__IFLA_VF_LINK_STATE_MAX,
+};
+
+struct ifla_vf_link_state {
+	__u32 vf;
+	__u32 link_state;
+};
+
 /* VF ports management section
  *
  *	Nested layout of set/get msg is:
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index a08bd2b..bec44a3 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -1238,6 +1238,15 @@ static int do_setvfinfo(struct net_device *dev, struct nlattr *attr)
 							       ivs->setting);
 			break;
 		}
+		case IFLA_VF_LINK_STATE: {
+			struct ifla_vf_link_state *ivl;
+			ivl = nla_data(vf);
+			err = -EOPNOTSUPP;
+			if (ops->ndo_set_vf_link_state)
+				err = ops->ndo_set_vf_link_state(dev, ivl->vf,
+							   ivl->link_state);
+			break;
+		}
 		default:
 			err = -EINVAL;
 			break;
-- 
1.7.1

Re: [PATCH RFC 1/2] net/core: Add netlink directives to control VF link state

[Sergei Shtylyov]

Hello.

On 08-05-2013 17:45, Or Gerlitz wrote:

> From: Rony Efraim <ronye@mellanox.com>

> Auto -	the VF link state will reflect the PF link state
>
> Enable - VF link stat will be always up, traffic from VF to VF can
> 	 work even if PF link is down.
>
> Disable - VF link state is down and the VF can't send/recv, e.g can be
>    	  used to suspend the link while configuring the VF.

> Signed-off-by: Rony Efraim <ronye@mellanox.com>

[...]

> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> index a08bd2b..bec44a3 100644
> --- a/net/core/rtnetlink.c
> +++ b/net/core/rtnetlink.c
> @@ -1238,6 +1238,15 @@ static int do_setvfinfo(struct net_device *dev, struct nlattr *attr)
>   							       ivs->setting);
>   			break;
>   		}
> +		case IFLA_VF_LINK_STATE: {
> +			struct ifla_vf_link_state *ivl;
> +			ivl = nla_data(vf);

     Why not make it initializer? And empty line is needed between the 
declaration and other code.

WBR, Sergei

Re: [PATCH RFC 1/2] net/core: Add netlink directives to control VF link state

[Or Gerlitz]

On 08/05/2013 19:02, Sergei Shtylyov wrote:
>> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
>> index a08bd2b..bec44a3 100644
>> --- a/net/core/rtnetlink.c
>> +++ b/net/core/rtnetlink.c
>> @@ -1238,6 +1238,15 @@ static int do_setvfinfo(struct net_device 
>> *dev, struct nlattr *attr)
>>                                      ivs->setting);
>>               break;
>>           }
>> +        case IFLA_VF_LINK_STATE: {
>> +            struct ifla_vf_link_state *ivl;
>> +            ivl = nla_data(vf);
>
>     Why not make it initializer? And empty line is needed between the 
> declaration and other code. 

b/c we followed the conventions introduced in the preceding cases of 
do_setvfinfo which we found to be OK

[PATCH RFC 2/2] net/mlx4: Add vf link state support

[Or Gerlitz]

From: Rony Efraim <ronye@mellanox.com>

Add support to change the link state of VF (vPort).

Signed-off-by: Rony Efraim <ronye@mellanox.com>

---
 drivers/net/ethernet/mellanox/mlx4/cmd.c       |   47 ++++++++++++++++++++++++
 drivers/net/ethernet/mellanox/mlx4/en_netdev.c |    8 ++++
 drivers/net/ethernet/mellanox/mlx4/eq.c        |    9 ++++-
 drivers/net/ethernet/mellanox/mlx4/fw.c        |    8 ++++
 drivers/net/ethernet/mellanox/mlx4/mlx4.h      |    1 +
 include/linux/mlx4/cmd.h                       |    2 +-
 6 files changed, 72 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx4/cmd.c b/drivers/net/ethernet/mellanox/mlx4/cmd.c
index 1df56cc..4a9f7a5 100644
--- a/drivers/net/ethernet/mellanox/mlx4/cmd.c
+++ b/drivers/net/ethernet/mellanox/mlx4/cmd.c
@@ -39,6 +39,7 @@
 #include <linux/errno.h>
 
 #include <linux/mlx4/cmd.h>
+#include <linux/mlx4/device.h>
 #include <linux/semaphore.h>
 #include <rdma/ib_smi.h>
 
@@ -2184,3 +2185,49 @@ int mlx4_get_vf_config(struct mlx4_dev *dev, int port, int vf, struct ifla_vf_in
 	return 0;
 }
 EXPORT_SYMBOL_GPL(mlx4_get_vf_config);
+
+int mlx4_set_vf_link_state(struct mlx4_dev *dev, int port, int vf, int link_state)
+{
+	struct mlx4_priv *priv = mlx4_priv(dev);
+	struct mlx4_vport_state *s_info;
+	struct mlx4_vport_oper_state *vp_oper;
+	int slave;
+	u8 link_stat_event;
+
+	slave = mlx4_get_slave_indx(dev, vf);
+	if (slave < 0)
+		return -EINVAL;
+
+	switch (link_state) {
+	case IFLA_VF_LINK_STATE_AUTO:
+		/* get link curent state */
+		if (!priv->sense.do_sense_port[port])
+			link_stat_event = MLX4_PORT_CHANGE_SUBTYPE_ACTIVE;
+		else
+			link_stat_event = MLX4_PORT_CHANGE_SUBTYPE_DOWN;
+	    break;
+
+	case IFLA_VF_LINK_STATE_ENABLE:
+		link_stat_event = MLX4_PORT_CHANGE_SUBTYPE_ACTIVE;
+	    break;
+
+	case IFLA_VF_LINK_STATE_DISABLE:
+		link_stat_event = MLX4_PORT_CHANGE_SUBTYPE_DOWN;
+	    break;
+
+	default:
+		mlx4_warn(dev, "unknown value for link_state %02x on slave %d port %d\n",
+			  link_state, slave, port);
+		return -EINVAL;
+	};
+	/* update the admin & oper state on the link state */
+	s_info = &priv->mfunc.master.vf_admin[slave].vport[port];
+	vp_oper = &priv->mfunc.master.vf_oper[slave].vport[port];
+	s_info->link_state = link_state;
+	vp_oper->state.link_state = link_state;
+
+	/* send event */
+	mlx4_gen_port_state_change_eqe(dev, slave, port, link_stat_event);
+	return 0;
+}
+EXPORT_SYMBOL_GPL(mlx4_set_vf_link_state);
diff --git a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
index a69a908..af3d058 100644
--- a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
+++ b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
@@ -2060,6 +2060,13 @@ static int mlx4_en_get_vf_config(struct net_device *dev, int vf, struct ifla_vf_
 	return mlx4_get_vf_config(mdev->dev, en_priv->port, vf, ivf);
 }
 
+static int mlx4_en_set_vf_link_state(struct net_device *dev, int vf, int link_state)
+{
+	struct mlx4_en_priv *en_priv = netdev_priv(dev);
+	struct mlx4_en_dev *mdev = en_priv->mdev;
+
+	return mlx4_set_vf_link_state(mdev->dev, en_priv->port, vf, link_state);
+}
 static const struct net_device_ops mlx4_netdev_ops = {
 	.ndo_open		= mlx4_en_open,
 	.ndo_stop		= mlx4_en_close,
@@ -2100,6 +2107,7 @@ static const struct net_device_ops mlx4_netdev_ops_master = {
 	.ndo_set_vf_mac		= mlx4_en_set_vf_mac,
 	.ndo_set_vf_vlan	= mlx4_en_set_vf_vlan,
 	.ndo_set_vf_spoofchk	= mlx4_en_set_vf_spoofchk,
+	.ndo_set_vf_link_state	= mlx4_en_set_vf_link_state,
 	.ndo_get_vf_config	= mlx4_en_get_vf_config,
 #ifdef CONFIG_NET_POLL_CONTROLLER
 	.ndo_poll_controller	= mlx4_en_netpoll,
diff --git a/drivers/net/ethernet/mellanox/mlx4/eq.c b/drivers/net/ethernet/mellanox/mlx4/eq.c
index 8e3123a..0a882d4 100644
--- a/drivers/net/ethernet/mellanox/mlx4/eq.c
+++ b/drivers/net/ethernet/mellanox/mlx4/eq.c
@@ -448,6 +448,7 @@ static int mlx4_eq_int(struct mlx4_dev *dev, struct mlx4_eq *eq)
 	int i;
 	enum slave_port_gen_event gen_event;
 	unsigned long flags;
+	struct mlx4_vport_state *s_info;
 
 	while ((eqe = next_eqe_sw(eq, dev->caps.eqe_factor))) {
 		/*
@@ -556,7 +557,9 @@ static int mlx4_eq_int(struct mlx4_dev *dev, struct mlx4_eq *eq)
 						mlx4_dbg(dev, "%s: Sending MLX4_PORT_CHANGE_SUBTYPE_DOWN"
 							 " to slave: %d, port:%d\n",
 							 __func__, i, port);
-						mlx4_slave_event(dev, i, eqe);
+						s_info = &priv->mfunc.master.vf_oper[slave].vport[port].state;
+						if (IFLA_VF_LINK_STATE_AUTO == s_info->link_state)
+							mlx4_slave_event(dev, i, eqe);
 					} else {  /* IB port */
 						set_and_calc_slave_port_state(dev, i, port,
 									      MLX4_PORT_STATE_DEV_EVENT_PORT_DOWN,
@@ -580,7 +583,9 @@ static int mlx4_eq_int(struct mlx4_dev *dev, struct mlx4_eq *eq)
 					for (i = 0; i < dev->num_slaves; i++) {
 						if (i == mlx4_master_func_num(dev))
 							continue;
-						mlx4_slave_event(dev, i, eqe);
+						s_info = &priv->mfunc.master.vf_oper[slave].vport[port].state;
+						if (IFLA_VF_LINK_STATE_AUTO == s_info->link_state)
+							mlx4_slave_event(dev, i, eqe);
 					}
 				else /* IB port */
 					/* port-up event will be sent to a slave when the
diff --git a/drivers/net/ethernet/mellanox/mlx4/fw.c b/drivers/net/ethernet/mellanox/mlx4/fw.c
index b147bdd..37b2d02 100644
--- a/drivers/net/ethernet/mellanox/mlx4/fw.c
+++ b/drivers/net/ethernet/mellanox/mlx4/fw.c
@@ -828,8 +828,10 @@ int mlx4_QUERY_PORT_wrapper(struct mlx4_dev *dev, int slave,
 	u8 port_type;
 	u16 short_field;
 	int err;
+	int admin_link_state;
 
 #define MLX4_VF_PORT_NO_LINK_SENSE_MASK	0xE0
+#define MLX4_PORT_LINK_UP_MASK		0x80
 #define QUERY_PORT_CUR_MAX_PKEY_OFFSET	0x0c
 #define QUERY_PORT_CUR_MAX_GID_OFFSET	0x0e
 
@@ -855,6 +857,12 @@ int mlx4_QUERY_PORT_wrapper(struct mlx4_dev *dev, int slave,
 		/* set port type to currently operating port type */
 		port_type |= (dev->caps.port_type[vhcr->in_modifier] & 0x3);
 
+		admin_link_state = priv->mfunc.master.vf_oper[slave].vport[vhcr->in_modifier].state.link_state;
+		if (IFLA_VF_LINK_STATE_ENABLE == admin_link_state)
+			port_type |= MLX4_PORT_LINK_UP_MASK;
+		else if (IFLA_VF_LINK_STATE_DISABLE == admin_link_state)
+			port_type &= ~MLX4_PORT_LINK_UP_MASK;
+
 		MLX4_PUT(outbox->buf, port_type,
 			 QUERY_PORT_SUPPORTED_TYPE_OFFSET);
 
diff --git a/drivers/net/ethernet/mellanox/mlx4/mlx4.h b/drivers/net/ethernet/mellanox/mlx4/mlx4.h
index eac3dae..6368dcb 100644
--- a/drivers/net/ethernet/mellanox/mlx4/mlx4.h
+++ b/drivers/net/ethernet/mellanox/mlx4/mlx4.h
@@ -482,6 +482,7 @@ struct mlx4_vport_state {
 	u8  default_qos;
 	u32 tx_rate;
 	bool spoofchk;
+	u32 link_state;
 };
 
 struct mlx4_vf_admin_state {
diff --git a/include/linux/mlx4/cmd.h b/include/linux/mlx4/cmd.h
index adf6e06..8074a97 100644
--- a/include/linux/mlx4/cmd.h
+++ b/include/linux/mlx4/cmd.h
@@ -237,7 +237,7 @@ int mlx4_set_vf_mac(struct mlx4_dev *dev, int port, int vf, u64 mac);
 int mlx4_set_vf_vlan(struct mlx4_dev *dev, int port, int vf, u16 vlan, u8 qos);
 int mlx4_set_vf_spoofchk(struct mlx4_dev *dev, int port, int vf, bool setting);
 int mlx4_get_vf_config(struct mlx4_dev *dev, int port, int vf, struct ifla_vf_info *ivf);
-
+int mlx4_set_vf_link_state(struct mlx4_dev *dev, int port, int vf, int link_state);
 
 #define MLX4_COMM_GET_IF_REV(cmd_chan_ver) (u8)((cmd_chan_ver) >> 8)
 
-- 
1.7.1

[PATCH RFC iproute2] Add VF link state control

[Or Gerlitz]

From: Rony Efraim <ronye@mellanox.com>

Add link state per VF command

Signed-off-by: Rony Efraim <ronye@mellanox.com>
---
 include/linux/if_link.h |   13 +++++++++++++
 ip/iplink.c             |   14 ++++++++++++++
 2 files changed, 27 insertions(+), 0 deletions(-)

diff --git a/include/linux/if_link.h b/include/linux/if_link.h
index 965dc9f..f01f691 100644
--- a/include/linux/if_link.h
+++ b/include/linux/if_link.h
@@ -334,6 +334,7 @@ enum {
 	IFLA_VF_VLAN,
 	IFLA_VF_TX_RATE,	/* TX Bandwidth Allocation */
 	IFLA_VF_SPOOFCHK,	/* Spoof Checking on/off switch */
+	IFLA_VF_LINK_STATE,     /* link state on/off switch */
 	__IFLA_VF_MAX,
 };
 
@@ -360,6 +361,18 @@ struct ifla_vf_spoofchk {
 	__u32 setting;
 };
 
+enum {
+	IFLA_VF_LINK_STATE_AUTO,	/* link state of the uplink */
+	IFLA_VF_LINK_STATE_ENABLE,	/* link always up */
+	IFLA_VF_LINK_STATE_DISABLE,	/* link always down */
+	__IFLA_VF_LINK_STATE_MAX,
+};
+
+struct ifla_vf_link_state {
+	__u32 vf;
+	__u32 link_state;
+};
+
 /* VF ports management section
  *
  *	Nested layout of set/get msg is:
diff --git a/ip/iplink.c b/ip/iplink.c
index dc98019..ada9d42 100644
--- a/ip/iplink.c
+++ b/ip/iplink.c
@@ -77,6 +77,7 @@ void iplink_usage(void)
 	fprintf(stderr, "				   [ rate TXRATE ] ] \n");
 
 	fprintf(stderr, "				   [ spoofchk { on | off} ] ] \n");
+	fprintf(stderr, "				   [ state { auto | enable | disable} ] ]\n");
 	fprintf(stderr, "			  [ master DEVICE ]\n");
 	fprintf(stderr, "			  [ nomaster ]\n");
 	fprintf(stderr, "       ip link show [ DEVICE | group GROUP ] [up]\n");
@@ -255,6 +256,19 @@ static int iplink_parse_vf(int vf, int *argcp, char ***argvp,
 			ivs.vf = vf;
 			addattr_l(&req->n, sizeof(*req), IFLA_VF_SPOOFCHK, &ivs, sizeof(ivs));
 
+		} else if (matches(*argv, "state") == 0) {
+			struct ifla_vf_link_state ivl;
+			NEXT_ARG();
+			if (matches(*argv, "auto") == 0)
+				ivl.link_state = IFLA_VF_LINK_STATE_AUTO;
+			else if (matches(*argv, "enable") == 0)
+				ivl.link_state = IFLA_VF_LINK_STATE_ENABLE;
+			else if (matches(*argv, "disable") == 0)
+				ivl.link_state = IFLA_VF_LINK_STATE_DISABLE;
+			else
+				invarg("Invalid \"state\" value\n", *argv);
+			ivl.vf = vf;
+			addattr_l(&req->n, sizeof(*req), IFLA_VF_LINK_STATE, &ivl, sizeof(ivl));
 		} else {
 			/* rewind arg */
 			PREV_ARG();
-- 
1.7.1

Re: [PATCH RFC iproute2] Add VF link state control

[Stephen Hemminger]

On Wed,  8 May 2013 16:45:17 +0300
Or Gerlitz <ogerlitz@mellanox.com> wrote:

> From: Rony Efraim <ronye@mellanox.com>
> 
> Add link state per VF command
> 
> Signed-off-by: Rony Efraim <ronye@mellanox.com>

Isn't this redundant with OPERSTATE and LOWER_DOWN?

Re: [PATCH RFC iproute2] Add VF link state control

[Dan Williams]

On Wed, 2013-05-08 at 09:17 -0700, Stephen Hemminger wrote:
> On Wed,  8 May 2013 16:45:17 +0300
> Or Gerlitz <ogerlitz@mellanox.com> wrote:
> 
> > From: Rony Efraim <ronye@mellanox.com>
> > 
> > Add link state per VF command
> > 
> > Signed-off-by: Rony Efraim <ronye@mellanox.com>
> 
> Isn't this redundant with OPERSTATE and LOWER_DOWN?

I was going to say it was mostly redundant with the "set carrier from
userspace" patches from jpirko last December, but since a VF doesn't
appear to always have a netdev, it seems that functionality has to be
special-cased for VFs instead of being generic :(

Dan

Re: [PATCH RFC iproute2] Add VF link state control

[John Fastabend]

On 5/8/2013 9:24 AM, Dan Williams wrote:
> On Wed, 2013-05-08 at 09:17 -0700, Stephen Hemminger wrote:
>> On Wed,  8 May 2013 16:45:17 +0300
>> Or Gerlitz <ogerlitz@mellanox.com> wrote:
>>
>>> From: Rony Efraim <ronye@mellanox.com>
>>>
>>> Add link state per VF command
>>>
>>> Signed-off-by: Rony Efraim <ronye@mellanox.com>
>>
>> Isn't this redundant with OPERSTATE and LOWER_DOWN?
>
> I was going to say it was mostly redundant with the "set carrier from
> userspace" patches from jpirko last December, but since a VF doesn't
> appear to always have a netdev, it seems that functionality has to be
> special-cased for VFs instead of being generic :(
>
> Dan
>

Or the netdev is direct assigned to some VM/namespace or otherwise out
of scope.

It does seem unfortunate though that every time we want a feature that
already exists to be applicable for a VF we have to go through this
exercise of adding an ndo op and adding lookup code in each and
every driver to find the VF and pass messages back and forth.

.John

> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH RFC iproute2] Add VF link state control

[Or Gerlitz]

On Wed, May 8, 2013 at 7:44 PM, John Fastabend
<john.r.fastabend@intel.com> wrote:
> It does seem unfortunate though that every time we want a feature that
> already exists to be applicable for a VF we have to go through this
> exercise of adding an ndo op and adding lookup code in each and
> every driver to find the VF and pass messages back and forth.

agree, how about introducing ndo_set_vf_config which we can extend to
include new attrbiutes each time we want to add them...  actually do
we have robust way to extend
the existing ifla_vf_info? I guess so

int (*ndo_get_vf_config)(struct net_device *dev, int vf, struct
ifla_vf_info *ivf);

these two calls are also there but only the enic driver uses them

int (*ndo_set_vf_port)(struct net_device *dev, int vf, struct nlattr *port[]);
int (*ndo_get_vf_port)(struct net_device *dev, int vf, struct sk_buff *skb);

Re: [PATCH RFC iproute2] Add VF link state control

[Or Gerlitz]

> John Fastabend <john.r.fastabend@intel.com> wrote:
> > It does seem unfortunate though that every time we want a feature that
> > already exists to be applicable for a VF we have to go through this
> > exercise of adding an ndo op and adding lookup code in each and
> > every driver to find the VF and pass messages back and forth.
>
> agree, how about introducing ndo_set_vf_config which we can extend to
> include new attrbiutes each time we want to add them...  actually do
> we have robust way to extend the existing ifla_vf_info? I guess so
>
> int (*ndo_get_vf_config)(struct net_device *dev, int vf, struct ifla_vf_info *ivf);

Do we lean towards introducing new call/s to set/get VF config and can
be extended to new features as they come  - e.g
control VF link state, read VF packet/byte counters, set vlan egress
map for the VF when its in VGT mode, etc, or
stick to the current method of one ndo per need?


>
> these two calls are also there but only the enic driver uses them
>
> int (*ndo_set_vf_port)(struct net_device *dev, int vf, struct nlattr *port[]);
> int (*ndo_get_vf_port)(struct net_device *dev, int vf, struct sk_buff *skb);

Re: [PATCH RFC 0/2] Control VF link state

[Ben Hutchings]

On Wed, 2013-05-08 at 16:45 +0300, Or Gerlitz wrote:
> Here's a suggestion for API and implementation that lets the admin to configure
> the link state of the VF / SRIOV eSwitch vPORT. Basically, it has three states
> 
> Auto -	the VF link state will reflect the PF link state
> 
> Enable - VF link stat will be always up, traffic from VF to VF can 
> 	 work even if PF link is down.

It seems like it would be useful to implement these two options on the
PF as well.

Perhaps the default should also be specified?

Ben.

> Disable - VF link state is down and the VF can't send/recv, e.g can be 
>   	  used to suspend the link while configuring the VF.
[...]

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [PATCH RFC 0/2] Control VF link state

[Or Gerlitz]

On Thu, May 9, 2013 at 6:24 PM, Ben Hutchings <bhutchings@solarflare.com> wrote:
>
> On Wed, 2013-05-08 at 16:45 +0300, Or Gerlitz wrote:
> > Here's a suggestion for API and implementation that lets the admin to
> > configure
> > the link state of the VF / SRIOV eSwitch vPORT. Basically, it has three
> > states
> >
> > Auto -        the VF link state will reflect the PF link state
> >
> > Enable - VF link stat will be always up, traffic from VF to VF can
> >        work even if PF link is down.
>
> It seems like it would be useful to implement these two options on the PF as well.

You mean that PF <--> VF communication on the same node can be made to
work even when the physical link is down? this is a bit problematic to
model/implement I think. Generally speaking it makes things easier to
grasp if PF is considered to be the uplink of the eSwitch whos link is
1:1 as the physical link, but need to think that a bit more.


> Perhaps the default should also be specified?

mmm, not sure if we can require/enforce the same default for all drivers.

Re: [PATCH RFC 0/2] Control VF link state

[Ben Hutchings]

On Fri, 2013-05-10 at 02:17 +0300, Or Gerlitz wrote:
> On Thu, May 9, 2013 at 6:24 PM, Ben Hutchings <bhutchings@solarflare.com> wrote:
> >
> > On Wed, 2013-05-08 at 16:45 +0300, Or Gerlitz wrote:
> > > Here's a suggestion for API and implementation that lets the admin to
> > > configure
> > > the link state of the VF / SRIOV eSwitch vPORT. Basically, it has three
> > > states
> > >
> > > Auto -        the VF link state will reflect the PF link state
> > >
> > > Enable - VF link stat will be always up, traffic from VF to VF can
> > >        work even if PF link is down.
> >
> > It seems like it would be useful to implement these two options on the PF as well.
> 
> You mean that PF <--> VF communication on the same node can be made to
> work even when the physical link is down? this is a bit problematic to
> model/implement I think. Generally speaking it makes things easier to
> grasp if PF is considered to be the uplink of the eSwitch whos link is
> 1:1 as the physical link, but need to think that a bit more.

Yeah.  In some ways it could be better for a PF driver to create two net
devices, one which acts as a vswitch port and one which bypasses it (if
possible).  But then that's going to confuse people too.  I don't think
we can win...

> > Perhaps the default should also be specified?
> 
> mmm, not sure if we can require/enforce the same default for all drivers.

I know. :-/  But arbitrary differences between drivers are no fun for
sysadmins.

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [PATCH RFC 0/2] Control VF link state

[Or Gerlitz]

On Fri, May 10, 2013 at 3:34 AM, Ben Hutchings
<bhutchings@solarflare.com> wrote:
> On Fri, 2013-05-10 at 02:17 +0300, Or Gerlitz wrote:
> > On Thu, May 9, 2013 at 6:24 PM, Ben Hutchings <bhutchings@solarflare.com> wrote:
> > >
> > > On Wed, 2013-05-08 at 16:45 +0300, Or Gerlitz wrote:
> > > > Here's a suggestion for API and implementation that lets the admin to
> > > > configure
> > > > the link state of the VF / SRIOV eSwitch vPORT. Basically, it has three
> > > > states
> > > >
> > > > Auto -        the VF link state will reflect the PF link state
> > > >
> > > > Enable - VF link stat will be always up, traffic from VF to VF can
> > > >        work even if PF link is down.
> > >
> > > It seems like it would be useful to implement these two options on the PF as well.
> >
> > You mean that PF <--> VF communication on the same node can be made to
> > work even when the physical link is down? this is a bit problematic to
> > model/implement I think. Generally speaking it makes things easier to
> > grasp if PF is considered to be the uplink of the eSwitch whos link is
> > 1:1 as the physical link, but need to think that a bit more.
>
> Yeah.  In some ways it could be better for a PF driver to create two net
> devices, one which acts as a vswitch port and one which bypasses it (if possible).

That's interesting approach, any rough thoughts what you think it would by us?

> But then that's going to confuse people too.  I don't think we can win...
>
> > > Perhaps the default should also be specified?
> >
> > mmm, not sure if we can require/enforce the same default for all drivers.
>
> I know. :-/  But arbitrary differences between drivers are no fun for sysadmins.

yes, basically, on a production cloud environment, I would say that
before a vport (VF) interface
is configured, e.g ndo_set_vf_yyy is applied for it, we would want the
VF link to be down, but for non
production environments it could be problematic to have down/disabled
as the default... not sure, maybe
the default should be auto and for smart env they would set it to down
before the VF is probed on the VM?

Or.

Re: [PATCH RFC 0/2] Control VF link state

[Ben Hutchings]

On Mon, 2013-05-13 at 00:48 +0300, Or Gerlitz wrote:
> On Fri, May 10, 2013 at 3:34 AM, Ben Hutchings
> <bhutchings@solarflare.com> wrote:
> > On Fri, 2013-05-10 at 02:17 +0300, Or Gerlitz wrote:
> > > On Thu, May 9, 2013 at 6:24 PM, Ben Hutchings <bhutchings@solarflare.com> wrote:
> > > >
> > > > On Wed, 2013-05-08 at 16:45 +0300, Or Gerlitz wrote:
> > > > > Here's a suggestion for API and implementation that lets the admin to
> > > > > configure
> > > > > the link state of the VF / SRIOV eSwitch vPORT. Basically, it has three
> > > > > states
> > > > >
> > > > > Auto -        the VF link state will reflect the PF link state
> > > > >
> > > > > Enable - VF link stat will be always up, traffic from VF to VF can
> > > > >        work even if PF link is down.
> > > >
> > > > It seems like it would be useful to implement these two options on the PF as well.
> > >
> > > You mean that PF <--> VF communication on the same node can be made to
> > > work even when the physical link is down? this is a bit problematic to
> > > model/implement I think. Generally speaking it makes things easier to
> > > grasp if PF is considered to be the uplink of the eSwitch whos link is
> > > 1:1 as the physical link, but need to think that a bit more.
> >
> > Yeah.  In some ways it could be better for a PF driver to create two net
> > devices, one which acts as a vswitch port and one which bypasses it (if possible).
> 
> That's interesting approach, any rough thoughts what you think it would by us?

There are many attributes that could differ between the external port
(or ports - there could be more than one on the same integrated switch)
and the PF's switch port, including at least:

- Link state
- Packet counters
- MTU
- VLAN filtering

So long as we conflate these two (or more) ports into a single net
device, there will be confusion about what the attributes mean.

> > But then that's going to confuse people too.  I don't think we can win...
> >
> > > > Perhaps the default should also be specified?
> > >
> > > mmm, not sure if we can require/enforce the same default for all drivers.
> >
> > I know. :-/  But arbitrary differences between drivers are no fun for sysadmins.
> 
> yes, basically, on a production cloud environment, I would say that
> before a vport (VF) interface
> is configured, e.g ndo_set_vf_yyy is applied for it, we would want the
> VF link to be down, but for non
> production environments it could be problematic to have down/disabled
> as the default... not sure, maybe
> the default should be auto and for smart env they would set it to down
> before the VF is probed on the VM?

Not sure about that.

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [PATCH RFC 0/2] Control VF link state

[John Fastabend]

On 5/14/2013 10:12 AM, Ben Hutchings wrote:
> On Mon, 2013-05-13 at 00:48 +0300, Or Gerlitz wrote:
>> On Fri, May 10, 2013 at 3:34 AM, Ben Hutchings
>> <bhutchings@solarflare.com> wrote:
>>> On Fri, 2013-05-10 at 02:17 +0300, Or Gerlitz wrote:
>>>> On Thu, May 9, 2013 at 6:24 PM, Ben Hutchings <bhutchings@solarflare.com> wrote:
>>>>>
>>>>> On Wed, 2013-05-08 at 16:45 +0300, Or Gerlitz wrote:
>>>>>> Here's a suggestion for API and implementation that lets the admin to
>>>>>> configure
>>>>>> the link state of the VF / SRIOV eSwitch vPORT. Basically, it has three
>>>>>> states
>>>>>>
>>>>>> Auto -        the VF link state will reflect the PF link state
>>>>>>
>>>>>> Enable - VF link stat will be always up, traffic from VF to VF can
>>>>>>         work even if PF link is down.
>>>>>
>>>>> It seems like it would be useful to implement these two options on the PF as well.
>>>>
>>>> You mean that PF <--> VF communication on the same node can be made to
>>>> work even when the physical link is down? this is a bit problematic to
>>>> model/implement I think. Generally speaking it makes things easier to
>>>> grasp if PF is considered to be the uplink of the eSwitch whos link is
>>>> 1:1 as the physical link, but need to think that a bit more.
>>>
>>> Yeah.  In some ways it could be better for a PF driver to create two net
>>> devices, one which acts as a vswitch port and one which bypasses it (if possible).
>>
>> That's interesting approach, any rough thoughts what you think it would by us?
>
> There are many attributes that could differ between the external port
> (or ports - there could be more than one on the same integrated switch)
> and the PF's switch port, including at least:
>
> - Link state
> - Packet counters
> - MTU
> - VLAN filtering
>
> So long as we conflate these two (or more) ports into a single net
> device, there will be confusion about what the attributes mean.
>

One more example is all the control protocols we run over these
ports, LLDP being one example. Its not really clear who your peer
is when we have only a single netdevice. Is it the embedded bridge
or the peer of the external port?

.John

Re: [PATCH RFC 0/2] Control VF link state

[Or Gerlitz]

On Fri, May 10, 2013 at 3:34 AM, Ben Hutchings
<bhutchings@solarflare.com> wrote:
> Yeah.  In some ways it could be better for a PF driver to create two net
> devices, one which acts as a vswitch port and one which bypasses it (if
> possible).  But then that's going to confuse people too.  I don't think we can win...

>>> Perhaps the default should also be specified?


So can we make progress in steps here... e.g deal with the question
whether the PF exposes one or two netdevices in a future thread and
add now code supporting an admin ability to control VF link state (I
suggest for the default, e.g when no config directive was applied to
be "auto" means follow the PF link state, as was suggested here)? if
we agree on that, do we want new ndo_set_vf_ call or introducr
ndo_set_vf_config call which whose param will be further extended each
time we add new feature to control/configure?

Or.

Re: [PATCH RFC 0/2] Control VF link state

[Ben Hutchings]

On Mon, 2013-05-20 at 23:06 +0300, Or Gerlitz wrote:
> On Fri, May 10, 2013 at 3:34 AM, Ben Hutchings
> <bhutchings@solarflare.com> wrote:
> > Yeah.  In some ways it could be better for a PF driver to create two net
> > devices, one which acts as a vswitch port and one which bypasses it (if
> > possible).  But then that's going to confuse people too.  I don't think we can win...
> 
> >>> Perhaps the default should also be specified?
> 
> 
> So can we make progress in steps here... e.g deal with the question
> whether the PF exposes one or two netdevices in a future thread

Definitely, I don't want to derail your proposal.

> and
> add now code supporting an admin ability to control VF link state (I
> suggest for the default, e.g when no config directive was applied to
> be "auto" means follow the PF link state, as was suggested here)?

I think that's a sensible default.

> if
> we agree on that, do we want new ndo_set_vf_ call or introducr
> ndo_set_vf_config call which whose param will be further extended each
> time we add new feature to control/configure?

The risk I see with extensible operations (and which has occurred with
many ethtool operations) is that drivers may quietly ignore the elements
they don't implement.  So either (1) add yet another specific operation
or (2) define a general VF setter, require drivers to set some flags
that indicate which VF attributes are settable, and check the flags in
rtnetlink.c before calling into the driver.

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [PATCH RFC 0/2] Control VF link state

[Or Gerlitz]

On Mon, May 20, 2013 at 11:37 PM, Ben Hutchings
<bhutchings@solarflare.com> wrote:
> On Mon, 2013-05-20 at 23:06 +0300, Or Gerlitz wrote:
>> On Fri, May 10, 2013 at 3:34 AM, Ben Hutchings <bhutchings@solarflare.com> wrote:
>> > Yeah.  In some ways it could be better for a PF driver to create two net
>> > devices, one which acts as a vswitch port and one which bypasses it (if
>> > possible).  But then that's going to confuse people too.  I don't think we can win...
>>
>> >>> Perhaps the default should also be specified?
>>
>>
>> So can we make progress in steps here... e.g deal with the question
>> whether the PF exposes one or two netdevices in a future thread
>
> Definitely, I don't want to derail your proposal.
>
>> and
>> add now code supporting an admin ability to control VF link state (I
>> suggest for the default, e.g when no config directive was applied to
>> be "auto" means follow the PF link state, as was suggested here)?
>
> I think that's a sensible default.
>
>> if
>> we agree on that, do we want new ndo_set_vf_ call or introducr
>> ndo_set_vf_config call which whose param will be further extended each
>> time we add new feature to control/configure?
>
> The risk I see with extensible operations (and which has occurred with
> many ethtool operations) is that drivers may quietly ignore the elements
> they don't implement.  So either (1) add yet another specific operation
> or (2) define a general VF setter, require drivers to set some flags
> that indicate which VF attributes are settable, and check the flags in
> rtnetlink.c before calling into the driver.

both (1) and (2) makes sense to me, I am more leaned to (2), John, do
we have a go from your side to post V1 for net-next? which of the
options makes more sense to you?

Or.

Re: [PATCH RFC 0/2] Control VF link state

[John Fastabend]

[...]

>> The risk I see with extensible operations (and which has occurred with
>> many ethtool operations) is that drivers may quietly ignore the elements
>> they don't implement.  So either (1) add yet another specific operation
>> or (2) define a general VF setter, require drivers to set some flags
>> that indicate which VF attributes are settable, and check the flags in
>> rtnetlink.c before calling into the driver.
>
> both (1) and (2) makes sense to me, I am more leaned to (2), John, do
> we have a go from your side to post V1 for net-next? which of the
> options makes more sense to you?
>
> Or.
>

Either is fine if you prefer (2) that works for me. I'll get it
working with ixgbe after you post it.

.John

Re: [PATCH RFC 0/2] Control VF link state

[Or Gerlitz]

On Wed, May 22, 2013 at 12:43 AM, John Fastabend
<john.r.fastabend@intel.com> wrote:
> Either is fine if you prefer (2) that works for me. I'll get it working with ixgbe after you post it.

So eventually I went on #1, you said you're OK with both...

Or.