[dpdk-dev] [PATCH] kni: Add link status update

[dpdk-dev] [PATCH] kni: Add link status update

[Vijayakumar Muthuvel Manickam]

Add an ioctl command in rte_kni module to enable
DPDK applications to propagate link state changes to
kni virtual interfaces.

Signed-off-by: Vijayakumar Muthuvel Manickam <mmvijay@gmail.com>
---
 .../linuxapp/eal/include/exec-env/rte_kni_common.h |  2 ++
 lib/librte_eal/linuxapp/kni/kni_misc.c             | 39 ++++++++++++++++++++++
 lib/librte_kni/rte_kni.c                           | 18 ++++++++++
 lib/librte_kni/rte_kni.h                           | 17 ++++++++++
 4 files changed, 76 insertions(+)

diff --git a/lib/librte_eal/linuxapp/eal/include/exec-env/rte_kni_common.h b/lib/librte_eal/linuxapp/eal/include/exec-env/rte_kni_common.h
index 1e55c2d..b68001d 100644
--- a/lib/librte_eal/linuxapp/eal/include/exec-env/rte_kni_common.h
+++ b/lib/librte_eal/linuxapp/eal/include/exec-env/rte_kni_common.h
@@ -163,6 +163,7 @@ struct rte_kni_device_info {
 
 	/* mbuf size */
 	unsigned mbuf_size;
+	uint8_t link_state;
 };
 
 #define KNI_DEVICE "kni"
@@ -170,5 +171,6 @@ struct rte_kni_device_info {
 #define RTE_KNI_IOCTL_TEST    _IOWR(0, 1, int)
 #define RTE_KNI_IOCTL_CREATE  _IOWR(0, 2, struct rte_kni_device_info)
 #define RTE_KNI_IOCTL_RELEASE _IOWR(0, 3, struct rte_kni_device_info)
+#define RTE_KNI_IOCTL_LINK_UPDATE _IOWR(0, 4, struct rte_kni_device_info)
 
 #endif /* _RTE_KNI_COMMON_H_ */
diff --git a/lib/librte_eal/linuxapp/kni/kni_misc.c b/lib/librte_eal/linuxapp/kni/kni_misc.c
index 1935d32..b1015cd 100644
--- a/lib/librte_eal/linuxapp/kni/kni_misc.c
+++ b/lib/librte_eal/linuxapp/kni/kni_misc.c
@@ -548,6 +548,42 @@ kni_ioctl_release(unsigned int ioctl_num, unsigned long ioctl_param)
 }
 
 static int
+kni_ioctl_update_link_state(unsigned int ioctl_num, unsigned long ioctl_param)
+{
+	int ret = -EINVAL;
+	struct kni_dev *dev, *n;
+	struct rte_kni_device_info dev_info;
+
+	if (_IOC_SIZE(ioctl_num) > sizeof(dev_info))
+			return -EINVAL;
+
+	ret = copy_from_user(&dev_info, (void *)ioctl_param, sizeof(dev_info));
+	if (ret) {
+		KNI_ERR("copy_from_user in kni_ioctl_update_link_status");
+		return -EIO;
+	}
+
+	if (strlen(dev_info.name) == 0)
+		return ret;
+
+	down_write(&kni_list_lock);
+	list_for_each_entry_safe(dev, n, &kni_list_head, list) {
+		if (strncmp(dev->name, dev_info.name, RTE_KNI_NAMESIZE) != 0)
+			continue;
+
+		if (dev_info.link_state == 0)
+			netif_carrier_off(dev->net_dev);
+		else
+			netif_carrier_on(dev->net_dev);
+		ret = 0;
+		break;
+	}
+	up_write(&kni_list_lock);
+
+	return ret;
+}
+
+static int
 kni_ioctl(struct inode *inode,
 	unsigned int ioctl_num,
 	unsigned long ioctl_param)
@@ -569,6 +605,9 @@ kni_ioctl(struct inode *inode,
 	case _IOC_NR(RTE_KNI_IOCTL_RELEASE):
 		ret = kni_ioctl_release(ioctl_num, ioctl_param);
 		break;
+	case _IOC_NR(RTE_KNI_IOCTL_LINK_UPDATE):
+		ret = kni_ioctl_update_link_state(ioctl_num, ioctl_param);
+		break;
 	default:
 		KNI_DBG("IOCTL default \n");
 		break;
diff --git a/lib/librte_kni/rte_kni.c b/lib/librte_kni/rte_kni.c
index 4e70fa0..b6eda8a 100644
--- a/lib/librte_kni/rte_kni.c
+++ b/lib/librte_kni/rte_kni.c
@@ -512,6 +512,24 @@ rte_kni_release(struct rte_kni *kni)
 }
 
 int
+rte_kni_update_link_state(struct rte_kni *kni, uint8_t if_up)
+{
+	struct rte_kni_device_info dev_info;
+
+	if (!kni || !kni->in_use)
+		return -1;
+
+	snprintf(dev_info.name, sizeof(dev_info.name), "%s", kni->name);
+	dev_info.link_state = if_up;
+	if (ioctl(kni_fd, RTE_KNI_IOCTL_LINK_UPDATE, &dev_info) < 0) {
+		RTE_LOG(ERR, KNI, "Fail to update link state\n");
+		return -1;
+	}
+
+	return 0;
+}
+
+int
 rte_kni_handle_request(struct rte_kni *kni)
 {
 	unsigned ret;
diff --git a/lib/librte_kni/rte_kni.h b/lib/librte_kni/rte_kni.h
index 98edd72..a1bafd9 100644
--- a/lib/librte_kni/rte_kni.h
+++ b/lib/librte_kni/rte_kni.h
@@ -167,6 +167,23 @@ extern struct rte_kni *rte_kni_create(uint8_t port_id,
 extern int rte_kni_release(struct rte_kni *kni);
 
 /**
+ * Send link state changes to KNI interface in kernel space
+ *
+ * rte_kni_update_link_state is thread safe.
+ *
+ * @param kni
+ *  The pointer to the context of an existent KNI interface.
+ * @param if_up
+ *  interface link status
+ *
+ * @return
+ *  - 0 indicates success.
+ *  - negative value indicates failure.
+ */
+
+extern int rte_kni_update_link_state(struct rte_kni *kni, uint8_t if_up);
+
+/**
  * It is used to handle the request mbufs sent from kernel space.
  * Then analyzes it and calls the specific actions for the specific requests.
  * Finally constructs the response mbuf and puts it back to the resp_q.
-- 
1.8.1.4

Re: [dpdk-dev] [PATCH] kni: Add link status update

[Zhang, Helin]

Hello

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Vijayakumar
> Muthuvel Manickam
> Sent: Tuesday, May 19, 2015 7:33 AM
> To: dev@dpdk.org
> Subject: [dpdk-dev] [PATCH] kni: Add link status update
> 
> Add an ioctl command in rte_kni module to enable DPDK applications to
> propagate link state changes to kni virtual interfaces.
> 
> Signed-off-by: Vijayakumar Muthuvel Manickam <mmvijay@gmail.com>
> ---
>  .../linuxapp/eal/include/exec-env/rte_kni_common.h |  2 ++
>  lib/librte_eal/linuxapp/kni/kni_misc.c             | 39
> ++++++++++++++++++++++
>  lib/librte_kni/rte_kni.c                           | 18 ++++++++++
>  lib/librte_kni/rte_kni.h                           | 17 ++++++++++
>  4 files changed, 76 insertions(+)
> 
> diff --git
> a/lib/librte_eal/linuxapp/eal/include/exec-env/rte_kni_common.h
> b/lib/librte_eal/linuxapp/eal/include/exec-env/rte_kni_common.h
> index 1e55c2d..b68001d 100644
> --- a/lib/librte_eal/linuxapp/eal/include/exec-env/rte_kni_common.h
> +++ b/lib/librte_eal/linuxapp/eal/include/exec-env/rte_kni_common.h
> @@ -163,6 +163,7 @@ struct rte_kni_device_info {
> 
>  	/* mbuf size */
>  	unsigned mbuf_size;
> +	uint8_t link_state;
How about transfer more states from user space to kernel space, such as link speed, duplex, etc?

>  };
> 
>  #define KNI_DEVICE "kni"
> @@ -170,5 +171,6 @@ struct rte_kni_device_info {
>  #define RTE_KNI_IOCTL_TEST    _IOWR(0, 1, int)
>  #define RTE_KNI_IOCTL_CREATE  _IOWR(0, 2, struct
> rte_kni_device_info)  #define RTE_KNI_IOCTL_RELEASE _IOWR(0, 3,
> struct rte_kni_device_info)
> +#define RTE_KNI_IOCTL_LINK_UPDATE _IOWR(0, 4, struct
> +rte_kni_device_info)
> 
>  #endif /* _RTE_KNI_COMMON_H_ */
> diff --git a/lib/librte_eal/linuxapp/kni/kni_misc.c
> b/lib/librte_eal/linuxapp/kni/kni_misc.c
> index 1935d32..b1015cd 100644
> --- a/lib/librte_eal/linuxapp/kni/kni_misc.c
> +++ b/lib/librte_eal/linuxapp/kni/kni_misc.c
> @@ -548,6 +548,42 @@ kni_ioctl_release(unsigned int ioctl_num,
> unsigned long ioctl_param)  }
> 
>  static int
> +kni_ioctl_update_link_state(unsigned int ioctl_num, unsigned long
> +ioctl_param) {
> +	int ret = -EINVAL;
> +	struct kni_dev *dev, *n;
> +	struct rte_kni_device_info dev_info;
> +
> +	if (_IOC_SIZE(ioctl_num) > sizeof(dev_info))
> +			return -EINVAL;
> +
> +	ret = copy_from_user(&dev_info, (void *)ioctl_param,
> sizeof(dev_info));
> +	if (ret) {
> +		KNI_ERR("copy_from_user in kni_ioctl_update_link_status");
> +		return -EIO;
> +	}
> +
> +	if (strlen(dev_info.name) == 0)
> +		return ret;
> +
> +	down_write(&kni_list_lock);
> +	list_for_each_entry_safe(dev, n, &kni_list_head, list) {
> +		if (strncmp(dev->name, dev_info.name, RTE_KNI_NAMESIZE) !=
> 0)
> +			continue;
> +
> +		if (dev_info.link_state == 0)
> +			netif_carrier_off(dev->net_dev);
> +		else
> +			netif_carrier_on(dev->net_dev);
> +		ret = 0;
> +		break;
> +	}
> +	up_write(&kni_list_lock);
> +
> +	return ret;
> +}
> +
> +static int
>  kni_ioctl(struct inode *inode,
>  	unsigned int ioctl_num,
>  	unsigned long ioctl_param)
> @@ -569,6 +605,9 @@ kni_ioctl(struct inode *inode,
>  	case _IOC_NR(RTE_KNI_IOCTL_RELEASE):
>  		ret = kni_ioctl_release(ioctl_num, ioctl_param);
>  		break;
> +	case _IOC_NR(RTE_KNI_IOCTL_LINK_UPDATE):
> +		ret = kni_ioctl_update_link_state(ioctl_num, ioctl_param);
> +		break;
>  	default:
>  		KNI_DBG("IOCTL default \n");
>  		break;
> diff --git a/lib/librte_kni/rte_kni.c b/lib/librte_kni/rte_kni.c index
> 4e70fa0..b6eda8a 100644
> --- a/lib/librte_kni/rte_kni.c
> +++ b/lib/librte_kni/rte_kni.c
> @@ -512,6 +512,24 @@ rte_kni_release(struct rte_kni *kni)  }
> 
>  int
> +rte_kni_update_link_state(struct rte_kni *kni, uint8_t if_up) {
> +	struct rte_kni_device_info dev_info;
> +
> +	if (!kni || !kni->in_use)
> +		return -1;
> +
> +	snprintf(dev_info.name, sizeof(dev_info.name), "%s", kni->name);
> +	dev_info.link_state = if_up;
> +	if (ioctl(kni_fd, RTE_KNI_IOCTL_LINK_UPDATE, &dev_info) < 0) {
> +		RTE_LOG(ERR, KNI, "Fail to update link state\n");
> +		return -1;
> +	}
> +
> +	return 0;
> +}
> +
This new interface should be called at the end of rte_kni_alloc() to notify
the link status after a KNI interface is created.

Thanks,
Helin

> +int
>  rte_kni_handle_request(struct rte_kni *kni)  {
>  	unsigned ret;
> diff --git a/lib/librte_kni/rte_kni.h b/lib/librte_kni/rte_kni.h index
> 98edd72..a1bafd9 100644
> --- a/lib/librte_kni/rte_kni.h
> +++ b/lib/librte_kni/rte_kni.h
> @@ -167,6 +167,23 @@ extern struct rte_kni *rte_kni_create(uint8_t
> port_id,  extern int rte_kni_release(struct rte_kni *kni);
> 
>  /**
> + * Send link state changes to KNI interface in kernel space
> + *
> + * rte_kni_update_link_state is thread safe.
> + *
> + * @param kni
> + *  The pointer to the context of an existent KNI interface.
> + * @param if_up
> + *  interface link status
> + *
> + * @return
> + *  - 0 indicates success.
> + *  - negative value indicates failure.
> + */
> +
> +extern int rte_kni_update_link_state(struct rte_kni *kni, uint8_t
> +if_up);
> +
> +/**
>   * It is used to handle the request mbufs sent from kernel space.
>   * Then analyzes it and calls the specific actions for the specific requests.
>   * Finally constructs the response mbuf and puts it back to the resp_q.
> --
> 1.8.1.4

Re: [dpdk-dev] [PATCH] kni: Add link status update

[Stephen Hemminger]

I agree that this looks like a good facility to have but this is not
the right way to do it.

There are already several facilities to accomplish the same thing.

1. You can use the operstate functionality in kernel to manipulate carrier
   from another application (read Documentation/operstate.txt)

2. It is possible with sysfs to change carrier state. The KNI driver just
   has to provide an .ndo_change_carrier callback.

Introducing more ioctl's is not a good thing. KNI needs to get rid of
the quick and dirty ioctl approach and follow current best practices
for Linux network drivers. It should really be using netlink instead of ioctl().
Ioctl's have several compatibility issues that make them unacceptable upstream.
There is a standard API for create/destroy with netlink and the custom
ioctl's could be banished into the attic of dead API's.

PS: Has anybody even attempted to get KNI upstream? I can see lots
     of style (and security) issues that would need to be fixed.

Re: [dpdk-dev] [PATCH] kni: Add link status update

[Zhang, Helin]

Any response for my comments? Did I miss anything?

- Helin

> -----Original Message-----
> From: Zhang, Helin
> Sent: Tuesday, May 19, 2015 8:54 AM
> To: Vijayakumar Muthuvel Manickam; dev@dpdk.org
> Subject: RE: [dpdk-dev] [PATCH] kni: Add link status update
> 
> Hello
> 
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Vijayakumar
> > Muthuvel Manickam
> > Sent: Tuesday, May 19, 2015 7:33 AM
> > To: dev@dpdk.org
> > Subject: [dpdk-dev] [PATCH] kni: Add link status update
> >
> > Add an ioctl command in rte_kni module to enable DPDK applications to
> > propagate link state changes to kni virtual interfaces.
> >
> > Signed-off-by: Vijayakumar Muthuvel Manickam <mmvijay@gmail.com>
> > ---
> >  .../linuxapp/eal/include/exec-env/rte_kni_common.h |  2 ++
> >  lib/librte_eal/linuxapp/kni/kni_misc.c             | 39
> > ++++++++++++++++++++++
> >  lib/librte_kni/rte_kni.c                           | 18 ++++++++++
> >  lib/librte_kni/rte_kni.h                           | 17 ++++++++++
> >  4 files changed, 76 insertions(+)
> >
> > diff --git
> > a/lib/librte_eal/linuxapp/eal/include/exec-env/rte_kni_common.h
> > b/lib/librte_eal/linuxapp/eal/include/exec-env/rte_kni_common.h
> > index 1e55c2d..b68001d 100644
> > --- a/lib/librte_eal/linuxapp/eal/include/exec-env/rte_kni_common.h
> > +++ b/lib/librte_eal/linuxapp/eal/include/exec-env/rte_kni_common.h
> > @@ -163,6 +163,7 @@ struct rte_kni_device_info {
> >
> >  	/* mbuf size */
> >  	unsigned mbuf_size;
> > +	uint8_t link_state;
> How about transfer more states from user space to kernel space, such as link
> speed, duplex, etc?
> 
> >  };
> >
> >  #define KNI_DEVICE "kni"
> > @@ -170,5 +171,6 @@ struct rte_kni_device_info {
> >  #define RTE_KNI_IOCTL_TEST    _IOWR(0, 1, int)
> >  #define RTE_KNI_IOCTL_CREATE  _IOWR(0, 2, struct
> > rte_kni_device_info)  #define RTE_KNI_IOCTL_RELEASE _IOWR(0, 3, struct
> > rte_kni_device_info)
> > +#define RTE_KNI_IOCTL_LINK_UPDATE _IOWR(0, 4, struct
> > +rte_kni_device_info)
> >
> >  #endif /* _RTE_KNI_COMMON_H_ */
> > diff --git a/lib/librte_eal/linuxapp/kni/kni_misc.c
> > b/lib/librte_eal/linuxapp/kni/kni_misc.c
> > index 1935d32..b1015cd 100644
> > --- a/lib/librte_eal/linuxapp/kni/kni_misc.c
> > +++ b/lib/librte_eal/linuxapp/kni/kni_misc.c
> > @@ -548,6 +548,42 @@ kni_ioctl_release(unsigned int ioctl_num,
> > unsigned long ioctl_param)  }
> >
> >  static int
> > +kni_ioctl_update_link_state(unsigned int ioctl_num, unsigned long
> > +ioctl_param) {
> > +	int ret = -EINVAL;
> > +	struct kni_dev *dev, *n;
> > +	struct rte_kni_device_info dev_info;
> > +
> > +	if (_IOC_SIZE(ioctl_num) > sizeof(dev_info))
> > +			return -EINVAL;
> > +
> > +	ret = copy_from_user(&dev_info, (void *)ioctl_param,
> > sizeof(dev_info));
> > +	if (ret) {
> > +		KNI_ERR("copy_from_user in kni_ioctl_update_link_status");
> > +		return -EIO;
> > +	}
> > +
> > +	if (strlen(dev_info.name) == 0)
> > +		return ret;
> > +
> > +	down_write(&kni_list_lock);
> > +	list_for_each_entry_safe(dev, n, &kni_list_head, list) {
> > +		if (strncmp(dev->name, dev_info.name, RTE_KNI_NAMESIZE) !=
> > 0)
> > +			continue;
> > +
> > +		if (dev_info.link_state == 0)
> > +			netif_carrier_off(dev->net_dev);
> > +		else
> > +			netif_carrier_on(dev->net_dev);
> > +		ret = 0;
> > +		break;
> > +	}
> > +	up_write(&kni_list_lock);
> > +
> > +	return ret;
> > +}
> > +
> > +static int
> >  kni_ioctl(struct inode *inode,
> >  	unsigned int ioctl_num,
> >  	unsigned long ioctl_param)
> > @@ -569,6 +605,9 @@ kni_ioctl(struct inode *inode,
> >  	case _IOC_NR(RTE_KNI_IOCTL_RELEASE):
> >  		ret = kni_ioctl_release(ioctl_num, ioctl_param);
> >  		break;
> > +	case _IOC_NR(RTE_KNI_IOCTL_LINK_UPDATE):
> > +		ret = kni_ioctl_update_link_state(ioctl_num, ioctl_param);
> > +		break;
> >  	default:
> >  		KNI_DBG("IOCTL default \n");
> >  		break;
> > diff --git a/lib/librte_kni/rte_kni.c b/lib/librte_kni/rte_kni.c index
> > 4e70fa0..b6eda8a 100644
> > --- a/lib/librte_kni/rte_kni.c
> > +++ b/lib/librte_kni/rte_kni.c
> > @@ -512,6 +512,24 @@ rte_kni_release(struct rte_kni *kni)  }
> >
> >  int
> > +rte_kni_update_link_state(struct rte_kni *kni, uint8_t if_up) {
> > +	struct rte_kni_device_info dev_info;
> > +
> > +	if (!kni || !kni->in_use)
> > +		return -1;
> > +
> > +	snprintf(dev_info.name, sizeof(dev_info.name), "%s", kni->name);
> > +	dev_info.link_state = if_up;
> > +	if (ioctl(kni_fd, RTE_KNI_IOCTL_LINK_UPDATE, &dev_info) < 0) {
> > +		RTE_LOG(ERR, KNI, "Fail to update link state\n");
> > +		return -1;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> This new interface should be called at the end of rte_kni_alloc() to notify the link
> status after a KNI interface is created.
> 
> Thanks,
> Helin
> 
> > +int
> >  rte_kni_handle_request(struct rte_kni *kni)  {
> >  	unsigned ret;
> > diff --git a/lib/librte_kni/rte_kni.h b/lib/librte_kni/rte_kni.h index
> > 98edd72..a1bafd9 100644
> > --- a/lib/librte_kni/rte_kni.h
> > +++ b/lib/librte_kni/rte_kni.h
> > @@ -167,6 +167,23 @@ extern struct rte_kni *rte_kni_create(uint8_t
> > port_id,  extern int rte_kni_release(struct rte_kni *kni);
> >
> >  /**
> > + * Send link state changes to KNI interface in kernel space
> > + *
> > + * rte_kni_update_link_state is thread safe.
> > + *
> > + * @param kni
> > + *  The pointer to the context of an existent KNI interface.
> > + * @param if_up
> > + *  interface link status
> > + *
> > + * @return
> > + *  - 0 indicates success.
> > + *  - negative value indicates failure.
> > + */
> > +
> > +extern int rte_kni_update_link_state(struct rte_kni *kni, uint8_t
> > +if_up);
> > +
> > +/**
> >   * It is used to handle the request mbufs sent from kernel space.
> >   * Then analyzes it and calls the specific actions for the specific requests.
> >   * Finally constructs the response mbuf and puts it back to the resp_q.
> > --
> > 1.8.1.4

Re: [dpdk-dev] [PATCH] kni: Add link status update

[Vijayakumar Muthuvel Manickam]

Hi Helin,

Since Stepthen pointed out ioctl is not the best way to add this facility,
I have a patch that will enable link status update through sysfs by
implementing .ndo_change_carrier. I will submit it today.
I will submit a separate patch for transferring link speed,duplex etc.,
this week.

Thanks,
Vijay

On Sun, Jun 14, 2015 at 5:57 PM, Zhang, Helin <helin.zhang@intel.com> wrote:

> Any response for my comments? Did I miss anything?
>
> - Helin
>
> > -----Original Message-----
> > From: Zhang, Helin
> > Sent: Tuesday, May 19, 2015 8:54 AM
> > To: Vijayakumar Muthuvel Manickam; dev@dpdk.org
> > Subject: RE: [dpdk-dev] [PATCH] kni: Add link status update
> >
> > Hello
> >
> > > -----Original Message-----
> > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Vijayakumar
> > > Muthuvel Manickam
> > > Sent: Tuesday, May 19, 2015 7:33 AM
> > > To: dev@dpdk.org
> > > Subject: [dpdk-dev] [PATCH] kni: Add link status update
> > >
> > > Add an ioctl command in rte_kni module to enable DPDK applications to
> > > propagate link state changes to kni virtual interfaces.
> > >
> > > Signed-off-by: Vijayakumar Muthuvel Manickam <mmvijay@gmail.com>
> > > ---
> > >  .../linuxapp/eal/include/exec-env/rte_kni_common.h |  2 ++
> > >  lib/librte_eal/linuxapp/kni/kni_misc.c             | 39
> > > ++++++++++++++++++++++
> > >  lib/librte_kni/rte_kni.c                           | 18 ++++++++++
> > >  lib/librte_kni/rte_kni.h                           | 17 ++++++++++
> > >  4 files changed, 76 insertions(+)
> > >
> > > diff --git
> > > a/lib/librte_eal/linuxapp/eal/include/exec-env/rte_kni_common.h
> > > b/lib/librte_eal/linuxapp/eal/include/exec-env/rte_kni_common.h
> > > index 1e55c2d..b68001d 100644
> > > --- a/lib/librte_eal/linuxapp/eal/include/exec-env/rte_kni_common.h
> > > +++ b/lib/librte_eal/linuxapp/eal/include/exec-env/rte_kni_common.h
> > > @@ -163,6 +163,7 @@ struct rte_kni_device_info {
> > >
> > >     /* mbuf size */
> > >     unsigned mbuf_size;
> > > +   uint8_t link_state;
> > How about transfer more states from user space to kernel space, such as
> link
> > speed, duplex, etc?
> >
> > >  };
> > >
> > >  #define KNI_DEVICE "kni"
> > > @@ -170,5 +171,6 @@ struct rte_kni_device_info {
> > >  #define RTE_KNI_IOCTL_TEST    _IOWR(0, 1, int)
> > >  #define RTE_KNI_IOCTL_CREATE  _IOWR(0, 2, struct
> > > rte_kni_device_info)  #define RTE_KNI_IOCTL_RELEASE _IOWR(0, 3, struct
> > > rte_kni_device_info)
> > > +#define RTE_KNI_IOCTL_LINK_UPDATE _IOWR(0, 4, struct
> > > +rte_kni_device_info)
> > >
> > >  #endif /* _RTE_KNI_COMMON_H_ */
> > > diff --git a/lib/librte_eal/linuxapp/kni/kni_misc.c
> > > b/lib/librte_eal/linuxapp/kni/kni_misc.c
> > > index 1935d32..b1015cd 100644
> > > --- a/lib/librte_eal/linuxapp/kni/kni_misc.c
> > > +++ b/lib/librte_eal/linuxapp/kni/kni_misc.c
> > > @@ -548,6 +548,42 @@ kni_ioctl_release(unsigned int ioctl_num,
> > > unsigned long ioctl_param)  }
> > >
> > >  static int
> > > +kni_ioctl_update_link_state(unsigned int ioctl_num, unsigned long
> > > +ioctl_param) {
> > > +   int ret = -EINVAL;
> > > +   struct kni_dev *dev, *n;
> > > +   struct rte_kni_device_info dev_info;
> > > +
> > > +   if (_IOC_SIZE(ioctl_num) > sizeof(dev_info))
> > > +                   return -EINVAL;
> > > +
> > > +   ret = copy_from_user(&dev_info, (void *)ioctl_param,
> > > sizeof(dev_info));
> > > +   if (ret) {
> > > +           KNI_ERR("copy_from_user in kni_ioctl_update_link_status");
> > > +           return -EIO;
> > > +   }
> > > +
> > > +   if (strlen(dev_info.name) == 0)
> > > +           return ret;
> > > +
> > > +   down_write(&kni_list_lock);
> > > +   list_for_each_entry_safe(dev, n, &kni_list_head, list) {
> > > +           if (strncmp(dev->name, dev_info.name, RTE_KNI_NAMESIZE) !=
> > > 0)
> > > +                   continue;
> > > +
> > > +           if (dev_info.link_state == 0)
> > > +                   netif_carrier_off(dev->net_dev);
> > > +           else
> > > +                   netif_carrier_on(dev->net_dev);
> > > +           ret = 0;
> > > +           break;
> > > +   }
> > > +   up_write(&kni_list_lock);
> > > +
> > > +   return ret;
> > > +}
> > > +
> > > +static int
> > >  kni_ioctl(struct inode *inode,
> > >     unsigned int ioctl_num,
> > >     unsigned long ioctl_param)
> > > @@ -569,6 +605,9 @@ kni_ioctl(struct inode *inode,
> > >     case _IOC_NR(RTE_KNI_IOCTL_RELEASE):
> > >             ret = kni_ioctl_release(ioctl_num, ioctl_param);
> > >             break;
> > > +   case _IOC_NR(RTE_KNI_IOCTL_LINK_UPDATE):
> > > +           ret = kni_ioctl_update_link_state(ioctl_num, ioctl_param);
> > > +           break;
> > >     default:
> > >             KNI_DBG("IOCTL default \n");
> > >             break;
> > > diff --git a/lib/librte_kni/rte_kni.c b/lib/librte_kni/rte_kni.c index
> > > 4e70fa0..b6eda8a 100644
> > > --- a/lib/librte_kni/rte_kni.c
> > > +++ b/lib/librte_kni/rte_kni.c
> > > @@ -512,6 +512,24 @@ rte_kni_release(struct rte_kni *kni)  }
> > >
> > >  int
> > > +rte_kni_update_link_state(struct rte_kni *kni, uint8_t if_up) {
> > > +   struct rte_kni_device_info dev_info;
> > > +
> > > +   if (!kni || !kni->in_use)
> > > +           return -1;
> > > +
> > > +   snprintf(dev_info.name, sizeof(dev_info.name), "%s", kni->name);
> > > +   dev_info.link_state = if_up;
> > > +   if (ioctl(kni_fd, RTE_KNI_IOCTL_LINK_UPDATE, &dev_info) < 0) {
> > > +           RTE_LOG(ERR, KNI, "Fail to update link state\n");
> > > +           return -1;
> > > +   }
> > > +
> > > +   return 0;
> > > +}
> > > +
> > This new interface should be called at the end of rte_kni_alloc() to
> notify the link
> > status after a KNI interface is created.
> >
> > Thanks,
> > Helin
> >
> > > +int
> > >  rte_kni_handle_request(struct rte_kni *kni)  {
> > >     unsigned ret;
> > > diff --git a/lib/librte_kni/rte_kni.h b/lib/librte_kni/rte_kni.h index
> > > 98edd72..a1bafd9 100644
> > > --- a/lib/librte_kni/rte_kni.h
> > > +++ b/lib/librte_kni/rte_kni.h
> > > @@ -167,6 +167,23 @@ extern struct rte_kni *rte_kni_create(uint8_t
> > > port_id,  extern int rte_kni_release(struct rte_kni *kni);
> > >
> > >  /**
> > > + * Send link state changes to KNI interface in kernel space
> > > + *
> > > + * rte_kni_update_link_state is thread safe.
> > > + *
> > > + * @param kni
> > > + *  The pointer to the context of an existent KNI interface.
> > > + * @param if_up
> > > + *  interface link status
> > > + *
> > > + * @return
> > > + *  - 0 indicates success.
> > > + *  - negative value indicates failure.
> > > + */
> > > +
> > > +extern int rte_kni_update_link_state(struct rte_kni *kni, uint8_t
> > > +if_up);
> > > +
> > > +/**
> > >   * It is used to handle the request mbufs sent from kernel space.
> > >   * Then analyzes it and calls the specific actions for the specific
> requests.
> > >   * Finally constructs the response mbuf and puts it back to the
> resp_q.
> > > --
> > > 1.8.1.4
>
>

Re: [dpdk-dev] [PATCH] kni: Add link status update

[Zhang, Helin]

That’s great. Thanks!


-          Helin

From: Vijayakumar Muthuvel Manickam [mailto:mmvijay@gmail.com]
Sent: Monday, June 15, 2015 9:11 AM
To: Zhang, Helin
Cc: thomas.monjalon@6wind.com; dev@dpdk.org
Subject: Re: [dpdk-dev] [PATCH] kni: Add link status update

Hi Helin,

Since Stepthen pointed out ioctl is not the best way to add this facility, I have a patch that will enable link status update through sysfs by implementing .ndo_change_carrier. I will submit it today.
I will submit a separate patch for transferring link speed,duplex etc., this week.

Thanks,
Vijay

On Sun, Jun 14, 2015 at 5:57 PM, Zhang, Helin <helin.zhang@intel.com<mailto:helin.zhang@intel.com>> wrote:
Any response for my comments? Did I miss anything?

- Helin

> -----Original Message-----
> From: Zhang, Helin
> Sent: Tuesday, May 19, 2015 8:54 AM
> To: Vijayakumar Muthuvel Manickam; dev@dpdk.org<mailto:dev@dpdk.org>
> Subject: RE: [dpdk-dev] [PATCH] kni: Add link status update
>
> Hello
>
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org<mailto:dev-bounces@dpdk.org>] On Behalf Of Vijayakumar
> > Muthuvel Manickam
> > Sent: Tuesday, May 19, 2015 7:33 AM
> > To: dev@dpdk.org<mailto:dev@dpdk.org>
> > Subject: [dpdk-dev] [PATCH] kni: Add link status update
> >
> > Add an ioctl command in rte_kni module to enable DPDK applications to
> > propagate link state changes to kni virtual interfaces.
> >
> > Signed-off-by: Vijayakumar Muthuvel Manickam <mmvijay@gmail.com<mailto:mmvijay@gmail.com>>
> > ---
> >  .../linuxapp/eal/include/exec-env/rte_kni_common.h |  2 ++
> >  lib/librte_eal/linuxapp/kni/kni_misc.c             | 39
> > ++++++++++++++++++++++
> >  lib/librte_kni/rte_kni.c                           | 18 ++++++++++
> >  lib/librte_kni/rte_kni.h                           | 17 ++++++++++
> >  4 files changed, 76 insertions(+)
> >
> > diff --git
> > a/lib/librte_eal/linuxapp/eal/include/exec-env/rte_kni_common.h
> > b/lib/librte_eal/linuxapp/eal/include/exec-env/rte_kni_common.h
> > index 1e55c2d..b68001d 100644
> > --- a/lib/librte_eal/linuxapp/eal/include/exec-env/rte_kni_common.h
> > +++ b/lib/librte_eal/linuxapp/eal/include/exec-env/rte_kni_common.h
> > @@ -163,6 +163,7 @@ struct rte_kni_device_info {
> >
> >     /* mbuf size */
> >     unsigned mbuf_size;
> > +   uint8_t link_state;
> How about transfer more states from user space to kernel space, such as link
> speed, duplex, etc?
>
> >  };
> >
> >  #define KNI_DEVICE "kni"
> > @@ -170,5 +171,6 @@ struct rte_kni_device_info {
> >  #define RTE_KNI_IOCTL_TEST    _IOWR(0, 1, int)
> >  #define RTE_KNI_IOCTL_CREATE  _IOWR(0, 2, struct
> > rte_kni_device_info)  #define RTE_KNI_IOCTL_RELEASE _IOWR(0, 3, struct
> > rte_kni_device_info)
> > +#define RTE_KNI_IOCTL_LINK_UPDATE _IOWR(0, 4, struct
> > +rte_kni_device_info)
> >
> >  #endif /* _RTE_KNI_COMMON_H_ */
> > diff --git a/lib/librte_eal/linuxapp/kni/kni_misc.c
> > b/lib/librte_eal/linuxapp/kni/kni_misc.c
> > index 1935d32..b1015cd 100644
> > --- a/lib/librte_eal/linuxapp/kni/kni_misc.c
> > +++ b/lib/librte_eal/linuxapp/kni/kni_misc.c
> > @@ -548,6 +548,42 @@ kni_ioctl_release(unsigned int ioctl_num,
> > unsigned long ioctl_param)  }
> >
> >  static int
> > +kni_ioctl_update_link_state(unsigned int ioctl_num, unsigned long
> > +ioctl_param) {
> > +   int ret = -EINVAL;
> > +   struct kni_dev *dev, *n;
> > +   struct rte_kni_device_info dev_info;
> > +
> > +   if (_IOC_SIZE(ioctl_num) > sizeof(dev_info))
> > +                   return -EINVAL;
> > +
> > +   ret = copy_from_user(&dev_info, (void *)ioctl_param,
> > sizeof(dev_info));
> > +   if (ret) {
> > +           KNI_ERR("copy_from_user in kni_ioctl_update_link_status");
> > +           return -EIO;
> > +   }
> > +
> > +   if (strlen(dev_info.name<http://dev_info.name>) == 0)
> > +           return ret;
> > +
> > +   down_write(&kni_list_lock);
> > +   list_for_each_entry_safe(dev, n, &kni_list_head, list) {
> > +           if (strncmp(dev->name, dev_info.name<http://dev_info.name>, RTE_KNI_NAMESIZE) !=
> > 0)
> > +                   continue;
> > +
> > +           if (dev_info.link_state == 0)
> > +                   netif_carrier_off(dev->net_dev);
> > +           else
> > +                   netif_carrier_on(dev->net_dev);
> > +           ret = 0;
> > +           break;
> > +   }
> > +   up_write(&kni_list_lock);
> > +
> > +   return ret;
> > +}
> > +
> > +static int
> >  kni_ioctl(struct inode *inode,
> >     unsigned int ioctl_num,
> >     unsigned long ioctl_param)
> > @@ -569,6 +605,9 @@ kni_ioctl(struct inode *inode,
> >     case _IOC_NR(RTE_KNI_IOCTL_RELEASE):
> >             ret = kni_ioctl_release(ioctl_num, ioctl_param);
> >             break;
> > +   case _IOC_NR(RTE_KNI_IOCTL_LINK_UPDATE):
> > +           ret = kni_ioctl_update_link_state(ioctl_num, ioctl_param);
> > +           break;
> >     default:
> >             KNI_DBG("IOCTL default \n");
> >             break;
> > diff --git a/lib/librte_kni/rte_kni.c b/lib/librte_kni/rte_kni.c index
> > 4e70fa0..b6eda8a 100644
> > --- a/lib/librte_kni/rte_kni.c
> > +++ b/lib/librte_kni/rte_kni.c
> > @@ -512,6 +512,24 @@ rte_kni_release(struct rte_kni *kni)  }
> >
> >  int
> > +rte_kni_update_link_state(struct rte_kni *kni, uint8_t if_up) {
> > +   struct rte_kni_device_info dev_info;
> > +
> > +   if (!kni || !kni->in_use)
> > +           return -1;
> > +
> > +   snprintf(dev_info.name<http://dev_info.name>, sizeof(dev_info.name<http://dev_info.name>), "%s", kni->name);
> > +   dev_info.link_state = if_up;
> > +   if (ioctl(kni_fd, RTE_KNI_IOCTL_LINK_UPDATE, &dev_info) < 0) {
> > +           RTE_LOG(ERR, KNI, "Fail to update link state\n");
> > +           return -1;
> > +   }
> > +
> > +   return 0;
> > +}
> > +
> This new interface should be called at the end of rte_kni_alloc() to notify the link
> status after a KNI interface is created.
>
> Thanks,
> Helin
>
> > +int
> >  rte_kni_handle_request(struct rte_kni *kni)  {
> >     unsigned ret;
> > diff --git a/lib/librte_kni/rte_kni.h b/lib/librte_kni/rte_kni.h index
> > 98edd72..a1bafd9 100644
> > --- a/lib/librte_kni/rte_kni.h
> > +++ b/lib/librte_kni/rte_kni.h
> > @@ -167,6 +167,23 @@ extern struct rte_kni *rte_kni_create(uint8_t
> > port_id,  extern int rte_kni_release(struct rte_kni *kni);
> >
> >  /**
> > + * Send link state changes to KNI interface in kernel space
> > + *
> > + * rte_kni_update_link_state is thread safe.
> > + *
> > + * @param kni
> > + *  The pointer to the context of an existent KNI interface.
> > + * @param if_up
> > + *  interface link status
> > + *
> > + * @return
> > + *  - 0 indicates success.
> > + *  - negative value indicates failure.
> > + */
> > +
> > +extern int rte_kni_update_link_state(struct rte_kni *kni, uint8_t
> > +if_up);
> > +
> > +/**
> >   * It is used to handle the request mbufs sent from kernel space.
> >   * Then analyzes it and calls the specific actions for the specific requests.
> >   * Finally constructs the response mbuf and puts it back to the resp_q.
> > --
> > 1.8.1.4

[dpdk-dev] [PATCH v2] kni: Add link status update

[Vijayakumar Muthuvel Manickam]

Implement .ndo_change_carrier to enable
DPDK applications to propagate link state changes to
kni virtual interfaces through sysfs

Signed-off-by: Vijayakumar Muthuvel Manickam <mmvijay@gmail.com>
---
Implemented .ndo_change_carrier instead of adding a new ioctl

 lib/librte_eal/linuxapp/kni/kni_net.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/lib/librte_eal/linuxapp/kni/kni_net.c b/lib/librte_eal/linuxapp/kni/kni_net.c
index dd95db5..10c94ce 100644
--- a/lib/librte_eal/linuxapp/kni/kni_net.c
+++ b/lib/librte_eal/linuxapp/kni/kni_net.c
@@ -632,6 +632,15 @@ static int kni_net_set_mac(struct net_device *netdev, void *p)
 	return 0;
 }
 
+static int kni_net_change_carrier(struct net_device *dev, bool new_carrier)
+{
+	if (new_carrier)
+		netif_carrier_on(dev);
+	else
+		netif_carrier_off(dev);
+	return 0;
+}
+
 static const struct header_ops kni_net_header_ops = {
 	.create  = kni_net_header,
 	.rebuild = kni_net_rebuild_header,
@@ -648,6 +657,7 @@ static const struct net_device_ops kni_net_netdev_ops = {
 	.ndo_get_stats = kni_net_stats,
 	.ndo_tx_timeout = kni_net_tx_timeout,
 	.ndo_set_mac_address = kni_net_set_mac,
+	.ndo_change_carrier = kni_net_change_carrier,
 };
 
 void
-- 
1.8.1.4

Re: [dpdk-dev] [PATCH v2] kni: Add link status update

[Stephen Hemminger]

On Mon, 15 Jun 2015 04:21:43 -0400
Vijayakumar Muthuvel Manickam <mmvijay@gmail.com> wrote:

> Implement .ndo_change_carrier to enable
> DPDK applications to propagate link state changes to
> kni virtual interfaces through sysfs
> 
> Signed-off-by: Vijayakumar Muthuvel Manickam <mmvijay@gmail.com>
> ---
> Implemented .ndo_change_carrier instead of adding a new ioctl
> 
>  lib/librte_eal/linuxapp/kni/kni_net.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/lib/librte_eal/linuxapp/kni/kni_net.c b/lib/librte_eal/linuxapp/kni/kni_net.c
> index dd95db5..10c94ce 100644
> --- a/lib/librte_eal/linuxapp/kni/kni_net.c
> +++ b/lib/librte_eal/linuxapp/kni/kni_net.c
> @@ -632,6 +632,15 @@ static int kni_net_set_mac(struct net_device *netdev, void *p)
>  	return 0;
>  }
>  
> +static int kni_net_change_carrier(struct net_device *dev, bool new_carrier)
> +{
> +	if (new_carrier)
> +		netif_carrier_on(dev);
> +	else
> +		netif_carrier_off(dev);
> +	return 0;
> +}
> +
>  static const struct header_ops kni_net_header_ops = {
>  	.create  = kni_net_header,
>  	.rebuild = kni_net_rebuild_header,
> @@ -648,6 +657,7 @@ static const struct net_device_ops kni_net_netdev_ops = {
>  	.ndo_get_stats = kni_net_stats,
>  	.ndo_tx_timeout = kni_net_tx_timeout,
>  	.ndo_set_mac_address = kni_net_set_mac,
> +	.ndo_change_carrier = kni_net_change_carrier,
>  };
>  
>  void

Acked-by: Stephen Hemminger <stephen@networkplumber.org>

You might need to have a kernel version test?? since carrier change was
introduced in 3.9 kernel

[dpdk-dev] [PATCH v3] kni: Add link status update

[Vijayakumar Muthuvel Manickam]

Implement .ndo_change_carrier to enable
DPDK applications to propagate link state changes to
kni virtual interfaces through sysfs

Signed-off-by: Vijayakumar Muthuvel Manickam <mmvijay@gmail.com>
---
Added kernel version check as .ndo_change_carrier is available only in kernel 
versions 3.9 and after

 lib/librte_eal/linuxapp/kni/kni_net.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/lib/librte_eal/linuxapp/kni/kni_net.c b/lib/librte_eal/linuxapp/kni/kni_net.c
index dd95db5..9f9022b 100644
--- a/lib/librte_eal/linuxapp/kni/kni_net.c
+++ b/lib/librte_eal/linuxapp/kni/kni_net.c
@@ -632,6 +632,17 @@ static int kni_net_set_mac(struct net_device *netdev, void *p)
 	return 0;
 }
 
+#if (LINUX_VERSION_CODE >= KERNEL_VERSION(3, 9, 0))
+static int kni_net_change_carrier(struct net_device *dev, bool new_carrier)
+{
+	if (new_carrier)
+		netif_carrier_on(dev);
+	else
+		netif_carrier_off(dev);
+	return 0;
+}
+#endif
+
 static const struct header_ops kni_net_header_ops = {
 	.create  = kni_net_header,
 	.rebuild = kni_net_rebuild_header,
@@ -648,6 +659,9 @@ static const struct net_device_ops kni_net_netdev_ops = {
 	.ndo_get_stats = kni_net_stats,
 	.ndo_tx_timeout = kni_net_tx_timeout,
 	.ndo_set_mac_address = kni_net_set_mac,
+#if (LINUX_VERSION_CODE >= KERNEL_VERSION(3, 9, 0))
+	.ndo_change_carrier = kni_net_change_carrier,
+#endif
 };
 
 void
-- 
1.8.1.4

Re: [dpdk-dev] [PATCH v3] kni: Add link status update

[Zhang, Helin]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Vijayakumar Muthuvel
> Manickam
> Sent: Tuesday, June 16, 2015 2:40 PM
> To: dev@dpdk.org
> Subject: [dpdk-dev] [PATCH v3] kni: Add link status update
> 
> Implement .ndo_change_carrier to enable
> DPDK applications to propagate link state changes to kni virtual interfaces
> through sysfs
> 
> Signed-off-by: Vijayakumar Muthuvel Manickam <mmvijay@gmail.com>
Acked-by: Helin Zhang <helin.zhang@intel.com>

> ---
> Added kernel version check as .ndo_change_carrier is available only in kernel
> versions 3.9 and after
> 
>  lib/librte_eal/linuxapp/kni/kni_net.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/lib/librte_eal/linuxapp/kni/kni_net.c
> b/lib/librte_eal/linuxapp/kni/kni_net.c
> index dd95db5..9f9022b 100644
> --- a/lib/librte_eal/linuxapp/kni/kni_net.c
> +++ b/lib/librte_eal/linuxapp/kni/kni_net.c
> @@ -632,6 +632,17 @@ static int kni_net_set_mac(struct net_device *netdev,
> void *p)
>  	return 0;
>  }
> 
> +#if (LINUX_VERSION_CODE >= KERNEL_VERSION(3, 9, 0)) static int
> +kni_net_change_carrier(struct net_device *dev, bool new_carrier) {
> +	if (new_carrier)
> +		netif_carrier_on(dev);
> +	else
> +		netif_carrier_off(dev);
> +	return 0;
> +}
> +#endif
> +
>  static const struct header_ops kni_net_header_ops = {
>  	.create  = kni_net_header,
>  	.rebuild = kni_net_rebuild_header,
> @@ -648,6 +659,9 @@ static const struct net_device_ops kni_net_netdev_ops =
> {
>  	.ndo_get_stats = kni_net_stats,
>  	.ndo_tx_timeout = kni_net_tx_timeout,
>  	.ndo_set_mac_address = kni_net_set_mac,
> +#if (LINUX_VERSION_CODE >= KERNEL_VERSION(3, 9, 0))
> +	.ndo_change_carrier = kni_net_change_carrier, #endif
>  };
> 
>  void
> --
> 1.8.1.4

Re: [dpdk-dev] [PATCH v3] kni: Add link status update

[Thomas Monjalon]

> > Implement .ndo_change_carrier to enable
> > DPDK applications to propagate link state changes to kni virtual interfaces
> > through sysfs
> > 
> > Signed-off-by: Vijayakumar Muthuvel Manickam <mmvijay@gmail.com>
> Acked-by: Helin Zhang <helin.zhang@intel.com>

Applied, thanks