[dpdk-dev] [PATCH 0/9] kni: cleanups and improvements

[dpdk-dev] [PATCH 0/9] kni: cleanups and improvements

[Stephen Hemminger]

Going over using KNI with netvsc, saw lots of remove
for improvment (deleting code).

This is still mostly "putting lipstick on a pig" all users
would be better off using virtio_user rather than KNI.

Stephen Hemminger (9):
  kni: don't need stubs for rx_mode or ioctl
  kni: use netdev_alloc_skb
  kni: drop unused field
  kni: don't keep stats in kni_net
  kni: drop unused group_id and device_id
  kni: drop unused status element
  kni: use proper type for kni fifo's
  kni: return -EFAULT if copy_from_user fails
  doc: update KNI documentation

 .../sample_app_ug/kernel_nic_interface.rst    | 18 ++---
 kernel/linux/kni/kni_dev.h                    | 21 ++---
 kernel/linux/kni/kni_misc.c                   | 17 ++--
 kernel/linux/kni/kni_net.c                    | 79 +++++--------------
 4 files changed, 38 insertions(+), 97 deletions(-)

-- 
2.20.1

[dpdk-dev] [PATCH 1/9] kni: don't need stubs for rx_mode or ioctl

[Stephen Hemminger]

The netdev subsystem already handles case where
network sevice does not support ioctl.

If device has no rx_mode hook it is not called.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 kernel/linux/kni/kni_net.c | 19 -------------------
 1 file changed, 19 deletions(-)

diff --git a/kernel/linux/kni/kni_net.c b/kernel/linux/kni/kni_net.c
index ad8365877cda..c86337d099ab 100644
--- a/kernel/linux/kni/kni_net.c
+++ b/kernel/linux/kni/kni_net.c
@@ -593,23 +593,6 @@ kni_net_tx_timeout(struct net_device *dev)
 	netif_wake_queue(dev);
 }
 
-/*
- * Ioctl commands
- */
-static int
-kni_net_ioctl(struct net_device *dev, struct ifreq *rq, int cmd)
-{
-	pr_debug("kni_net_ioctl group:%d cmd:%d\n",
-		((struct kni_dev *)netdev_priv(dev))->group_id, cmd);
-
-	return -EOPNOTSUPP;
-}
-
-static void
-kni_net_set_rx_mode(struct net_device *dev)
-{
-}
-
 static int
 kni_net_change_mtu(struct net_device *dev, int new_mtu)
 {
@@ -758,8 +741,6 @@ static const struct net_device_ops kni_net_netdev_ops = {
 	.ndo_change_rx_flags = kni_net_set_promiscusity,
 	.ndo_start_xmit = kni_net_tx,
 	.ndo_change_mtu = kni_net_change_mtu,
-	.ndo_do_ioctl = kni_net_ioctl,
-	.ndo_set_rx_mode = kni_net_set_rx_mode,
 	.ndo_get_stats = kni_net_stats,
 	.ndo_tx_timeout = kni_net_tx_timeout,
 	.ndo_set_mac_address = kni_net_set_mac,
-- 
2.20.1

[dpdk-dev] [PATCH 2/9] kni: use netdev_alloc_skb

[Stephen Hemminger]

netdev_alloc_skb is optimized to any alignment or setup
of skb->dev that is required. The kernel has chosen to not pad
packets on x86 (for many years), because it is faster.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 kernel/linux/kni/kni_net.c | 17 +++--------------
 1 file changed, 3 insertions(+), 14 deletions(-)

diff --git a/kernel/linux/kni/kni_net.c b/kernel/linux/kni/kni_net.c
index c86337d099ab..2f2c30cc13ff 100644
--- a/kernel/linux/kni/kni_net.c
+++ b/kernel/linux/kni/kni_net.c
@@ -340,16 +340,13 @@ kni_net_rx_normal(struct kni_dev *kni)
 		data_kva = kva2data_kva(kva);
 		kni->va[i] = pa2va(kni->pa[i], kva);
 
-		skb = dev_alloc_skb(len + 2);
+		skb = netdev_alloc_skb(dev, len + 2);
 		if (!skb) {
 			/* Update statistics */
 			kni->stats.rx_dropped++;
 			continue;
 		}
 
-		/* Align IP on 16B boundary */
-		skb_reserve(skb, 2);
-
 		if (kva->nb_segs == 1) {
 			memcpy(skb_put(skb, len), data_kva, len);
 		} else {
@@ -368,7 +365,6 @@ kni_net_rx_normal(struct kni_dev *kni)
 			}
 		}
 
-		skb->dev = dev;
 		skb->protocol = eth_type_trans(skb, dev);
 		skb->ip_summed = CHECKSUM_UNNECESSARY;
 
@@ -512,26 +508,20 @@ kni_net_rx_lo_fifo_skb(struct kni_dev *kni)
 		data_kva = kva2data_kva(kva);
 		kni->va[i] = pa2va(kni->pa[i], kva);
 
-		skb = dev_alloc_skb(len + 2);
+		skb = netdev_alloc_skb(dev, len + 2);
 		if (skb) {
-			/* Align IP on 16B boundary */
-			skb_reserve(skb, 2);
 			memcpy(skb_put(skb, len), data_kva, len);
-			skb->dev = dev;
 			skb->ip_summed = CHECKSUM_UNNECESSARY;
 			dev_kfree_skb(skb);
 		}
 
 		/* Simulate real usage, allocate/copy skb twice */
-		skb = dev_alloc_skb(len + 2);
+		skb = netdev_alloc_skb(dev, len + 2);
 		if (skb == NULL) {
 			kni->stats.rx_dropped++;
 			continue;
 		}
 
-		/* Align IP on 16B boundary */
-		skb_reserve(skb, 2);
-
 		if (kva->nb_segs == 1) {
 			memcpy(skb_put(skb, len), data_kva, len);
 		} else {
@@ -550,7 +540,6 @@ kni_net_rx_lo_fifo_skb(struct kni_dev *kni)
 			}
 		}
 
-		skb->dev = dev;
 		skb->ip_summed = CHECKSUM_UNNECESSARY;
 
 		kni->stats.rx_bytes += len;
-- 
2.20.1

[dpdk-dev] [PATCH 3/9] kni: drop unused field

[Stephen Hemminger]

The kni net structure only exists in driver no API/ABI.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 kernel/linux/kni/kni_dev.h | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/kernel/linux/kni/kni_dev.h b/kernel/linux/kni/kni_dev.h
index 44a5a61f7279..0117941e5a6c 100644
--- a/kernel/linux/kni/kni_dev.h
+++ b/kernel/linux/kni/kni_dev.h
@@ -83,9 +83,6 @@ struct kni_dev {
 	/* mbuf size */
 	uint32_t mbuf_size;
 
-	/* synchro for request processing */
-	unsigned long synchro;
-
 	/* buffers */
 	void *pa[MBUF_BURST_SZ];
 	void *va[MBUF_BURST_SZ];
-- 
2.20.1

[dpdk-dev] [PATCH 4/9] kni: don't keep stats in kni_net

[Stephen Hemminger]

Since kernel 2.6.28 the network subsystem has provided
dev->stats for devices to use statistics handling and is the
default if no ndo_get_stats is provided.

This allow allows for 64 bit (rather than just 32 bit)
statistics with KNI.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 kernel/linux/kni/kni_dev.h |  1 -
 kernel/linux/kni/kni_net.c | 43 +++++++++++++-------------------------
 2 files changed, 15 insertions(+), 29 deletions(-)

diff --git a/kernel/linux/kni/kni_dev.h b/kernel/linux/kni/kni_dev.h
index 0117941e5a6c..ca3b07678b96 100644
--- a/kernel/linux/kni/kni_dev.h
+++ b/kernel/linux/kni/kni_dev.h
@@ -39,7 +39,6 @@ struct kni_dev {
 	/* kni list */
 	struct list_head list;
 
-	struct net_device_stats stats;
 	int status;
 	uint16_t group_id;           /* Group ID of a group of KNI devices */
 	uint32_t core_id;            /* Core ID to bind */
diff --git a/kernel/linux/kni/kni_net.c b/kernel/linux/kni/kni_net.c
index 2f2c30cc13ff..564b9de3db87 100644
--- a/kernel/linux/kni/kni_net.c
+++ b/kernel/linux/kni/kni_net.c
@@ -291,15 +291,15 @@ kni_net_tx(struct sk_buff *skb, struct net_device *dev)
 
 	/* Free skb and update statistics */
 	dev_kfree_skb(skb);
-	kni->stats.tx_bytes += len;
-	kni->stats.tx_packets++;
+	dev->stats.tx_bytes += len;
+	dev->stats.tx_packets++;
 
 	return NETDEV_TX_OK;
 
 drop:
 	/* Free skb and update statistics */
 	dev_kfree_skb(skb);
-	kni->stats.tx_dropped++;
+	dev->stats.tx_dropped++;
 
 	return NETDEV_TX_OK;
 }
@@ -343,7 +343,7 @@ kni_net_rx_normal(struct kni_dev *kni)
 		skb = netdev_alloc_skb(dev, len + 2);
 		if (!skb) {
 			/* Update statistics */
-			kni->stats.rx_dropped++;
+			dev->stats.rx_dropped++;
 			continue;
 		}
 
@@ -372,8 +372,8 @@ kni_net_rx_normal(struct kni_dev *kni)
 		netif_rx_ni(skb);
 
 		/* Update statistics */
-		kni->stats.rx_bytes += len;
-		kni->stats.rx_packets++;
+		dev->stats.rx_bytes += len;
+		dev->stats.rx_packets++;
 	}
 
 	/* Burst enqueue mbufs into free_q */
@@ -396,6 +396,7 @@ kni_net_rx_lo_fifo(struct kni_dev *kni)
 	void *data_kva;
 	struct rte_kni_mbuf *alloc_kva;
 	void *alloc_data_kva;
+	struct net_device *dev = kni->net_dev;
 
 	/* Get the number of entries in rx_q */
 	num_rq = kni_fifo_count(kni->rx_q);
@@ -443,8 +444,8 @@ kni_net_rx_lo_fifo(struct kni_dev *kni)
 			alloc_kva->pkt_len = len;
 			alloc_kva->data_len = len;
 
-			kni->stats.tx_bytes += len;
-			kni->stats.rx_bytes += len;
+			dev->stats.tx_bytes += len;
+			dev->stats.rx_bytes += len;
 		}
 
 		/* Burst enqueue mbufs into tx_q */
@@ -464,8 +465,8 @@ kni_net_rx_lo_fifo(struct kni_dev *kni)
 	 * Update statistic, and enqueue/dequeue failure is impossible,
 	 * as all queues are checked at first.
 	 */
-	kni->stats.tx_packets += num;
-	kni->stats.rx_packets += num;
+	dev->stats.tx_packets += num;
+	dev->stats.rx_packets += num;
 }
 
 /*
@@ -518,7 +519,7 @@ kni_net_rx_lo_fifo_skb(struct kni_dev *kni)
 		/* Simulate real usage, allocate/copy skb twice */
 		skb = netdev_alloc_skb(dev, len + 2);
 		if (skb == NULL) {
-			kni->stats.rx_dropped++;
+			dev->stats.rx_dropped++;
 			continue;
 		}
 
@@ -542,8 +543,8 @@ kni_net_rx_lo_fifo_skb(struct kni_dev *kni)
 
 		skb->ip_summed = CHECKSUM_UNNECESSARY;
 
-		kni->stats.rx_bytes += len;
-		kni->stats.rx_packets++;
+		dev->stats.rx_bytes += len;
+		dev->stats.rx_packets++;
 
 		/* call tx interface */
 		kni_net_tx(skb, dev);
@@ -573,12 +574,10 @@ kni_net_rx(struct kni_dev *kni)
 static void
 kni_net_tx_timeout(struct net_device *dev)
 {
-	struct kni_dev *kni = netdev_priv(dev);
-
 	pr_debug("Transmit timeout at %ld, latency %ld\n", jiffies,
 			jiffies - dev_trans_start(dev));
 
-	kni->stats.tx_errors++;
+	dev->stats.tx_errors++;
 	netif_wake_queue(dev);
 }
 
@@ -627,17 +626,6 @@ kni_net_poll_resp(struct kni_dev *kni)
 		wake_up_interruptible(&kni->wq);
 }
 
-/*
- * Return statistics to the caller
- */
-static struct net_device_stats *
-kni_net_stats(struct net_device *dev)
-{
-	struct kni_dev *kni = netdev_priv(dev);
-
-	return &kni->stats;
-}
-
 /*
  *  Fill the eth header
  */
@@ -730,7 +718,6 @@ static const struct net_device_ops kni_net_netdev_ops = {
 	.ndo_change_rx_flags = kni_net_set_promiscusity,
 	.ndo_start_xmit = kni_net_tx,
 	.ndo_change_mtu = kni_net_change_mtu,
-	.ndo_get_stats = kni_net_stats,
 	.ndo_tx_timeout = kni_net_tx_timeout,
 	.ndo_set_mac_address = kni_net_set_mac,
 #ifdef HAVE_CHANGE_CARRIER_CB
-- 
2.20.1

[dpdk-dev] [PATCH 5/9] kni: drop unused group_id and device_id

[Stephen Hemminger]

Kni devices no longer are restricted to PCI and device_id
in kni_dev is no longer used.

The group_id is set but never used.

The fields do need to stay in the ABI (dev_info) but
kernel can ignore them.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 kernel/linux/kni/kni_dev.h  | 4 ----
 kernel/linux/kni/kni_misc.c | 1 -
 2 files changed, 5 deletions(-)

diff --git a/kernel/linux/kni/kni_dev.h b/kernel/linux/kni/kni_dev.h
index ca3b07678b96..e39af97feaa7 100644
--- a/kernel/linux/kni/kni_dev.h
+++ b/kernel/linux/kni/kni_dev.h
@@ -40,7 +40,6 @@ struct kni_dev {
 	struct list_head list;
 
 	int status;
-	uint16_t group_id;           /* Group ID of a group of KNI devices */
 	uint32_t core_id;            /* Core ID to bind */
 	char name[RTE_KNI_NAMESIZE]; /* Network device name */
 	struct task_struct *pthread;
@@ -49,9 +48,6 @@ struct kni_dev {
 	wait_queue_head_t wq;
 	struct mutex sync_lock;
 
-	/* PCI device id */
-	uint16_t device_id;
-
 	/* kni device */
 	struct net_device *net_dev;
 
diff --git a/kernel/linux/kni/kni_misc.c b/kernel/linux/kni/kni_misc.c
index af18c67c422f..6a206d883c0d 100644
--- a/kernel/linux/kni/kni_misc.c
+++ b/kernel/linux/kni/kni_misc.c
@@ -346,7 +346,6 @@ kni_ioctl_create(struct net *net, uint32_t ioctl_num,
 	kni = netdev_priv(net_dev);
 
 	kni->net_dev = net_dev;
-	kni->group_id = dev_info.group_id;
 	kni->core_id = dev_info.core_id;
 	strncpy(kni->name, dev_info.name, RTE_KNI_NAMESIZE);
 
-- 
2.20.1

[dpdk-dev] [PATCH 6/9] kni: drop unused status element

[Stephen Hemminger]

Yet another ethtool leftover.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 kernel/linux/kni/kni_dev.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/kernel/linux/kni/kni_dev.h b/kernel/linux/kni/kni_dev.h
index e39af97feaa7..f3e6c3ca4efa 100644
--- a/kernel/linux/kni/kni_dev.h
+++ b/kernel/linux/kni/kni_dev.h
@@ -39,7 +39,6 @@ struct kni_dev {
 	/* kni list */
 	struct list_head list;
 
-	int status;
 	uint32_t core_id;            /* Core ID to bind */
 	char name[RTE_KNI_NAMESIZE]; /* Network device name */
 	struct task_struct *pthread;
-- 
2.20.1

[dpdk-dev] [PATCH 7/9] kni: use proper type for kni fifo's

[Stephen Hemminger]

Using void * instead of proper type is unsafe practice.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 kernel/linux/kni/kni_dev.h | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/kernel/linux/kni/kni_dev.h b/kernel/linux/kni/kni_dev.h
index f3e6c3ca4efa..ceba5f73c1d9 100644
--- a/kernel/linux/kni/kni_dev.h
+++ b/kernel/linux/kni/kni_dev.h
@@ -51,22 +51,22 @@ struct kni_dev {
 	struct net_device *net_dev;
 
 	/* queue for packets to be sent out */
-	void *tx_q;
+	struct rte_kni_fifo *tx_q;
 
 	/* queue for the packets received */
-	void *rx_q;
+	struct rte_kni_fifo *rx_q;
 
 	/* queue for the allocated mbufs those can be used to save sk buffs */
-	void *alloc_q;
+	struct rte_kni_fifo *alloc_q;
 
 	/* free queue for the mbufs to be freed */
-	void *free_q;
+	struct rte_kni_fifo *free_q;
 
 	/* request queue */
-	void *req_q;
+	struct rte_kni_fifo *req_q;
 
 	/* response queue */
-	void *resp_q;
+	struct rte_kni_fifo *resp_q;
 
 	void *sync_kva;
 	void *sync_va;
-- 
2.20.1

[dpdk-dev] [PATCH 8/9] kni: return -EFAULT if copy_from_user fails

[Stephen Hemminger]

The correct thing to return if user gives a bad data
is to return -EFAULT. Logging is also discouraged because
it could be used as a DoS attack.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 kernel/linux/kni/kni_misc.c | 16 +++++-----------
 1 file changed, 5 insertions(+), 11 deletions(-)

diff --git a/kernel/linux/kni/kni_misc.c b/kernel/linux/kni/kni_misc.c
index 6a206d883c0d..a67991066cd9 100644
--- a/kernel/linux/kni/kni_misc.c
+++ b/kernel/linux/kni/kni_misc.c
@@ -301,11 +301,8 @@ kni_ioctl_create(struct net *net, uint32_t ioctl_num,
 		return -EINVAL;
 
 	/* Copy kni info from user space */
-	ret = copy_from_user(&dev_info, (void *)ioctl_param, sizeof(dev_info));
-	if (ret) {
-		pr_err("copy_from_user in kni_ioctl_create");
-		return -EIO;
-	}
+	if (copy_from_user(&dev_info, (void *)ioctl_param, sizeof(dev_info)))
+		return -EFAULT;
 
 	/* Check if name is zero-ended */
 	if (strnlen(dev_info.name, sizeof(dev_info.name)) == sizeof(dev_info.name)) {
@@ -433,15 +430,12 @@ kni_ioctl_release(struct net *net, uint32_t ioctl_num,
 	if (_IOC_SIZE(ioctl_num) > sizeof(dev_info))
 		return -EINVAL;
 
-	ret = copy_from_user(&dev_info, (void *)ioctl_param, sizeof(dev_info));
-	if (ret) {
-		pr_err("copy_from_user in kni_ioctl_release");
-		return -EIO;
-	}
+	if (copy_from_user(&dev_info, (void *)ioctl_param, sizeof(dev_info)))
+		return -EFAULT;
 
 	/* Release the network device according to its name */
 	if (strlen(dev_info.name) == 0)
-		return ret;
+		return -EINVAL;
 
 	down_write(&knet->kni_list_lock);
 	list_for_each_entry_safe(dev, n, &knet->kni_list_head, list) {
-- 
2.20.1

[dpdk-dev] [PATCH 9/9] doc: update KNI documentation

[Stephen Hemminger]

Make the KNI documentation reflect modern kernel networking.
Ifconfig has been superseded by iproute2 for 15 years.
Iproute2 is well maintained, supports current feature set.

Ethtool is no longer supported by KNI.
Tshark is a better replacement for tcpdump.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 .../sample_app_ug/kernel_nic_interface.rst     | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/doc/guides/sample_app_ug/kernel_nic_interface.rst b/doc/guides/sample_app_ug/kernel_nic_interface.rst
index a7e549d5213a..422bd8c98465 100644
--- a/doc/guides/sample_app_ug/kernel_nic_interface.rst
+++ b/doc/guides/sample_app_ug/kernel_nic_interface.rst
@@ -21,14 +21,14 @@ The FIFO queues contain pointers to data packets in the DPDK. This:
 
 *   Provides a faster mechanism to interface with the kernel net stack and eliminates system calls
 
-*   Facilitates the DPDK using standard Linux* userspace net tools (tcpdump, ftp, and so on)
+*   Facilitates the DPDK using standard Linux* userspace net tools (tshark, rsync, and so on)
 
 *   Eliminate the copy_to_user and copy_from_user operations on packets.
 
 The Kernel NIC Interface sample application is a simple example that demonstrates the use
 of the DPDK to create a path for packets to go through the Linux* kernel.
 This is done by creating one or more kernel net devices for each of the DPDK ports.
-The application allows the use of standard Linux tools (ethtool, ifconfig, tcpdump) with the DPDK ports and
+The application allows the use of standard Linux tools (iproute, tshark) with the DPDK ports and
 also the exchange of packets between the DPDK application and the Linux* kernel.
 
 The Kernel NIC Interface sample application requires that the
@@ -220,13 +220,13 @@ Enable KNI interface and assign an IP address:
 
 .. code-block:: console
 
-    # ifconfig vEth0_0 192.168.0.1
+    # ip addr add dev vEth0_0 192.168.0.1
 
 Show KNI interface configuration and statistics:
 
 .. code-block:: console
 
-    # ifconfig vEth0_0
+    # ip -s -d addr show vEth0_0
 
 The user can also check and reset the packet statistics inside the ``kni``
 application by sending the app the USR1 and USR2 signals:
@@ -234,16 +234,16 @@ application by sending the app the USR1 and USR2 signals:
 .. code-block:: console
 
     # Print statistics
-    # kill -SIGUSR1 `pidof kni`
+    # pkill -USR1 kni
 
     # Zero statistics
-    # kill -SIGUSR2 `pidof kni`
+    # pkill -USR2 kni
 
 Dump network traffic:
 
 .. code-block:: console
 
-    # tcpdump -i vEth0_0
+    # tshark -n -i vEth0_0
 
 The normal Linux commands can also be used to change the MAC address and
 MTU size used by the physical NIC which corresponds to the KNI interface.
@@ -254,13 +254,13 @@ Change the MAC address:
 
 .. code-block:: console
 
-    # ifconfig vEth0_0 hw ether 0C:01:02:03:04:08
+    # ip link set dev vEth0_0 lladdr 0C:01:02:03:04:08
 
 Change the MTU size:
 
 .. code-block:: console
 
-    # ifconfig vEth0_0 mtu 1450
+    # ip link set dev vEth0_0 mtu 1450
 
 When the ``kni`` application is closed, all the KNI interfaces are deleted
 from the Linux kernel.
-- 
2.20.1

Re: [dpdk-dev] [PATCH 2/9] kni: use netdev_alloc_skb

[Stephen Hemminger]

On Fri,  7 Jun 2019 17:19:56 -0700
Stephen Hemminger <stephen@networkplumber.org> wrote:

> +++ b/kernel/linux/kni/kni_net.c
> @@ -340,16 +340,13 @@ kni_net_rx_normal(struct kni_dev *kni)
>  		data_kva = kva2data_kva(kva);
>  		kni->va[i] = pa2va(kni->pa[i], kva);
>  
> -		skb = dev_alloc_skb(len + 2);
> +		skb = netdev_alloc_skb(dev, len + 2);

The +2 is no longer needed. will update in next version