[dpdk-dev] [PATCH] net/tap: perform proto field update for tun only

[dpdk-dev] [PATCH] net/tap: perform proto field update for tun only

[Vipin Varghese]

The TX function is shared between TAP and TUN PMD. Checking TUN-TAP
type field will ensure the TAP PMD will always have protocol field as 0.

Signed-off-by: Vipin Varghese <vipin.varghese@intel.com>
---
 drivers/net/tap/rte_eth_tap.c | 50 +++++++++++++++++++++++++++----------------
 drivers/net/tap/rte_eth_tap.h | 10 +++++++++
 2 files changed, 41 insertions(+), 19 deletions(-)

diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
index ea6d899..98ac3b5 100644
--- a/drivers/net/tap/rte_eth_tap.c
+++ b/drivers/net/tap/rte_eth_tap.c
@@ -115,7 +115,8 @@ tun_alloc(struct pmd_internals *pmd)
 	 * Do not set IFF_NO_PI as packet information header will be needed
 	 * to check if a received packet has been truncated.
 	 */
-	ifr.ifr_flags = (tap_type) ? IFF_TAP : IFF_TUN | IFF_POINTOPOINT;
+	ifr.ifr_flags = (pmd->type == ETH_TUNTAP_TYPE_TAP) ?
+			IFF_TAP : IFF_TUN | IFF_POINTOPOINT;
 	snprintf(ifr.ifr_name, IFNAMSIZ, "%s", pmd->name);
 
 	TAP_LOG(DEBUG, "ifr_name '%s'", ifr.ifr_name);
@@ -502,20 +503,24 @@ pmd_tx_burst(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
 		if (rte_pktmbuf_pkt_len(mbuf) > max_size)
 			break;
 
-		/*
-		 * TUN and TAP are created with IFF_NO_PI disabled.
-		 * For TUN PMD this mandatory as fields are used by
-		 * Kernel tun.c to determine whether its IP or non IP
-		 * packets.
-		 *
-		 * The logic fetches the first byte of data from mbuf.
-		 * compares whether its v4 or v6. If none matches default
-		 * value 0x00 is taken for protocol field.
-		 */
-		char *buff_data = rte_pktmbuf_mtod(seg, void *);
-		j = (*buff_data & 0xf0);
-		pi.proto = (j == 0x40) ? 0x0008 :
-				(j == 0x60) ? 0xdd86 : 0x00;
+		if (txq->type == ETH_TUNTAP_TYPE_TUN) {
+			/*
+			 * TUN and TAP are created with IFF_NO_PI disabled.
+			 * For TUN PMD this mandatory as fields are used by
+			 * Kernel tun.c to determine whether its IP or non IP
+			 * packets.
+			 *
+			 * The logic fetches the first byte of data from mbuf.
+			 * compares whether its v4 or v6. If none match default
+			 * value 0x00 is taken for protocol field.
+			 */
+			char *buff_data = rte_pktmbuf_mtod(seg, void *);
+			j = (*buff_data & 0xf0);
+			pi.proto = (j == 0x40) ? 0x0008 :
+					(j == 0x60) ? 0xdd86 : 0x00;
+			printf("j %x pi proto %x\n", j, pi.proto);
+			rte_pktmbuf_dump(stdout, seg, 64);
+		}
 
 		iovecs[0].iov_base = &pi;
 		iovecs[0].iov_len = sizeof(pi);
@@ -1052,6 +1057,9 @@ tap_setup_queue(struct rte_eth_dev *dev,
 	tx->mtu = &dev->data->mtu;
 	rx->rxmode = &dev->data->dev_conf.rxmode;
 
+	tx->type = pmd->type;
+	rx->type = pmd->type;
+
 	return *fd;
 }
 
@@ -1386,6 +1394,7 @@ eth_dev_tap_create(struct rte_vdev_device *vdev, char *tap_name,
 	pmd = dev->data->dev_private;
 	pmd->dev = dev;
 	snprintf(pmd->name, sizeof(pmd->name), "%s", tap_name);
+	pmd->type = ETH_TUNTAP_TYPE_UNKNOWN;
 
 	pmd->ioctl_sock = socket(AF_INET, SOCK_DGRAM, 0);
 	if (pmd->ioctl_sock == -1) {
@@ -1421,7 +1430,9 @@ eth_dev_tap_create(struct rte_vdev_device *vdev, char *tap_name,
 		pmd->txq[i].fd = -1;
 	}
 
-	if (tap_type) {
+	pmd->type = (tap_type) ? ETH_TUNTAP_TYPE_TAP : ETH_TUNTAP_TYPE_TUN;
+
+	if (pmd->type == ETH_TUNTAP_TYPE_TAP) {
 		if (is_zero_ether_addr(mac_addr))
 			eth_random_addr((uint8_t *)&pmd->eth_addr);
 		else
@@ -1440,7 +1451,7 @@ eth_dev_tap_create(struct rte_vdev_device *vdev, char *tap_name,
 	if (tap_ioctl(pmd, SIOCSIFMTU, &ifr, 1, LOCAL_AND_REMOTE) < 0)
 		goto error_exit;
 
-	if (tap_type) {
+	if (pmd->type == ETH_TUNTAP_TYPE_TAP) {
 		memset(&ifr, 0, sizeof(struct ifreq));
 		ifr.ifr_hwaddr.sa_family = AF_LOCAL;
 		rte_memcpy(ifr.ifr_hwaddr.sa_data, &pmd->eth_addr,
@@ -1812,7 +1823,9 @@ rte_pmd_tap_remove(struct rte_vdev_device *dev)
 	struct pmd_internals *internals;
 	int i;
 
-	TAP_LOG(DEBUG, "Closing TUN/TAP Ethernet device on numa %u",
+	internals = eth_dev->data->dev_private;
+	TAP_LOG(DEBUG, "Closing %s Ethernet device on numa %u",
+		(internals->type == ETH_TUNTAP_TYPE_TAP) ? "TAP" : "TUN",
 		rte_socket_id());
 
 	/* find the ethdev entry */
@@ -1820,7 +1833,6 @@ rte_pmd_tap_remove(struct rte_vdev_device *dev)
 	if (!eth_dev)
 		return 0;
 
-	internals = eth_dev->data->dev_private;
 	if (internals->nlsk_fd) {
 		tap_flow_flush(eth_dev, NULL);
 		tap_flow_implicit_flush(internals, NULL);
diff --git a/drivers/net/tap/rte_eth_tap.h b/drivers/net/tap/rte_eth_tap.h
index 67c9d4b..8b0da5a 100644
--- a/drivers/net/tap/rte_eth_tap.h
+++ b/drivers/net/tap/rte_eth_tap.h
@@ -23,6 +23,13 @@
 #define RTE_PMD_TAP_MAX_QUEUES	1
 #endif
 
+enum rte_tuntap_type {
+	ETH_TUNTAP_TYPE_UNKNOWN,
+	ETH_TUNTAP_TYPE_TUN,
+	ETH_TUNTAP_TYPE_TAP,
+	ETH_TUNTAP_TYPE_MAX,
+};
+
 struct pkt_stats {
 	uint64_t opackets;              /* Number of output packets */
 	uint64_t ipackets;              /* Number of input packets */
@@ -38,6 +45,7 @@ struct rx_queue {
 	uint32_t trigger_seen;          /* Last seen Rx trigger value */
 	uint16_t in_port;               /* Port ID */
 	int fd;
+	int type;			  /* Type field - TUN|TAP */
 	struct pkt_stats stats;         /* Stats for this RX queue */
 	uint16_t nb_rx_desc;            /* max number of mbufs available */
 	struct rte_eth_rxmode *rxmode;  /* RX features */
@@ -48,6 +56,7 @@ struct rx_queue {
 
 struct tx_queue {
 	int fd;
+	int type;			  /* Type field - TUN|TAP */
 	uint16_t *mtu;                  /* Pointer to MTU from dev_data */
 	uint16_t csum:1;                /* Enable checksum offloading */
 	struct pkt_stats stats;         /* Stats for this TX queue */
@@ -57,6 +66,7 @@ struct pmd_internals {
 	struct rte_eth_dev *dev;          /* Ethernet device. */
 	char remote_iface[RTE_ETH_NAME_MAX_LEN]; /* Remote netdevice name */
 	char name[RTE_ETH_NAME_MAX_LEN];  /* Internal Tap device name */
+	int type;			  /* Type field - TUN|TAP */
 	struct ether_addr eth_addr;       /* Mac address of the device port */
 	struct ifreq remote_initial_flags;   /* Remote netdevice flags on init */
 	int remote_if_index;              /* remote netdevice IF_INDEX */
-- 
2.7.4

Re: [dpdk-dev] [PATCH] net/tap: perform proto field update for tun only

[Wiles, Keith]

> On May 13, 2018, at 11:53 PM, Vipin Varghese <vipin.varghese@intel.com> wrote:
> 
> The TX function is shared between TAP and TUN PMD. Checking TUN-TAP
> type field will ensure the TAP PMD will always have protocol field as 0.
> 
> Signed-off-by: Vipin Varghese <vipin.varghese@intel.com>
> ---
> drivers/net/tap/rte_eth_tap.c | 50 +++++++++++++++++++++++++++----------------
> drivers/net/tap/rte_eth_tap.h | 10 +++++++++
> 2 files changed, 41 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
> index ea6d899..98ac3b5 100644
> --- a/drivers/net/tap/rte_eth_tap.c
> +++ b/drivers/net/tap/rte_eth_tap.c
> @@ -115,7 +115,8 @@ tun_alloc(struct pmd_internals *pmd)
> 	 * Do not set IFF_NO_PI as packet information header will be needed
> 	 * to check if a received packet has been truncated.
> 	 */
> -	ifr.ifr_flags = (tap_type) ? IFF_TAP : IFF_TUN | IFF_POINTOPOINT;
> +	ifr.ifr_flags = (pmd->type == ETH_TUNTAP_TYPE_TAP) ?
> +			IFF_TAP : IFF_TUN | IFF_POINTOPOINT;
> 	snprintf(ifr.ifr_name, IFNAMSIZ, "%s", pmd->name);
> 
> 	TAP_LOG(DEBUG, "ifr_name '%s'", ifr.ifr_name);
> @@ -502,20 +503,24 @@ pmd_tx_burst(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
> 		if (rte_pktmbuf_pkt_len(mbuf) > max_size)
> 			break;
> 
> -		/*
> -		 * TUN and TAP are created with IFF_NO_PI disabled.
> -		 * For TUN PMD this mandatory as fields are used by
> -		 * Kernel tun.c to determine whether its IP or non IP
> -		 * packets.
> -		 *
> -		 * The logic fetches the first byte of data from mbuf.
> -		 * compares whether its v4 or v6. If none matches default
> -		 * value 0x00 is taken for protocol field.
> -		 */
> -		char *buff_data = rte_pktmbuf_mtod(seg, void *);
> -		j = (*buff_data & 0xf0);
> -		pi.proto = (j == 0x40) ? 0x0008 :
> -				(j == 0x60) ? 0xdd86 : 0x00;
> +		if (txq->type == ETH_TUNTAP_TYPE_TUN) {
> +			/*
> +			 * TUN and TAP are created with IFF_NO_PI disabled.
> +			 * For TUN PMD this mandatory as fields are used by
> +			 * Kernel tun.c to determine whether its IP or non IP
> +			 * packets.
> +			 *
> +			 * The logic fetches the first byte of data from mbuf.
> +			 * compares whether its v4 or v6. If none match default
> +			 * value 0x00 is taken for protocol field.

Little reword and remove the ‘.’ at end of first line.

The logic fetches the first byte of data from mbuf then compares whether it is v4 or v6. If not equal to zero then it must be a protocol field.



> +			 */
> +			char *buff_data = rte_pktmbuf_mtod(seg, void *);
> +			j = (*buff_data & 0xf0);
> +			pi.proto = (j == 0x40) ? 0x0008 :
> +					(j == 0x60) ? 0xdd86 : 0x00;
> +			printf("j %x pi proto %x\n", j, pi.proto);

Should this not be a LOG message or is this debug that was not removed? If log message then add move text to explain the output better.

> +			rte_pktmbuf_dump(stdout, seg, 64);
> +		}
> 
> 		iovecs[0].iov_base = &pi;
> 		iovecs[0].iov_len = sizeof(pi);
> @@ -1052,6 +1057,9 @@ tap_setup_queue(struct rte_eth_dev *dev,
> 	tx->mtu = &dev->data->mtu;
> 	rx->rxmode = &dev->data->dev_conf.rxmode;
> 
> +	tx->type = pmd->type;
> +	rx->type = pmd->type;
> +
> 	return *fd;
> }
> 
> @@ -1386,6 +1394,7 @@ eth_dev_tap_create(struct rte_vdev_device *vdev, char *tap_name,
> 	pmd = dev->data->dev_private;
> 	pmd->dev = dev;
> 	snprintf(pmd->name, sizeof(pmd->name), "%s", tap_name);
> +	pmd->type = ETH_TUNTAP_TYPE_UNKNOWN;
> 
> 	pmd->ioctl_sock = socket(AF_INET, SOCK_DGRAM, 0);
> 	if (pmd->ioctl_sock == -1) {
> @@ -1421,7 +1430,9 @@ eth_dev_tap_create(struct rte_vdev_device *vdev, char *tap_name,
> 		pmd->txq[i].fd = -1;
> 	}
> 
> -	if (tap_type) {
> +	pmd->type = (tap_type) ? ETH_TUNTAP_TYPE_TAP : ETH_TUNTAP_TYPE_TUN;
> +
> +	if (pmd->type == ETH_TUNTAP_TYPE_TAP) {
> 		if (is_zero_ether_addr(mac_addr))
> 			eth_random_addr((uint8_t *)&pmd->eth_addr);
> 		else
> @@ -1440,7 +1451,7 @@ eth_dev_tap_create(struct rte_vdev_device *vdev, char *tap_name,
> 	if (tap_ioctl(pmd, SIOCSIFMTU, &ifr, 1, LOCAL_AND_REMOTE) < 0)
> 		goto error_exit;
> 
> -	if (tap_type) {
> +	if (pmd->type == ETH_TUNTAP_TYPE_TAP) {
> 		memset(&ifr, 0, sizeof(struct ifreq));
> 		ifr.ifr_hwaddr.sa_family = AF_LOCAL;
> 		rte_memcpy(ifr.ifr_hwaddr.sa_data, &pmd->eth_addr,
> @@ -1812,7 +1823,9 @@ rte_pmd_tap_remove(struct rte_vdev_device *dev)
> 	struct pmd_internals *internals;
> 	int i;
> 
> -	TAP_LOG(DEBUG, "Closing TUN/TAP Ethernet device on numa %u",
> +	internals = eth_dev->data->dev_private;
> +	TAP_LOG(DEBUG, "Closing %s Ethernet device on numa %u",
> +		(internals->type == ETH_TUNTAP_TYPE_TAP) ? "TAP" : "TUN",
> 		rte_socket_id());
> 
> 	/* find the ethdev entry */
> @@ -1820,7 +1833,6 @@ rte_pmd_tap_remove(struct rte_vdev_device *dev)
> 	if (!eth_dev)
> 		return 0;
> 
> -	internals = eth_dev->data->dev_private;
> 	if (internals->nlsk_fd) {
> 		tap_flow_flush(eth_dev, NULL);
> 		tap_flow_implicit_flush(internals, NULL);
> diff --git a/drivers/net/tap/rte_eth_tap.h b/drivers/net/tap/rte_eth_tap.h
> index 67c9d4b..8b0da5a 100644
> --- a/drivers/net/tap/rte_eth_tap.h
> +++ b/drivers/net/tap/rte_eth_tap.h
> @@ -23,6 +23,13 @@
> #define RTE_PMD_TAP_MAX_QUEUES	1
> #endif
> 
> +enum rte_tuntap_type {
> +	ETH_TUNTAP_TYPE_UNKNOWN,
> +	ETH_TUNTAP_TYPE_TUN,
> +	ETH_TUNTAP_TYPE_TAP,
> +	ETH_TUNTAP_TYPE_MAX,
> +};
> +
> struct pkt_stats {
> 	uint64_t opackets;              /* Number of output packets */
> 	uint64_t ipackets;              /* Number of input packets */
> @@ -38,6 +45,7 @@ struct rx_queue {
> 	uint32_t trigger_seen;          /* Last seen Rx trigger value */
> 	uint16_t in_port;               /* Port ID */
> 	int fd;
> +	int type;			  /* Type field - TUN|TAP */
> 	struct pkt_stats stats;         /* Stats for this RX queue */
> 	uint16_t nb_rx_desc;            /* max number of mbufs available */
> 	struct rte_eth_rxmode *rxmode;  /* RX features */
> @@ -48,6 +56,7 @@ struct rx_queue {
> 
> struct tx_queue {
> 	int fd;
> +	int type;			  /* Type field - TUN|TAP */
> 	uint16_t *mtu;                  /* Pointer to MTU from dev_data */
> 	uint16_t csum:1;                /* Enable checksum offloading */
> 	struct pkt_stats stats;         /* Stats for this TX queue */
> @@ -57,6 +66,7 @@ struct pmd_internals {
> 	struct rte_eth_dev *dev;          /* Ethernet device. */
> 	char remote_iface[RTE_ETH_NAME_MAX_LEN]; /* Remote netdevice name */
> 	char name[RTE_ETH_NAME_MAX_LEN];  /* Internal Tap device name */
> +	int type;			  /* Type field - TUN|TAP */
> 	struct ether_addr eth_addr;       /* Mac address of the device port */
> 	struct ifreq remote_initial_flags;   /* Remote netdevice flags on init */
> 	int remote_if_index;              /* remote netdevice IF_INDEX */
> -- 
> 2.7.4
> 

Regards,
Keith

Re: [dpdk-dev] [PATCH] net/tap: perform proto field update for tun only

[Varghese, Vipin]

Thanks Keith,

I have made changes and shared v2 patch for both the suggestions.

Thanks
Vipin Varghese

<snipped>

> > +			/*
> > +			 * TUN and TAP are created with IFF_NO_PI disabled.
> > +			 * For TUN PMD this mandatory as fields are used by
> > +			 * Kernel tun.c to determine whether its IP or non IP
> > +			 * packets.
> > +			 *
> > +			 * The logic fetches the first byte of data from mbuf.
> > +			 * compares whether its v4 or v6. If none match default
> > +			 * value 0x00 is taken for protocol field.
> 
> Little reword and remove the ‘.’ at end of first line.
> 
> The logic fetches the first byte of data from mbuf then compares whether it is v4
> or v6. If not equal to zero then it must be a protocol field.
> 
> 
> 
> > +			 */
> > +			char *buff_data = rte_pktmbuf_mtod(seg, void *);
> > +			j = (*buff_data & 0xf0);
> > +			pi.proto = (j == 0x40) ? 0x0008 :
> > +					(j == 0x60) ? 0xdd86 : 0x00;
> > +			printf("j %x pi proto %x\n", j, pi.proto);
> 
> Should this not be a LOG message or is this debug that was not removed? If log
> message then add move text to explain the output better.
> 

<snipped>

Re: [dpdk-dev] [PATCH v2] net/tap: perform proto field update for tun only

[Wiles, Keith]

> On May 15, 2018, at 9:49 AM, Varghese, Vipin <vipin.varghese@intel.com> wrote:
> 
> The TX function is shared between TAP and TUN PMD. Checking TUN-TAP
> type field will ensure the TAP PMD will always have protocol field
> as 0.
> 
> Signed-off-by: Vipin Varghese <vipin.varghese@intel.com>
> Suggested-by: Ferruh Yigit <ferruh.yigit@intel.com>
> ---
> 
> Changes in V2:
> updated in comment wording - Keith Wiles
> remove debug print from tx code - Keith Wiles


Acked by: Keith Wiles<keith.wiles@intel.com>

Regards,
Keith

[dpdk-dev] [PATCH v2] net/tap: perform proto field update for tun only

[Vipin Varghese]

The TX function is shared between TAP and TUN PMD. Checking TUN-TAP
type field will ensure the TAP PMD will always have protocol field
as 0.

Signed-off-by: Vipin Varghese <vipin.varghese@intel.com>
Suggested-by: Ferruh Yigit <ferruh.yigit@intel.com>
---

Changes in V2:
updated in comment wording - Keith Wiles
remove debug print from tx code - Keith Wiles
---
 drivers/net/tap/rte_eth_tap.c | 48 ++++++++++++++++++++++++++-----------------
 drivers/net/tap/rte_eth_tap.h | 10 +++++++++
 2 files changed, 39 insertions(+), 19 deletions(-)

diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
index ea6d899..fafd557 100644
--- a/drivers/net/tap/rte_eth_tap.c
+++ b/drivers/net/tap/rte_eth_tap.c
@@ -115,7 +115,8 @@ tun_alloc(struct pmd_internals *pmd)
 	 * Do not set IFF_NO_PI as packet information header will be needed
 	 * to check if a received packet has been truncated.
 	 */
-	ifr.ifr_flags = (tap_type) ? IFF_TAP : IFF_TUN | IFF_POINTOPOINT;
+	ifr.ifr_flags = (pmd->type == ETH_TUNTAP_TYPE_TAP) ?
+			IFF_TAP : IFF_TUN | IFF_POINTOPOINT;
 	snprintf(ifr.ifr_name, IFNAMSIZ, "%s", pmd->name);
 
 	TAP_LOG(DEBUG, "ifr_name '%s'", ifr.ifr_name);
@@ -502,20 +503,22 @@ pmd_tx_burst(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
 		if (rte_pktmbuf_pkt_len(mbuf) > max_size)
 			break;
 
-		/*
-		 * TUN and TAP are created with IFF_NO_PI disabled.
-		 * For TUN PMD this mandatory as fields are used by
-		 * Kernel tun.c to determine whether its IP or non IP
-		 * packets.
-		 *
-		 * The logic fetches the first byte of data from mbuf.
-		 * compares whether its v4 or v6. If none matches default
-		 * value 0x00 is taken for protocol field.
-		 */
-		char *buff_data = rte_pktmbuf_mtod(seg, void *);
-		j = (*buff_data & 0xf0);
-		pi.proto = (j == 0x40) ? 0x0008 :
-				(j == 0x60) ? 0xdd86 : 0x00;
+		if (txq->type == ETH_TUNTAP_TYPE_TUN) {
+			/*
+			 * TUN and TAP are created with IFF_NO_PI disabled.
+			 * For TUN PMD this mandatory as fields are used by
+			 * Kernel tun.c to determine whether its IP or non IP
+			 * packets.
+			 *
+			 * The logic fetches the first byte of data from mbuf
+			 * then compares whether its v4 or v6. If first byte
+			 * is 4 or 6, then protocol field is updated.
+			 */
+			char *buff_data = rte_pktmbuf_mtod(seg, void *);
+			j = (*buff_data & 0xf0);
+			pi.proto = (j == 0x40) ? 0x0008 :
+					(j == 0x60) ? 0xdd86 : 0x00;
+		}
 
 		iovecs[0].iov_base = &pi;
 		iovecs[0].iov_len = sizeof(pi);
@@ -1052,6 +1055,9 @@ tap_setup_queue(struct rte_eth_dev *dev,
 	tx->mtu = &dev->data->mtu;
 	rx->rxmode = &dev->data->dev_conf.rxmode;
 
+	tx->type = pmd->type;
+	rx->type = pmd->type;
+
 	return *fd;
 }
 
@@ -1386,6 +1392,7 @@ eth_dev_tap_create(struct rte_vdev_device *vdev, char *tap_name,
 	pmd = dev->data->dev_private;
 	pmd->dev = dev;
 	snprintf(pmd->name, sizeof(pmd->name), "%s", tap_name);
+	pmd->type = ETH_TUNTAP_TYPE_UNKNOWN;
 
 	pmd->ioctl_sock = socket(AF_INET, SOCK_DGRAM, 0);
 	if (pmd->ioctl_sock == -1) {
@@ -1421,7 +1428,9 @@ eth_dev_tap_create(struct rte_vdev_device *vdev, char *tap_name,
 		pmd->txq[i].fd = -1;
 	}
 
-	if (tap_type) {
+	pmd->type = (tap_type) ? ETH_TUNTAP_TYPE_TAP : ETH_TUNTAP_TYPE_TUN;
+
+	if (pmd->type == ETH_TUNTAP_TYPE_TAP) {
 		if (is_zero_ether_addr(mac_addr))
 			eth_random_addr((uint8_t *)&pmd->eth_addr);
 		else
@@ -1440,7 +1449,7 @@ eth_dev_tap_create(struct rte_vdev_device *vdev, char *tap_name,
 	if (tap_ioctl(pmd, SIOCSIFMTU, &ifr, 1, LOCAL_AND_REMOTE) < 0)
 		goto error_exit;
 
-	if (tap_type) {
+	if (pmd->type == ETH_TUNTAP_TYPE_TAP) {
 		memset(&ifr, 0, sizeof(struct ifreq));
 		ifr.ifr_hwaddr.sa_family = AF_LOCAL;
 		rte_memcpy(ifr.ifr_hwaddr.sa_data, &pmd->eth_addr,
@@ -1812,7 +1821,9 @@ rte_pmd_tap_remove(struct rte_vdev_device *dev)
 	struct pmd_internals *internals;
 	int i;
 
-	TAP_LOG(DEBUG, "Closing TUN/TAP Ethernet device on numa %u",
+	internals = eth_dev->data->dev_private;
+	TAP_LOG(DEBUG, "Closing %s Ethernet device on numa %u",
+		(internals->type == ETH_TUNTAP_TYPE_TAP) ? "TAP" : "TUN",
 		rte_socket_id());
 
 	/* find the ethdev entry */
@@ -1820,7 +1831,6 @@ rte_pmd_tap_remove(struct rte_vdev_device *dev)
 	if (!eth_dev)
 		return 0;
 
-	internals = eth_dev->data->dev_private;
 	if (internals->nlsk_fd) {
 		tap_flow_flush(eth_dev, NULL);
 		tap_flow_implicit_flush(internals, NULL);
diff --git a/drivers/net/tap/rte_eth_tap.h b/drivers/net/tap/rte_eth_tap.h
index 67c9d4b..8b0da5a 100644
--- a/drivers/net/tap/rte_eth_tap.h
+++ b/drivers/net/tap/rte_eth_tap.h
@@ -23,6 +23,13 @@
 #define RTE_PMD_TAP_MAX_QUEUES	1
 #endif
 
+enum rte_tuntap_type {
+	ETH_TUNTAP_TYPE_UNKNOWN,
+	ETH_TUNTAP_TYPE_TUN,
+	ETH_TUNTAP_TYPE_TAP,
+	ETH_TUNTAP_TYPE_MAX,
+};
+
 struct pkt_stats {
 	uint64_t opackets;              /* Number of output packets */
 	uint64_t ipackets;              /* Number of input packets */
@@ -38,6 +45,7 @@ struct rx_queue {
 	uint32_t trigger_seen;          /* Last seen Rx trigger value */
 	uint16_t in_port;               /* Port ID */
 	int fd;
+	int type;			  /* Type field - TUN|TAP */
 	struct pkt_stats stats;         /* Stats for this RX queue */
 	uint16_t nb_rx_desc;            /* max number of mbufs available */
 	struct rte_eth_rxmode *rxmode;  /* RX features */
@@ -48,6 +56,7 @@ struct rx_queue {
 
 struct tx_queue {
 	int fd;
+	int type;			  /* Type field - TUN|TAP */
 	uint16_t *mtu;                  /* Pointer to MTU from dev_data */
 	uint16_t csum:1;                /* Enable checksum offloading */
 	struct pkt_stats stats;         /* Stats for this TX queue */
@@ -57,6 +66,7 @@ struct pmd_internals {
 	struct rte_eth_dev *dev;          /* Ethernet device. */
 	char remote_iface[RTE_ETH_NAME_MAX_LEN]; /* Remote netdevice name */
 	char name[RTE_ETH_NAME_MAX_LEN];  /* Internal Tap device name */
+	int type;			  /* Type field - TUN|TAP */
 	struct ether_addr eth_addr;       /* Mac address of the device port */
 	struct ifreq remote_initial_flags;   /* Remote netdevice flags on init */
 	int remote_if_index;              /* remote netdevice IF_INDEX */
-- 
2.7.4

Re: [dpdk-dev] [PATCH v2] net/tap: perform proto field update for tun only

[Ophir Munk]

Hi,
I am adding my review although this patch has already been acked.
Please find some comments inline. Otherwise it looks OK.

Regards,
Ophir

> -----Original Message-----
> From: Vipin Varghese [mailto:vipin.varghese@intel.com]
> Sent: Tuesday, May 15, 2018 5:50 PM
> To: dev@dpdk.org; keith.wiles@intel.com; ferruh.yigit@intel.com; Ophir
> Munk <ophirmu@mellanox.com>
> Cc: Vipin Varghese <vipin.varghese@intel.com>
> Subject: [PATCH v2] net/tap: perform proto field update for tun only
> 
> The TX function is shared between TAP and TUN PMD. Checking TUN-TAP
> type field will ensure the TAP PMD will always have protocol field as 0.
> 
> Signed-off-by: Vipin Varghese <vipin.varghese@intel.com>
> Suggested-by: Ferruh Yigit <ferruh.yigit@intel.com>
> ---
> 
> Changes in V2:
> updated in comment wording - Keith Wiles remove debug print from tx code
> - Keith Wiles
> ---
>  drivers/net/tap/rte_eth_tap.c | 48 ++++++++++++++++++++++++++-----------
> ------
>  drivers/net/tap/rte_eth_tap.h | 10 +++++++++
>  2 files changed, 39 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
> index ea6d899..fafd557 100644
> --- a/drivers/net/tap/rte_eth_tap.c
> +++ b/drivers/net/tap/rte_eth_tap.c
> @@ -115,7 +115,8 @@ tun_alloc(struct pmd_internals *pmd)
>  	 * Do not set IFF_NO_PI as packet information header will be needed
>  	 * to check if a received packet has been truncated.
>  	 */
> -	ifr.ifr_flags = (tap_type) ? IFF_TAP : IFF_TUN | IFF_POINTOPOINT;
> +	ifr.ifr_flags = (pmd->type == ETH_TUNTAP_TYPE_TAP) ?
> +			IFF_TAP : IFF_TUN | IFF_POINTOPOINT;
>  	snprintf(ifr.ifr_name, IFNAMSIZ, "%s", pmd->name);
> 
>  	TAP_LOG(DEBUG, "ifr_name '%s'", ifr.ifr_name); @@ -502,20
> +503,22 @@ pmd_tx_burst(void *queue, struct rte_mbuf **bufs, uint16_t
> nb_pkts)
>  		if (rte_pktmbuf_pkt_len(mbuf) > max_size)
>  			break;
> 
> -		/*
> -		 * TUN and TAP are created with IFF_NO_PI disabled.
> -		 * For TUN PMD this mandatory as fields are used by
> -		 * Kernel tun.c to determine whether its IP or non IP
> -		 * packets.
> -		 *
> -		 * The logic fetches the first byte of data from mbuf.
> -		 * compares whether its v4 or v6. If none matches default
> -		 * value 0x00 is taken for protocol field.
> -		 */
> -		char *buff_data = rte_pktmbuf_mtod(seg, void *);
> -		j = (*buff_data & 0xf0);
> -		pi.proto = (j == 0x40) ? 0x0008 :
> -				(j == 0x60) ? 0xdd86 : 0x00;
> +		if (txq->type == ETH_TUNTAP_TYPE_TUN) {
> +			/*
> +			 * TUN and TAP are created with IFF_NO_PI disabled.
> +			 * For TUN PMD this mandatory as fields are used by
> +			 * Kernel tun.c to determine whether its IP or non IP
> +			 * packets.
> +			 *
> +			 * The logic fetches the first byte of data from mbuf
> +			 * then compares whether its v4 or v6. If first byte
> +			 * is 4 or 6, then protocol field is updated.
> +			 */
> +			char *buff_data = rte_pktmbuf_mtod(seg, void *);
> +			j = (*buff_data & 0xf0);
> +			pi.proto = (j == 0x40) ? 0x0008 :
> +					(j == 0x60) ? 0xdd86 : 0x00;
> +		}
> 
>  		iovecs[0].iov_base = &pi;
>  		iovecs[0].iov_len = sizeof(pi);
> @@ -1052,6 +1055,9 @@ tap_setup_queue(struct rte_eth_dev *dev,
>  	tx->mtu = &dev->data->mtu;
>  	rx->rxmode = &dev->data->dev_conf.rxmode;
> 
> +	tx->type = pmd->type;
> +	rx->type = pmd->type;

It seems there is no usage for rx->type in TAP code. Therefore - can you please remove rx->type assignment?
> +
>  	return *fd;
>  }
> 
> @@ -1386,6 +1392,7 @@ eth_dev_tap_create(struct rte_vdev_device
> *vdev, char *tap_name,
>  	pmd = dev->data->dev_private;
>  	pmd->dev = dev;
>  	snprintf(pmd->name, sizeof(pmd->name), "%s", tap_name);
> +	pmd->type = ETH_TUNTAP_TYPE_UNKNOWN;
> 
>  	pmd->ioctl_sock = socket(AF_INET, SOCK_DGRAM, 0);
>  	if (pmd->ioctl_sock == -1) {
> @@ -1421,7 +1428,9 @@ eth_dev_tap_create(struct rte_vdev_device
> *vdev, char *tap_name,
>  		pmd->txq[i].fd = -1;
>  	}
> 
> -	if (tap_type) {
> +	pmd->type = (tap_type) ? ETH_TUNTAP_TYPE_TAP :
> ETH_TUNTAP_TYPE_TUN;

1. It seem you are using 'tap_type' as a global static variable to pass a value to eth_dev_tap_create().
Can you please pass this value by adding a parameter to eth_dev_tap_create() call and remove the global static variable?
2. You are using tap_type with values 0 and 1 which are converted to enums ETH_TUNTAP_TYPE_TUN and ETH_TUNTAP_TYPE_TAP respectively. Can you please always just use the enums (avoiding the conversion)?

> +
> +	if (pmd->type == ETH_TUNTAP_TYPE_TAP) {
>  		if (is_zero_ether_addr(mac_addr))
>  			eth_random_addr((uint8_t *)&pmd->eth_addr);
>  		else
> @@ -1440,7 +1449,7 @@ eth_dev_tap_create(struct rte_vdev_device
> *vdev, char *tap_name,
>  	if (tap_ioctl(pmd, SIOCSIFMTU, &ifr, 1, LOCAL_AND_REMOTE) < 0)
>  		goto error_exit;
> 
> -	if (tap_type) {
> +	if (pmd->type == ETH_TUNTAP_TYPE_TAP) {
>  		memset(&ifr, 0, sizeof(struct ifreq));
>  		ifr.ifr_hwaddr.sa_family = AF_LOCAL;
>  		rte_memcpy(ifr.ifr_hwaddr.sa_data, &pmd->eth_addr, @@ -
> 1812,7 +1821,9 @@ rte_pmd_tap_remove(struct rte_vdev_device *dev)
>  	struct pmd_internals *internals;
>  	int i;
> 
> -	TAP_LOG(DEBUG, "Closing TUN/TAP Ethernet device on numa %u",
> +	internals = eth_dev->data->dev_private;
> +	TAP_LOG(DEBUG, "Closing %s Ethernet device on numa %u",
> +		(internals->type == ETH_TUNTAP_TYPE_TAP) ? "TAP" :
> "TUN",
>  		rte_socket_id());
> 
>  	/* find the ethdev entry */
> @@ -1820,7 +1831,6 @@ rte_pmd_tap_remove(struct rte_vdev_device
> *dev)
>  	if (!eth_dev)
>  		return 0;
> 
> -	internals = eth_dev->data->dev_private;
>  	if (internals->nlsk_fd) {
>  		tap_flow_flush(eth_dev, NULL);
>  		tap_flow_implicit_flush(internals, NULL); diff --git
> a/drivers/net/tap/rte_eth_tap.h b/drivers/net/tap/rte_eth_tap.h index
> 67c9d4b..8b0da5a 100644
> --- a/drivers/net/tap/rte_eth_tap.h
> +++ b/drivers/net/tap/rte_eth_tap.h
> @@ -23,6 +23,13 @@
>  #define RTE_PMD_TAP_MAX_QUEUES	1
>  #endif
> 
> +enum rte_tuntap_type {
> +	ETH_TUNTAP_TYPE_UNKNOWN,
> +	ETH_TUNTAP_TYPE_TUN,
> +	ETH_TUNTAP_TYPE_TAP,
> +	ETH_TUNTAP_TYPE_MAX,
> +};
> +
>  struct pkt_stats {
>  	uint64_t opackets;              /* Number of output packets */
>  	uint64_t ipackets;              /* Number of input packets */
> @@ -38,6 +45,7 @@ struct rx_queue {
>  	uint32_t trigger_seen;          /* Last seen Rx trigger value */
>  	uint16_t in_port;               /* Port ID */
>  	int fd;
> +	int type;			  /* Type field - TUN|TAP */

In seems there is no usage for 'type' field from rx_queue struct in the code. If so - can you please remove it?

>  	struct pkt_stats stats;         /* Stats for this RX queue */
>  	uint16_t nb_rx_desc;            /* max number of mbufs available */
>  	struct rte_eth_rxmode *rxmode;  /* RX features */ @@ -48,6 +56,7
> @@ struct rx_queue {
> 
>  struct tx_queue {
>  	int fd;
> +	int type;			  /* Type field - TUN|TAP */

Please use spaces (like all other fields in this struct) instead of tabs, so all fields look aligned.

>  	uint16_t *mtu;                  /* Pointer to MTU from dev_data */
>  	uint16_t csum:1;                /* Enable checksum offloading */
>  	struct pkt_stats stats;         /* Stats for this TX queue */
> @@ -57,6 +66,7 @@ struct pmd_internals {
>  	struct rte_eth_dev *dev;          /* Ethernet device. */
>  	char remote_iface[RTE_ETH_NAME_MAX_LEN]; /* Remote
> netdevice name */
>  	char name[RTE_ETH_NAME_MAX_LEN];  /* Internal Tap device
> name */
> +	int type;			  /* Type field - TUN|TAP */

Same comment here about replacing tabs with spaces

>  	struct ether_addr eth_addr;       /* Mac address of the device port */
>  	struct ifreq remote_initial_flags;   /* Remote netdevice flags on init
> */
>  	int remote_if_index;              /* remote netdevice IF_INDEX */
> --
> 2.7.4

Re: [dpdk-dev] [PATCH v2] net/tap: perform proto field update for tun only

[Varghese, Vipin]

Hi Ophir,

Thanks for the suggestion, once these are merged to dpdk or dpdk-net mainline, I can start the modifications.

Thanks
Vipin Varghese

> -----Original Message-----
> From: Ophir Munk [mailto:ophirmu@mellanox.com]
> Sent: Wednesday, May 16, 2018 3:59 AM
> To: Varghese, Vipin <vipin.varghese@intel.com>; dev@dpdk.org; Wiles, Keith
> <keith.wiles@intel.com>; Yigit, Ferruh <ferruh.yigit@intel.com>
> Cc: Thomas Monjalon <thomas@monjalon.net>; Olga Shern
> <olgas@mellanox.com>
> Subject: RE: [PATCH v2] net/tap: perform proto field update for tun only
> 
> Hi,
> I am adding my review although this patch has already been acked.
> Please find some comments inline. Otherwise it looks OK.
> 
> Regards,
> Ophir
> 
> > -----Original Message-----
> > From: Vipin Varghese [mailto:vipin.varghese@intel.com]
> > Sent: Tuesday, May 15, 2018 5:50 PM
> > To: dev@dpdk.org; keith.wiles@intel.com; ferruh.yigit@intel.com; Ophir
> > Munk <ophirmu@mellanox.com>
> > Cc: Vipin Varghese <vipin.varghese@intel.com>
> > Subject: [PATCH v2] net/tap: perform proto field update for tun only
> >
> > The TX function is shared between TAP and TUN PMD. Checking TUN-TAP
> > type field will ensure the TAP PMD will always have protocol field as 0.
> >
> > Signed-off-by: Vipin Varghese <vipin.varghese@intel.com>
> > Suggested-by: Ferruh Yigit <ferruh.yigit@intel.com>
> > ---
> >
> > Changes in V2:
> > updated in comment wording - Keith Wiles remove debug print from tx
> > code
> > - Keith Wiles
> > ---
> >  drivers/net/tap/rte_eth_tap.c | 48
> > ++++++++++++++++++++++++++-----------
> > ------
> >  drivers/net/tap/rte_eth_tap.h | 10 +++++++++
> >  2 files changed, 39 insertions(+), 19 deletions(-)
> >
> > diff --git a/drivers/net/tap/rte_eth_tap.c
> > b/drivers/net/tap/rte_eth_tap.c index ea6d899..fafd557 100644
> > --- a/drivers/net/tap/rte_eth_tap.c
> > +++ b/drivers/net/tap/rte_eth_tap.c
> > @@ -115,7 +115,8 @@ tun_alloc(struct pmd_internals *pmd)
> >  	 * Do not set IFF_NO_PI as packet information header will be needed
> >  	 * to check if a received packet has been truncated.
> >  	 */
> > -	ifr.ifr_flags = (tap_type) ? IFF_TAP : IFF_TUN | IFF_POINTOPOINT;
> > +	ifr.ifr_flags = (pmd->type == ETH_TUNTAP_TYPE_TAP) ?
> > +			IFF_TAP : IFF_TUN | IFF_POINTOPOINT;
> >  	snprintf(ifr.ifr_name, IFNAMSIZ, "%s", pmd->name);
> >
> >  	TAP_LOG(DEBUG, "ifr_name '%s'", ifr.ifr_name); @@ -502,20
> > +503,22 @@ pmd_tx_burst(void *queue, struct rte_mbuf **bufs, uint16_t
> > nb_pkts)
> >  		if (rte_pktmbuf_pkt_len(mbuf) > max_size)
> >  			break;
> >
> > -		/*
> > -		 * TUN and TAP are created with IFF_NO_PI disabled.
> > -		 * For TUN PMD this mandatory as fields are used by
> > -		 * Kernel tun.c to determine whether its IP or non IP
> > -		 * packets.
> > -		 *
> > -		 * The logic fetches the first byte of data from mbuf.
> > -		 * compares whether its v4 or v6. If none matches default
> > -		 * value 0x00 is taken for protocol field.
> > -		 */
> > -		char *buff_data = rte_pktmbuf_mtod(seg, void *);
> > -		j = (*buff_data & 0xf0);
> > -		pi.proto = (j == 0x40) ? 0x0008 :
> > -				(j == 0x60) ? 0xdd86 : 0x00;
> > +		if (txq->type == ETH_TUNTAP_TYPE_TUN) {
> > +			/*
> > +			 * TUN and TAP are created with IFF_NO_PI disabled.
> > +			 * For TUN PMD this mandatory as fields are used by
> > +			 * Kernel tun.c to determine whether its IP or non IP
> > +			 * packets.
> > +			 *
> > +			 * The logic fetches the first byte of data from mbuf
> > +			 * then compares whether its v4 or v6. If first byte
> > +			 * is 4 or 6, then protocol field is updated.
> > +			 */
> > +			char *buff_data = rte_pktmbuf_mtod(seg, void *);
> > +			j = (*buff_data & 0xf0);
> > +			pi.proto = (j == 0x40) ? 0x0008 :
> > +					(j == 0x60) ? 0xdd86 : 0x00;
> > +		}
> >
> >  		iovecs[0].iov_base = &pi;
> >  		iovecs[0].iov_len = sizeof(pi);
> > @@ -1052,6 +1055,9 @@ tap_setup_queue(struct rte_eth_dev *dev,
> >  	tx->mtu = &dev->data->mtu;
> >  	rx->rxmode = &dev->data->dev_conf.rxmode;
> >
> > +	tx->type = pmd->type;
> > +	rx->type = pmd->type;
> 
> It seems there is no usage for rx->type in TAP code. Therefore - can you please
> remove rx->type assignment?
> > +
> >  	return *fd;
> >  }
> >
> > @@ -1386,6 +1392,7 @@ eth_dev_tap_create(struct rte_vdev_device *vdev,
> > char *tap_name,
> >  	pmd = dev->data->dev_private;
> >  	pmd->dev = dev;
> >  	snprintf(pmd->name, sizeof(pmd->name), "%s", tap_name);
> > +	pmd->type = ETH_TUNTAP_TYPE_UNKNOWN;
> >
> >  	pmd->ioctl_sock = socket(AF_INET, SOCK_DGRAM, 0);
> >  	if (pmd->ioctl_sock == -1) {
> > @@ -1421,7 +1428,9 @@ eth_dev_tap_create(struct rte_vdev_device *vdev,
> > char *tap_name,
> >  		pmd->txq[i].fd = -1;
> >  	}
> >
> > -	if (tap_type) {
> > +	pmd->type = (tap_type) ? ETH_TUNTAP_TYPE_TAP :
> > ETH_TUNTAP_TYPE_TUN;
> 
> 1. It seem you are using 'tap_type' as a global static variable to pass a value to
> eth_dev_tap_create().
> Can you please pass this value by adding a parameter to eth_dev_tap_create()
> call and remove the global static variable?
> 2. You are using tap_type with values 0 and 1 which are converted to enums
> ETH_TUNTAP_TYPE_TUN and ETH_TUNTAP_TYPE_TAP respectively. Can you
> please always just use the enums (avoiding the conversion)?
> 
> > +
> > +	if (pmd->type == ETH_TUNTAP_TYPE_TAP) {
> >  		if (is_zero_ether_addr(mac_addr))
> >  			eth_random_addr((uint8_t *)&pmd->eth_addr);
> >  		else
> > @@ -1440,7 +1449,7 @@ eth_dev_tap_create(struct rte_vdev_device *vdev,
> > char *tap_name,
> >  	if (tap_ioctl(pmd, SIOCSIFMTU, &ifr, 1, LOCAL_AND_REMOTE) < 0)
> >  		goto error_exit;
> >
> > -	if (tap_type) {
> > +	if (pmd->type == ETH_TUNTAP_TYPE_TAP) {
> >  		memset(&ifr, 0, sizeof(struct ifreq));
> >  		ifr.ifr_hwaddr.sa_family = AF_LOCAL;
> >  		rte_memcpy(ifr.ifr_hwaddr.sa_data, &pmd->eth_addr, @@ -
> > 1812,7 +1821,9 @@ rte_pmd_tap_remove(struct rte_vdev_device *dev)
> >  	struct pmd_internals *internals;
> >  	int i;
> >
> > -	TAP_LOG(DEBUG, "Closing TUN/TAP Ethernet device on numa %u",
> > +	internals = eth_dev->data->dev_private;
> > +	TAP_LOG(DEBUG, "Closing %s Ethernet device on numa %u",
> > +		(internals->type == ETH_TUNTAP_TYPE_TAP) ? "TAP" :
> > "TUN",
> >  		rte_socket_id());
> >
> >  	/* find the ethdev entry */
> > @@ -1820,7 +1831,6 @@ rte_pmd_tap_remove(struct rte_vdev_device
> > *dev)
> >  	if (!eth_dev)
> >  		return 0;
> >
> > -	internals = eth_dev->data->dev_private;
> >  	if (internals->nlsk_fd) {
> >  		tap_flow_flush(eth_dev, NULL);
> >  		tap_flow_implicit_flush(internals, NULL); diff --git
> > a/drivers/net/tap/rte_eth_tap.h b/drivers/net/tap/rte_eth_tap.h index
> > 67c9d4b..8b0da5a 100644
> > --- a/drivers/net/tap/rte_eth_tap.h
> > +++ b/drivers/net/tap/rte_eth_tap.h
> > @@ -23,6 +23,13 @@
> >  #define RTE_PMD_TAP_MAX_QUEUES	1
> >  #endif
> >
> > +enum rte_tuntap_type {
> > +	ETH_TUNTAP_TYPE_UNKNOWN,
> > +	ETH_TUNTAP_TYPE_TUN,
> > +	ETH_TUNTAP_TYPE_TAP,
> > +	ETH_TUNTAP_TYPE_MAX,
> > +};
> > +
> >  struct pkt_stats {
> >  	uint64_t opackets;              /* Number of output packets */
> >  	uint64_t ipackets;              /* Number of input packets */
> > @@ -38,6 +45,7 @@ struct rx_queue {
> >  	uint32_t trigger_seen;          /* Last seen Rx trigger value */
> >  	uint16_t in_port;               /* Port ID */
> >  	int fd;
> > +	int type;			  /* Type field - TUN|TAP */
> 
> In seems there is no usage for 'type' field from rx_queue struct in the code. If so
> - can you please remove it?
> 
> >  	struct pkt_stats stats;         /* Stats for this RX queue */
> >  	uint16_t nb_rx_desc;            /* max number of mbufs available */
> >  	struct rte_eth_rxmode *rxmode;  /* RX features */ @@ -48,6 +56,7
> @@
> > struct rx_queue {
> >
> >  struct tx_queue {
> >  	int fd;
> > +	int type;			  /* Type field - TUN|TAP */
> 
> Please use spaces (like all other fields in this struct) instead of tabs, so all fields
> look aligned.
> 
> >  	uint16_t *mtu;                  /* Pointer to MTU from dev_data */
> >  	uint16_t csum:1;                /* Enable checksum offloading */
> >  	struct pkt_stats stats;         /* Stats for this TX queue */
> > @@ -57,6 +66,7 @@ struct pmd_internals {
> >  	struct rte_eth_dev *dev;          /* Ethernet device. */
> >  	char remote_iface[RTE_ETH_NAME_MAX_LEN]; /* Remote netdevice
> name */
> >  	char name[RTE_ETH_NAME_MAX_LEN];  /* Internal Tap device name */
> > +	int type;			  /* Type field - TUN|TAP */
> 
> Same comment here about replacing tabs with spaces
> 
> >  	struct ether_addr eth_addr;       /* Mac address of the device port */
> >  	struct ifreq remote_initial_flags;   /* Remote netdevice flags on init
> > */
> >  	int remote_if_index;              /* remote netdevice IF_INDEX */
> > --
> > 2.7.4

[dpdk-dev] [PATCH v3] net/tap: perform proto field update for tun only

[Vipin Varghese]

The TX function is shared between TAP and TUN PMD. Checking TUN-TAP
type field will ensure the TAP PMD will always have protocol field
as 0.

Signed-off-by: Vipin Varghese <vipin.varghese@intel.com>
Suggested-by: Ferruh Yigit <ferruh.yigit@intel.com>
---
Changes in V3:
- remove type field from rx struct - Ophir Munk
- add space for comment in struct - Ophir Munk
- pass type field into eth_dev_tap_create - Ophir Munk
- replace with enum value for type - Ophir Munk
- return as 'not supported' for mac_set - Vipin Varghese

Changes in V2:
- updated in comment wording - Keith Wiles
- remove debug print from tx code - Keith Wiles
---
 drivers/net/tap/rte_eth_tap.c | 61 +++++++++++++++++++++++++------------------
 drivers/net/tap/rte_eth_tap.h |  9 +++++++
 2 files changed, 45 insertions(+), 25 deletions(-)

diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
index 01552fa..8d8f67b 100644
--- a/drivers/net/tap/rte_eth_tap.c
+++ b/drivers/net/tap/rte_eth_tap.c
@@ -68,7 +68,6 @@ static const char *valid_arguments[] = {
 static int tap_unit;
 static unsigned int tun_unit;
 
-static int tap_type;
 static char tuntap_name[8];
 
 static volatile uint32_t tap_trigger;	/* Rx trigger */
@@ -116,7 +115,8 @@ tun_alloc(struct pmd_internals *pmd)
 	 * Do not set IFF_NO_PI as packet information header will be needed
 	 * to check if a received packet has been truncated.
 	 */
-	ifr.ifr_flags = (tap_type) ? IFF_TAP : IFF_TUN | IFF_POINTOPOINT;
+	ifr.ifr_flags = (pmd->type == ETH_TUNTAP_TYPE_TAP) ?
+		IFF_TAP : IFF_TUN | IFF_POINTOPOINT;
 	snprintf(ifr.ifr_name, IFNAMSIZ, "%s", pmd->name);
 
 	TAP_LOG(DEBUG, "ifr_name '%s'", ifr.ifr_name);
@@ -472,20 +472,22 @@ pmd_tx_burst(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
 		if (rte_pktmbuf_pkt_len(mbuf) > max_size)
 			break;
 
-		/*
-		 * TUN and TAP are created with IFF_NO_PI disabled.
-		 * For TUN PMD this mandatory as fields are used by
-		 * Kernel tun.c to determine whether its IP or non IP
-		 * packets.
-		 *
-		 * The logic fetches the first byte of data from mbuf.
-		 * compares whether its v4 or v6. If none matches default
-		 * value 0x00 is taken for protocol field.
-		 */
-		char *buff_data = rte_pktmbuf_mtod(seg, void *);
-		j = (*buff_data & 0xf0);
-		pi.proto = (j == 0x40) ? 0x0008 :
+		if (txq->type == ETH_TUNTAP_TYPE_TUN) {
+			/*
+			 * TUN and TAP are created with IFF_NO_PI disabled.
+			 * For TUN PMD this mandatory as fields are used by
+			 * Kernel tun.c to determine whether its IP or non IP
+			 * packets.
+			 *
+			 * The logic fetches the first byte of data from mbuf
+			 * then compares whether its v4 or v6. If first byte
+			 * is 4 or 6, then protocol field is updated.
+			 */
+			char *buff_data = rte_pktmbuf_mtod(seg, void *);
+			j = (*buff_data & 0xf0);
+			pi.proto = (j == 0x40) ? 0x0008 :
 				(j == 0x60) ? 0xdd86 : 0x00;
+		}
 
 		iovecs[0].iov_base = &pi;
 		iovecs[0].iov_len = sizeof(pi);
@@ -928,6 +930,9 @@ tap_mac_set(struct rte_eth_dev *dev, struct ether_addr *mac_addr)
 	struct ifreq ifr;
 	int ret;
 
+	if (pmd->type == ETH_TUNTAP_TYPE_TUN)
+		return -ENOTSUP;
+
 	if (is_zero_ether_addr(mac_addr)) {
 		TAP_LOG(ERR, "%s: can't set an empty MAC address",
 			dev->device->name);
@@ -1025,6 +1030,8 @@ tap_setup_queue(struct rte_eth_dev *dev,
 	tx->mtu = &dev->data->mtu;
 	rx->rxmode = &dev->data->dev_conf.rxmode;
 
+	tx->type = pmd->type;
+
 	return *fd;
 }
 
@@ -1342,7 +1349,8 @@ static const struct eth_dev_ops ops = {
 
 static int
 eth_dev_tap_create(struct rte_vdev_device *vdev, char *tap_name,
-		   char *remote_iface, struct ether_addr *mac_addr)
+		   char *remote_iface, struct ether_addr *mac_addr,
+		   enum rte_tuntap_type type)
 {
 	int numa_node = rte_socket_id();
 	struct rte_eth_dev *dev;
@@ -1364,6 +1372,7 @@ eth_dev_tap_create(struct rte_vdev_device *vdev, char *tap_name,
 	pmd = dev->data->dev_private;
 	pmd->dev = dev;
 	snprintf(pmd->name, sizeof(pmd->name), "%s", tap_name);
+	pmd->type = type;
 
 	pmd->ioctl_sock = socket(AF_INET, SOCK_DGRAM, 0);
 	if (pmd->ioctl_sock == -1) {
@@ -1400,7 +1409,7 @@ eth_dev_tap_create(struct rte_vdev_device *vdev, char *tap_name,
 		pmd->txq[i].fd = -1;
 	}
 
-	if (tap_type) {
+	if (pmd->type == ETH_TUNTAP_TYPE_TAP) {
 		if (is_zero_ether_addr(mac_addr))
 			eth_random_addr((uint8_t *)&pmd->eth_addr);
 		else
@@ -1423,7 +1432,7 @@ eth_dev_tap_create(struct rte_vdev_device *vdev, char *tap_name,
 	if (tap_ioctl(pmd, SIOCSIFMTU, &ifr, 1, LOCAL_AND_REMOTE) < 0)
 		goto error_exit;
 
-	if (tap_type) {
+	if (pmd->type == ETH_TUNTAP_TYPE_TAP) {
 		memset(&ifr, 0, sizeof(struct ifreq));
 		ifr.ifr_hwaddr.sa_family = AF_LOCAL;
 		rte_memcpy(ifr.ifr_hwaddr.sa_data, &pmd->eth_addr,
@@ -1642,7 +1651,6 @@ rte_pmd_tun_probe(struct rte_vdev_device *dev)
 	char tun_name[RTE_ETH_NAME_MAX_LEN];
 	char remote_iface[RTE_ETH_NAME_MAX_LEN];
 
-	tap_type = 0;
 	strcpy(tuntap_name, "TUN");
 
 	name = rte_vdev_device_name(dev);
@@ -1673,7 +1681,8 @@ rte_pmd_tun_probe(struct rte_vdev_device *dev)
 	TAP_LOG(NOTICE, "Initializing pmd_tun for %s as %s",
 		name, tun_name);
 
-	ret = eth_dev_tap_create(dev, tun_name, remote_iface, 0);
+	ret = eth_dev_tap_create(dev, tun_name, remote_iface, 0,
+		ETH_TUNTAP_TYPE_TUN);
 
 leave:
 	if (ret == -1) {
@@ -1700,7 +1709,6 @@ rte_pmd_tap_probe(struct rte_vdev_device *dev)
 	struct ether_addr user_mac = { .addr_bytes = {0} };
 	struct rte_eth_dev *eth_dev;
 
-	tap_type = 1;
 	strcpy(tuntap_name, "TAP");
 
 	name = rte_vdev_device_name(dev);
@@ -1762,7 +1770,8 @@ rte_pmd_tap_probe(struct rte_vdev_device *dev)
 	TAP_LOG(NOTICE, "Initializing pmd_tap for %s as %s",
 		name, tap_name);
 
-	ret = eth_dev_tap_create(dev, tap_name, remote_iface, &user_mac);
+	ret = eth_dev_tap_create(dev, tap_name, remote_iface, &user_mac,
+		ETH_TUNTAP_TYPE_TAP);
 
 leave:
 	if (ret == -1) {
@@ -1784,15 +1793,17 @@ rte_pmd_tap_remove(struct rte_vdev_device *dev)
 	struct pmd_internals *internals;
 	int i;
 
-	TAP_LOG(DEBUG, "Closing TUN/TAP Ethernet device on numa %u",
-		rte_socket_id());
-
 	/* find the ethdev entry */
 	eth_dev = rte_eth_dev_allocated(rte_vdev_device_name(dev));
 	if (!eth_dev)
 		return 0;
 
 	internals = eth_dev->data->dev_private;
+
+	TAP_LOG(DEBUG, "Closing %s Ethernet device on numa %u",
+		(internals->type == ETH_TUNTAP_TYPE_TAP) ? "TAP" : "TUN",
+		rte_socket_id());
+
 	if (internals->nlsk_fd) {
 		tap_flow_flush(eth_dev, NULL);
 		tap_flow_implicit_flush(internals, NULL);
diff --git a/drivers/net/tap/rte_eth_tap.h b/drivers/net/tap/rte_eth_tap.h
index c87a0b8..7b21d0d 100644
--- a/drivers/net/tap/rte_eth_tap.h
+++ b/drivers/net/tap/rte_eth_tap.h
@@ -23,6 +23,13 @@
 #define RTE_PMD_TAP_MAX_QUEUES	1
 #endif
 
+enum rte_tuntap_type {
+	ETH_TUNTAP_TYPE_UNKNOWN,
+	ETH_TUNTAP_TYPE_TUN,
+	ETH_TUNTAP_TYPE_TAP,
+	ETH_TUNTAP_TYPE_MAX,
+};
+
 struct pkt_stats {
 	uint64_t opackets;              /* Number of output packets */
 	uint64_t ipackets;              /* Number of input packets */
@@ -48,6 +55,7 @@ struct rx_queue {
 
 struct tx_queue {
 	int fd;
+	int type;                       /* Type field - TUN|TAP */
 	uint16_t *mtu;                  /* Pointer to MTU from dev_data */
 	uint16_t csum:1;                /* Enable checksum offloading */
 	struct pkt_stats stats;         /* Stats for this TX queue */
@@ -57,6 +65,7 @@ struct pmd_internals {
 	struct rte_eth_dev *dev;          /* Ethernet device. */
 	char remote_iface[RTE_ETH_NAME_MAX_LEN]; /* Remote netdevice name */
 	char name[RTE_ETH_NAME_MAX_LEN];  /* Internal Tap device name */
+	int type;                         /* Type field - TUN|TAP */
 	struct ether_addr eth_addr;       /* Mac address of the device port */
 	struct ifreq remote_initial_flags;   /* Remote netdevice flags on init */
 	int remote_if_index;              /* remote netdevice IF_INDEX */
-- 
2.7.4

Re: [dpdk-dev] [PATCH v2] net/tap: perform proto field update for tun only

[Varghese, Vipin]

Hi Ophir, Keith and Ferruh,

I have shared v3 with all the suggested changes. 

Thanks
Vipin Varghese

> -----Original Message-----
> From: Varghese, Vipin
> Sent: Tuesday, May 22, 2018 12:04 PM
> To: Ophir Munk <ophirmu@mellanox.com>; dev@dpdk.org; Wiles, Keith
> <keith.wiles@intel.com>; Yigit, Ferruh <ferruh.yigit@intel.com>
> Cc: Thomas Monjalon <thomas@monjalon.net>; Olga Shern
> <olgas@mellanox.com>
> Subject: RE: [PATCH v2] net/tap: perform proto field update for tun only
> 
> Hi Ophir,
> 
> Thanks for the suggestion, once these are merged to dpdk or dpdk-net mainline,
> I can start the modifications.
> 
> Thanks
> Vipin Varghese
> 
> > -----Original Message-----
> > From: Ophir Munk [mailto:ophirmu@mellanox.com]
> > Sent: Wednesday, May 16, 2018 3:59 AM
> > To: Varghese, Vipin <vipin.varghese@intel.com>; dev@dpdk.org; Wiles,
> > Keith <keith.wiles@intel.com>; Yigit, Ferruh <ferruh.yigit@intel.com>
> > Cc: Thomas Monjalon <thomas@monjalon.net>; Olga Shern
> > <olgas@mellanox.com>
> > Subject: RE: [PATCH v2] net/tap: perform proto field update for tun
> > only
> >
> > Hi,
> > I am adding my review although this patch has already been acked.
> > Please find some comments inline. Otherwise it looks OK.
> >
> > Regards,
> > Ophir
> >
> > > -----Original Message-----
> > > From: Vipin Varghese [mailto:vipin.varghese@intel.com]
> > > Sent: Tuesday, May 15, 2018 5:50 PM
> > > To: dev@dpdk.org; keith.wiles@intel.com; ferruh.yigit@intel.com;
> > > Ophir Munk <ophirmu@mellanox.com>
> > > Cc: Vipin Varghese <vipin.varghese@intel.com>
> > > Subject: [PATCH v2] net/tap: perform proto field update for tun only
> > >
> > > The TX function is shared between TAP and TUN PMD. Checking TUN-TAP
> > > type field will ensure the TAP PMD will always have protocol field as 0.
> > >
> > > Signed-off-by: Vipin Varghese <vipin.varghese@intel.com>
> > > Suggested-by: Ferruh Yigit <ferruh.yigit@intel.com>
> > > ---
> > >
> > > Changes in V2:
> > > updated in comment wording - Keith Wiles remove debug print from tx
> > > code
> > > - Keith Wiles
> > > ---
> > >  drivers/net/tap/rte_eth_tap.c | 48
> > > ++++++++++++++++++++++++++-----------
> > > ------
> > >  drivers/net/tap/rte_eth_tap.h | 10 +++++++++
> > >  2 files changed, 39 insertions(+), 19 deletions(-)
> > >
> > > diff --git a/drivers/net/tap/rte_eth_tap.c
> > > b/drivers/net/tap/rte_eth_tap.c index ea6d899..fafd557 100644
> > > --- a/drivers/net/tap/rte_eth_tap.c
> > > +++ b/drivers/net/tap/rte_eth_tap.c
> > > @@ -115,7 +115,8 @@ tun_alloc(struct pmd_internals *pmd)
> > >  	 * Do not set IFF_NO_PI as packet information header will be needed
> > >  	 * to check if a received packet has been truncated.
> > >  	 */
> > > -	ifr.ifr_flags = (tap_type) ? IFF_TAP : IFF_TUN | IFF_POINTOPOINT;
> > > +	ifr.ifr_flags = (pmd->type == ETH_TUNTAP_TYPE_TAP) ?
> > > +			IFF_TAP : IFF_TUN | IFF_POINTOPOINT;
> > >  	snprintf(ifr.ifr_name, IFNAMSIZ, "%s", pmd->name);
> > >
> > >  	TAP_LOG(DEBUG, "ifr_name '%s'", ifr.ifr_name); @@ -502,20
> > > +503,22 @@ pmd_tx_burst(void *queue, struct rte_mbuf **bufs,
> > > +uint16_t
> > > nb_pkts)
> > >  		if (rte_pktmbuf_pkt_len(mbuf) > max_size)
> > >  			break;
> > >
> > > -		/*
> > > -		 * TUN and TAP are created with IFF_NO_PI disabled.
> > > -		 * For TUN PMD this mandatory as fields are used by
> > > -		 * Kernel tun.c to determine whether its IP or non IP
> > > -		 * packets.
> > > -		 *
> > > -		 * The logic fetches the first byte of data from mbuf.
> > > -		 * compares whether its v4 or v6. If none matches default
> > > -		 * value 0x00 is taken for protocol field.
> > > -		 */
> > > -		char *buff_data = rte_pktmbuf_mtod(seg, void *);
> > > -		j = (*buff_data & 0xf0);
> > > -		pi.proto = (j == 0x40) ? 0x0008 :
> > > -				(j == 0x60) ? 0xdd86 : 0x00;
> > > +		if (txq->type == ETH_TUNTAP_TYPE_TUN) {
> > > +			/*
> > > +			 * TUN and TAP are created with IFF_NO_PI disabled.
> > > +			 * For TUN PMD this mandatory as fields are used by
> > > +			 * Kernel tun.c to determine whether its IP or non IP
> > > +			 * packets.
> > > +			 *
> > > +			 * The logic fetches the first byte of data from mbuf
> > > +			 * then compares whether its v4 or v6. If first byte
> > > +			 * is 4 or 6, then protocol field is updated.
> > > +			 */
> > > +			char *buff_data = rte_pktmbuf_mtod(seg, void *);
> > > +			j = (*buff_data & 0xf0);
> > > +			pi.proto = (j == 0x40) ? 0x0008 :
> > > +					(j == 0x60) ? 0xdd86 : 0x00;
> > > +		}
> > >
> > >  		iovecs[0].iov_base = &pi;
> > >  		iovecs[0].iov_len = sizeof(pi);
> > > @@ -1052,6 +1055,9 @@ tap_setup_queue(struct rte_eth_dev *dev,
> > >  	tx->mtu = &dev->data->mtu;
> > >  	rx->rxmode = &dev->data->dev_conf.rxmode;
> > >
> > > +	tx->type = pmd->type;
> > > +	rx->type = pmd->type;
> >
> > It seems there is no usage for rx->type in TAP code. Therefore - can
> > you please remove rx->type assignment?
> > > +
> > >  	return *fd;
> > >  }
> > >
> > > @@ -1386,6 +1392,7 @@ eth_dev_tap_create(struct rte_vdev_device
> > > *vdev, char *tap_name,
> > >  	pmd = dev->data->dev_private;
> > >  	pmd->dev = dev;
> > >  	snprintf(pmd->name, sizeof(pmd->name), "%s", tap_name);
> > > +	pmd->type = ETH_TUNTAP_TYPE_UNKNOWN;
> > >
> > >  	pmd->ioctl_sock = socket(AF_INET, SOCK_DGRAM, 0);
> > >  	if (pmd->ioctl_sock == -1) {
> > > @@ -1421,7 +1428,9 @@ eth_dev_tap_create(struct rte_vdev_device
> > > *vdev, char *tap_name,
> > >  		pmd->txq[i].fd = -1;
> > >  	}
> > >
> > > -	if (tap_type) {
> > > +	pmd->type = (tap_type) ? ETH_TUNTAP_TYPE_TAP :
> > > ETH_TUNTAP_TYPE_TUN;
> >
> > 1. It seem you are using 'tap_type' as a global static variable to
> > pass a value to eth_dev_tap_create().
> > Can you please pass this value by adding a parameter to
> > eth_dev_tap_create() call and remove the global static variable?
> > 2. You are using tap_type with values 0 and 1 which are converted to
> > enums ETH_TUNTAP_TYPE_TUN and ETH_TUNTAP_TYPE_TAP respectively.
> Can
> > you please always just use the enums (avoiding the conversion)?
> >
> > > +
> > > +	if (pmd->type == ETH_TUNTAP_TYPE_TAP) {
> > >  		if (is_zero_ether_addr(mac_addr))
> > >  			eth_random_addr((uint8_t *)&pmd->eth_addr);
> > >  		else
> > > @@ -1440,7 +1449,7 @@ eth_dev_tap_create(struct rte_vdev_device
> > > *vdev, char *tap_name,
> > >  	if (tap_ioctl(pmd, SIOCSIFMTU, &ifr, 1, LOCAL_AND_REMOTE) < 0)
> > >  		goto error_exit;
> > >
> > > -	if (tap_type) {
> > > +	if (pmd->type == ETH_TUNTAP_TYPE_TAP) {
> > >  		memset(&ifr, 0, sizeof(struct ifreq));
> > >  		ifr.ifr_hwaddr.sa_family = AF_LOCAL;
> > >  		rte_memcpy(ifr.ifr_hwaddr.sa_data, &pmd->eth_addr, @@ -
> > > 1812,7 +1821,9 @@ rte_pmd_tap_remove(struct rte_vdev_device *dev)
> > >  	struct pmd_internals *internals;
> > >  	int i;
> > >
> > > -	TAP_LOG(DEBUG, "Closing TUN/TAP Ethernet device on numa %u",
> > > +	internals = eth_dev->data->dev_private;
> > > +	TAP_LOG(DEBUG, "Closing %s Ethernet device on numa %u",
> > > +		(internals->type == ETH_TUNTAP_TYPE_TAP) ? "TAP" :
> > > "TUN",
> > >  		rte_socket_id());
> > >
> > >  	/* find the ethdev entry */
> > > @@ -1820,7 +1831,6 @@ rte_pmd_tap_remove(struct rte_vdev_device
> > > *dev)
> > >  	if (!eth_dev)
> > >  		return 0;
> > >
> > > -	internals = eth_dev->data->dev_private;
> > >  	if (internals->nlsk_fd) {
> > >  		tap_flow_flush(eth_dev, NULL);
> > >  		tap_flow_implicit_flush(internals, NULL); diff --git
> > > a/drivers/net/tap/rte_eth_tap.h b/drivers/net/tap/rte_eth_tap.h
> > > index 67c9d4b..8b0da5a 100644
> > > --- a/drivers/net/tap/rte_eth_tap.h
> > > +++ b/drivers/net/tap/rte_eth_tap.h
> > > @@ -23,6 +23,13 @@
> > >  #define RTE_PMD_TAP_MAX_QUEUES	1
> > >  #endif
> > >
> > > +enum rte_tuntap_type {
> > > +	ETH_TUNTAP_TYPE_UNKNOWN,
> > > +	ETH_TUNTAP_TYPE_TUN,
> > > +	ETH_TUNTAP_TYPE_TAP,
> > > +	ETH_TUNTAP_TYPE_MAX,
> > > +};
> > > +
> > >  struct pkt_stats {
> > >  	uint64_t opackets;              /* Number of output packets */
> > >  	uint64_t ipackets;              /* Number of input packets */
> > > @@ -38,6 +45,7 @@ struct rx_queue {
> > >  	uint32_t trigger_seen;          /* Last seen Rx trigger value */
> > >  	uint16_t in_port;               /* Port ID */
> > >  	int fd;
> > > +	int type;			  /* Type field - TUN|TAP */
> >
> > In seems there is no usage for 'type' field from rx_queue struct in
> > the code. If so
> > - can you please remove it?
> >
> > >  	struct pkt_stats stats;         /* Stats for this RX queue */
> > >  	uint16_t nb_rx_desc;            /* max number of mbufs available */
> > >  	struct rte_eth_rxmode *rxmode;  /* RX features */ @@ -48,6 +56,7
> > @@
> > > struct rx_queue {
> > >
> > >  struct tx_queue {
> > >  	int fd;
> > > +	int type;			  /* Type field - TUN|TAP */
> >
> > Please use spaces (like all other fields in this struct) instead of
> > tabs, so all fields look aligned.
> >
> > >  	uint16_t *mtu;                  /* Pointer to MTU from dev_data */
> > >  	uint16_t csum:1;                /* Enable checksum offloading */
> > >  	struct pkt_stats stats;         /* Stats for this TX queue */
> > > @@ -57,6 +66,7 @@ struct pmd_internals {
> > >  	struct rte_eth_dev *dev;          /* Ethernet device. */
> > >  	char remote_iface[RTE_ETH_NAME_MAX_LEN]; /* Remote netdevice
> > name */
> > >  	char name[RTE_ETH_NAME_MAX_LEN];  /* Internal Tap device name */
> > > +	int type;			  /* Type field - TUN|TAP */
> >
> > Same comment here about replacing tabs with spaces
> >
> > >  	struct ether_addr eth_addr;       /* Mac address of the device port */
> > >  	struct ifreq remote_initial_flags;   /* Remote netdevice flags on init
> > > */
> > >  	int remote_if_index;              /* remote netdevice IF_INDEX */
> > > --
> > > 2.7.4

Re: [dpdk-dev] [PATCH v3] net/tap: perform proto field update for tun only

[Wiles, Keith]

> On May 22, 2018, at 5:47 AM, Varghese, Vipin <vipin.varghese@intel.com> wrote:
> 
> The TX function is shared between TAP and TUN PMD. Checking TUN-TAP
> type field will ensure the TAP PMD will always have protocol field
> as 0.
> 
> Signed-off-by: Vipin Varghese <vipin.varghese@intel.com>
> Suggested-by: Ferruh Yigit <ferruh.yigit@intel.com>
> ---
> Changes in V3:
> - remove type field from rx struct - Ophir Munk
> - add space for comment in struct - Ophir Munk
> - pass type field into eth_dev_tap_create - Ophir Munk
> - replace with enum value for type - Ophir Munk
> - return as 'not supported' for mac_set - Vipin Varghese
> 
> Changes in V2:
> - updated in comment wording - Keith Wiles
> - remove debug print from tx code - Keith Wiles
> —

Sorry, I did not catch the problem below before :-(

> drivers/net/tap/rte_eth_tap.c | 61 +++++++++++++++++++++++++------------------
> drivers/net/tap/rte_eth_tap.h |  9 +++++++
> 2 files changed, 45 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
> index 01552fa..8d8f67b 100644
> --- a/drivers/net/tap/rte_eth_tap.c
> +++ b/drivers/net/tap/rte_eth_tap.c
> @@ -68,7 +68,6 @@ static const char *valid_arguments[] = {
> static int tap_unit;
> static unsigned int tun_unit;
> 
> -static int tap_type;
> static char tuntap_name[8];
> 
> static volatile uint32_t tap_trigger;	/* Rx trigger */
> @@ -116,7 +115,8 @@ tun_alloc(struct pmd_internals *pmd)
> 	 * Do not set IFF_NO_PI as packet information header will be needed
> 	 * to check if a received packet has been truncated.
> 	 */
> -	ifr.ifr_flags = (tap_type) ? IFF_TAP : IFF_TUN | IFF_POINTOPOINT;
> +	ifr.ifr_flags = (pmd->type == ETH_TUNTAP_TYPE_TAP) ?
> +		IFF_TAP : IFF_TUN | IFF_POINTOPOINT;
> 	snprintf(ifr.ifr_name, IFNAMSIZ, "%s", pmd->name);
> 
> 	TAP_LOG(DEBUG, "ifr_name '%s'", ifr.ifr_name);
> @@ -472,20 +472,22 @@ pmd_tx_burst(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
> 		if (rte_pktmbuf_pkt_len(mbuf) > max_size)
> 			break;
> 
> -		/*
> -		 * TUN and TAP are created with IFF_NO_PI disabled.
> -		 * For TUN PMD this mandatory as fields are used by
> -		 * Kernel tun.c to determine whether its IP or non IP
> -		 * packets.
> -		 *
> -		 * The logic fetches the first byte of data from mbuf.
> -		 * compares whether its v4 or v6. If none matches default
> -		 * value 0x00 is taken for protocol field.
> -		 */
> -		char *buff_data = rte_pktmbuf_mtod(seg, void *);
> -		j = (*buff_data & 0xf0);
> -		pi.proto = (j == 0x40) ? 0x0008 :
> +		if (txq->type == ETH_TUNTAP_TYPE_TUN) {
> +			/*
> +			 * TUN and TAP are created with IFF_NO_PI disabled.
> +			 * For TUN PMD this mandatory as fields are used by
> +			 * Kernel tun.c to determine whether its IP or non IP
> +			 * packets.
> +			 *
> +			 * The logic fetches the first byte of data from mbuf
> +			 * then compares whether its v4 or v6. If first byte
> +			 * is 4 or 6, then protocol field is updated.
> +			 */
> +			char *buff_data = rte_pktmbuf_mtod(seg, void *);
> +			j = (*buff_data & 0xf0);
> +			pi.proto = (j == 0x40) ? 0x0008 :
> 				(j == 0x60) ? 0xdd86 : 0x00;

Warning Will Robinson: Magic numbers :-)

Can we use the correct values here ETHERTYPE_IPV6 and ETHERTYPE_IP and then use htons() on the values please.

> +		}
> 
> 		iovecs[0].iov_base = &pi;
> 		iovecs[0].iov_len = sizeof(pi);
> @@ -928,6 +930,9 @@ tap_mac_set(struct rte_eth_dev *dev, struct ether_addr *mac_addr)
> 	struct ifreq ifr;
> 	int ret;
> 
> +	if (pmd->type == ETH_TUNTAP_TYPE_TUN)
> +		return -ENOTSUP;
> +
> 	if (is_zero_ether_addr(mac_addr)) {
> 		TAP_LOG(ERR, "%s: can't set an empty MAC address",
> 			dev->device->name);
> @@ -1025,6 +1030,8 @@ tap_setup_queue(struct rte_eth_dev *dev,
> 	tx->mtu = &dev->data->mtu;
> 	rx->rxmode = &dev->data->dev_conf.rxmode;
> 
> +	tx->type = pmd->type;
> +
> 	return *fd;
> }
> 
> @@ -1342,7 +1349,8 @@ static const struct eth_dev_ops ops = {
> 
> static int
> eth_dev_tap_create(struct rte_vdev_device *vdev, char *tap_name,
> -		   char *remote_iface, struct ether_addr *mac_addr)
> +		   char *remote_iface, struct ether_addr *mac_addr,
> +		   enum rte_tuntap_type type)
> {
> 	int numa_node = rte_socket_id();
> 	struct rte_eth_dev *dev;
> @@ -1364,6 +1372,7 @@ eth_dev_tap_create(struct rte_vdev_device *vdev, char *tap_name,
> 	pmd = dev->data->dev_private;
> 	pmd->dev = dev;
> 	snprintf(pmd->name, sizeof(pmd->name), "%s", tap_name);
> +	pmd->type = type;
> 
> 	pmd->ioctl_sock = socket(AF_INET, SOCK_DGRAM, 0);
> 	if (pmd->ioctl_sock == -1) {
> @@ -1400,7 +1409,7 @@ eth_dev_tap_create(struct rte_vdev_device *vdev, char *tap_name,
> 		pmd->txq[i].fd = -1;
> 	}
> 
> -	if (tap_type) {
> +	if (pmd->type == ETH_TUNTAP_TYPE_TAP) {
> 		if (is_zero_ether_addr(mac_addr))
> 			eth_random_addr((uint8_t *)&pmd->eth_addr);
> 		else
> @@ -1423,7 +1432,7 @@ eth_dev_tap_create(struct rte_vdev_device *vdev, char *tap_name,
> 	if (tap_ioctl(pmd, SIOCSIFMTU, &ifr, 1, LOCAL_AND_REMOTE) < 0)
> 		goto error_exit;
> 
> -	if (tap_type) {
> +	if (pmd->type == ETH_TUNTAP_TYPE_TAP) {
> 		memset(&ifr, 0, sizeof(struct ifreq));
> 		ifr.ifr_hwaddr.sa_family = AF_LOCAL;
> 		rte_memcpy(ifr.ifr_hwaddr.sa_data, &pmd->eth_addr,
> @@ -1642,7 +1651,6 @@ rte_pmd_tun_probe(struct rte_vdev_device *dev)
> 	char tun_name[RTE_ETH_NAME_MAX_LEN];
> 	char remote_iface[RTE_ETH_NAME_MAX_LEN];
> 
> -	tap_type = 0;
> 	strcpy(tuntap_name, "TUN");
> 
> 	name = rte_vdev_device_name(dev);
> @@ -1673,7 +1681,8 @@ rte_pmd_tun_probe(struct rte_vdev_device *dev)
> 	TAP_LOG(NOTICE, "Initializing pmd_tun for %s as %s",
> 		name, tun_name);
> 
> -	ret = eth_dev_tap_create(dev, tun_name, remote_iface, 0);
> +	ret = eth_dev_tap_create(dev, tun_name, remote_iface, 0,
> +		ETH_TUNTAP_TYPE_TUN);
> 
> leave:
> 	if (ret == -1) {
> @@ -1700,7 +1709,6 @@ rte_pmd_tap_probe(struct rte_vdev_device *dev)
> 	struct ether_addr user_mac = { .addr_bytes = {0} };
> 	struct rte_eth_dev *eth_dev;
> 
> -	tap_type = 1;
> 	strcpy(tuntap_name, "TAP");
> 
> 	name = rte_vdev_device_name(dev);
> @@ -1762,7 +1770,8 @@ rte_pmd_tap_probe(struct rte_vdev_device *dev)
> 	TAP_LOG(NOTICE, "Initializing pmd_tap for %s as %s",
> 		name, tap_name);
> 
> -	ret = eth_dev_tap_create(dev, tap_name, remote_iface, &user_mac);
> +	ret = eth_dev_tap_create(dev, tap_name, remote_iface, &user_mac,
> +		ETH_TUNTAP_TYPE_TAP);
> 
> leave:
> 	if (ret == -1) {
> @@ -1784,15 +1793,17 @@ rte_pmd_tap_remove(struct rte_vdev_device *dev)
> 	struct pmd_internals *internals;
> 	int i;
> 
> -	TAP_LOG(DEBUG, "Closing TUN/TAP Ethernet device on numa %u",
> -		rte_socket_id());
> -
> 	/* find the ethdev entry */
> 	eth_dev = rte_eth_dev_allocated(rte_vdev_device_name(dev));
> 	if (!eth_dev)
> 		return 0;
> 
> 	internals = eth_dev->data->dev_private;
> +
> +	TAP_LOG(DEBUG, "Closing %s Ethernet device on numa %u",
> +		(internals->type == ETH_TUNTAP_TYPE_TAP) ? "TAP" : "TUN",
> +		rte_socket_id());
> +
> 	if (internals->nlsk_fd) {
> 		tap_flow_flush(eth_dev, NULL);
> 		tap_flow_implicit_flush(internals, NULL);
> diff --git a/drivers/net/tap/rte_eth_tap.h b/drivers/net/tap/rte_eth_tap.h
> index c87a0b8..7b21d0d 100644
> --- a/drivers/net/tap/rte_eth_tap.h
> +++ b/drivers/net/tap/rte_eth_tap.h
> @@ -23,6 +23,13 @@
> #define RTE_PMD_TAP_MAX_QUEUES	1
> #endif
> 
> +enum rte_tuntap_type {
> +	ETH_TUNTAP_TYPE_UNKNOWN,
> +	ETH_TUNTAP_TYPE_TUN,
> +	ETH_TUNTAP_TYPE_TAP,
> +	ETH_TUNTAP_TYPE_MAX,
> +};
> +
> struct pkt_stats {
> 	uint64_t opackets;              /* Number of output packets */
> 	uint64_t ipackets;              /* Number of input packets */
> @@ -48,6 +55,7 @@ struct rx_queue {
> 
> struct tx_queue {
> 	int fd;
> +	int type;                       /* Type field - TUN|TAP */
> 	uint16_t *mtu;                  /* Pointer to MTU from dev_data */
> 	uint16_t csum:1;                /* Enable checksum offloading */
> 	struct pkt_stats stats;         /* Stats for this TX queue */
> @@ -57,6 +65,7 @@ struct pmd_internals {
> 	struct rte_eth_dev *dev;          /* Ethernet device. */
> 	char remote_iface[RTE_ETH_NAME_MAX_LEN]; /* Remote netdevice name */
> 	char name[RTE_ETH_NAME_MAX_LEN];  /* Internal Tap device name */
> +	int type;                         /* Type field - TUN|TAP */
> 	struct ether_addr eth_addr;       /* Mac address of the device port */
> 	struct ifreq remote_initial_flags;   /* Remote netdevice flags on init */
> 	int remote_if_index;              /* remote netdevice IF_INDEX */
> -- 
> 2.7.4
> 

Regards,
Keith

Re: [dpdk-dev] [PATCH v2] net/tap: perform proto field update for tun only

[Wiles, Keith]

> On May 22, 2018, at 6:05 AM, Varghese, Vipin <vipin.varghese@intel.com> wrote:
> 
> Hi Ophir, Keith and Ferruh,
> 
> I have shared v3 with all the suggested changes. 

I just sent another change, sorry I did not catch if before, which I obviously was not paying attention.

> 
> Thanks
> Vipin Varghese
> 
>> -----Original Message-----
>> From: Varghese, Vipin
>> Sent: Tuesday, May 22, 2018 12:04 PM
>> To: Ophir Munk <ophirmu@mellanox.com>; dev@dpdk.org; Wiles, Keith
>> <keith.wiles@intel.com>; Yigit, Ferruh <ferruh.yigit@intel.com>
>> Cc: Thomas Monjalon <thomas@monjalon.net>; Olga Shern
>> <olgas@mellanox.com>
>> Subject: RE: [PATCH v2] net/tap: perform proto field update for tun only
>> 
>> Hi Ophir,
>> 
>> Thanks for the suggestion, once these are merged to dpdk or dpdk-net mainline,
>> I can start the modifications.
>> 
>> Thanks
>> Vipin Varghese
>> 
>>> -----Original Message-----
>>> From: Ophir Munk [mailto:ophirmu@mellanox.com]
>>> Sent: Wednesday, May 16, 2018 3:59 AM
>>> To: Varghese, Vipin <vipin.varghese@intel.com>; dev@dpdk.org; Wiles,
>>> Keith <keith.wiles@intel.com>; Yigit, Ferruh <ferruh.yigit@intel.com>
>>> Cc: Thomas Monjalon <thomas@monjalon.net>; Olga Shern
>>> <olgas@mellanox.com>
>>> Subject: RE: [PATCH v2] net/tap: perform proto field update for tun
>>> only
>>> 
>>> Hi,
>>> I am adding my review although this patch has already been acked.
>>> Please find some comments inline. Otherwise it looks OK.
>>> 
>>> Regards,
>>> Ophir
>>> 
>>>> -----Original Message-----
>>>> From: Vipin Varghese [mailto:vipin.varghese@intel.com]
>>>> Sent: Tuesday, May 15, 2018 5:50 PM
>>>> To: dev@dpdk.org; keith.wiles@intel.com; ferruh.yigit@intel.com;
>>>> Ophir Munk <ophirmu@mellanox.com>
>>>> Cc: Vipin Varghese <vipin.varghese@intel.com>
>>>> Subject: [PATCH v2] net/tap: perform proto field update for tun only
>>>> 
>>>> The TX function is shared between TAP and TUN PMD. Checking TUN-TAP
>>>> type field will ensure the TAP PMD will always have protocol field as 0.
>>>> 
>>>> Signed-off-by: Vipin Varghese <vipin.varghese@intel.com>
>>>> Suggested-by: Ferruh Yigit <ferruh.yigit@intel.com>
>>>> ---
>>>> 
>>>> Changes in V2:
>>>> updated in comment wording - Keith Wiles remove debug print from tx
>>>> code
>>>> - Keith Wiles
>>>> ---
>>>> drivers/net/tap/rte_eth_tap.c | 48
>>>> ++++++++++++++++++++++++++-----------
>>>> ------
>>>> drivers/net/tap/rte_eth_tap.h | 10 +++++++++
>>>> 2 files changed, 39 insertions(+), 19 deletions(-)
>>>> 
>>>> diff --git a/drivers/net/tap/rte_eth_tap.c
>>>> b/drivers/net/tap/rte_eth_tap.c index ea6d899..fafd557 100644
>>>> --- a/drivers/net/tap/rte_eth_tap.c
>>>> +++ b/drivers/net/tap/rte_eth_tap.c
>>>> @@ -115,7 +115,8 @@ tun_alloc(struct pmd_internals *pmd)
>>>> 	 * Do not set IFF_NO_PI as packet information header will be needed
>>>> 	 * to check if a received packet has been truncated.
>>>> 	 */
>>>> -	ifr.ifr_flags = (tap_type) ? IFF_TAP : IFF_TUN | IFF_POINTOPOINT;
>>>> +	ifr.ifr_flags = (pmd->type == ETH_TUNTAP_TYPE_TAP) ?
>>>> +			IFF_TAP : IFF_TUN | IFF_POINTOPOINT;
>>>> 	snprintf(ifr.ifr_name, IFNAMSIZ, "%s", pmd->name);
>>>> 
>>>> 	TAP_LOG(DEBUG, "ifr_name '%s'", ifr.ifr_name); @@ -502,20
>>>> +503,22 @@ pmd_tx_burst(void *queue, struct rte_mbuf **bufs,
>>>> +uint16_t
>>>> nb_pkts)
>>>> 		if (rte_pktmbuf_pkt_len(mbuf) > max_size)
>>>> 			break;
>>>> 
>>>> -		/*
>>>> -		 * TUN and TAP are created with IFF_NO_PI disabled.
>>>> -		 * For TUN PMD this mandatory as fields are used by
>>>> -		 * Kernel tun.c to determine whether its IP or non IP
>>>> -		 * packets.
>>>> -		 *
>>>> -		 * The logic fetches the first byte of data from mbuf.
>>>> -		 * compares whether its v4 or v6. If none matches default
>>>> -		 * value 0x00 is taken for protocol field.
>>>> -		 */
>>>> -		char *buff_data = rte_pktmbuf_mtod(seg, void *);
>>>> -		j = (*buff_data & 0xf0);
>>>> -		pi.proto = (j == 0x40) ? 0x0008 :
>>>> -				(j == 0x60) ? 0xdd86 : 0x00;
>>>> +		if (txq->type == ETH_TUNTAP_TYPE_TUN) {
>>>> +			/*
>>>> +			 * TUN and TAP are created with IFF_NO_PI disabled.
>>>> +			 * For TUN PMD this mandatory as fields are used by
>>>> +			 * Kernel tun.c to determine whether its IP or non IP
>>>> +			 * packets.
>>>> +			 *
>>>> +			 * The logic fetches the first byte of data from mbuf
>>>> +			 * then compares whether its v4 or v6. If first byte
>>>> +			 * is 4 or 6, then protocol field is updated.
>>>> +			 */
>>>> +			char *buff_data = rte_pktmbuf_mtod(seg, void *);
>>>> +			j = (*buff_data & 0xf0);
>>>> +			pi.proto = (j == 0x40) ? 0x0008 :
>>>> +					(j == 0x60) ? 0xdd86 : 0x00;
>>>> +		}
>>>> 
>>>> 		iovecs[0].iov_base = &pi;
>>>> 		iovecs[0].iov_len = sizeof(pi);
>>>> @@ -1052,6 +1055,9 @@ tap_setup_queue(struct rte_eth_dev *dev,
>>>> 	tx->mtu = &dev->data->mtu;
>>>> 	rx->rxmode = &dev->data->dev_conf.rxmode;
>>>> 
>>>> +	tx->type = pmd->type;
>>>> +	rx->type = pmd->type;
>>> 
>>> It seems there is no usage for rx->type in TAP code. Therefore - can
>>> you please remove rx->type assignment?
>>>> +
>>>> 	return *fd;
>>>> }
>>>> 
>>>> @@ -1386,6 +1392,7 @@ eth_dev_tap_create(struct rte_vdev_device
>>>> *vdev, char *tap_name,
>>>> 	pmd = dev->data->dev_private;
>>>> 	pmd->dev = dev;
>>>> 	snprintf(pmd->name, sizeof(pmd->name), "%s", tap_name);
>>>> +	pmd->type = ETH_TUNTAP_TYPE_UNKNOWN;
>>>> 
>>>> 	pmd->ioctl_sock = socket(AF_INET, SOCK_DGRAM, 0);
>>>> 	if (pmd->ioctl_sock == -1) {
>>>> @@ -1421,7 +1428,9 @@ eth_dev_tap_create(struct rte_vdev_device
>>>> *vdev, char *tap_name,
>>>> 		pmd->txq[i].fd = -1;
>>>> 	}
>>>> 
>>>> -	if (tap_type) {
>>>> +	pmd->type = (tap_type) ? ETH_TUNTAP_TYPE_TAP :
>>>> ETH_TUNTAP_TYPE_TUN;
>>> 
>>> 1. It seem you are using 'tap_type' as a global static variable to
>>> pass a value to eth_dev_tap_create().
>>> Can you please pass this value by adding a parameter to
>>> eth_dev_tap_create() call and remove the global static variable?
>>> 2. You are using tap_type with values 0 and 1 which are converted to
>>> enums ETH_TUNTAP_TYPE_TUN and ETH_TUNTAP_TYPE_TAP respectively.
>> Can
>>> you please always just use the enums (avoiding the conversion)?
>>> 
>>>> +
>>>> +	if (pmd->type == ETH_TUNTAP_TYPE_TAP) {
>>>> 		if (is_zero_ether_addr(mac_addr))
>>>> 			eth_random_addr((uint8_t *)&pmd->eth_addr);
>>>> 		else
>>>> @@ -1440,7 +1449,7 @@ eth_dev_tap_create(struct rte_vdev_device
>>>> *vdev, char *tap_name,
>>>> 	if (tap_ioctl(pmd, SIOCSIFMTU, &ifr, 1, LOCAL_AND_REMOTE) < 0)
>>>> 		goto error_exit;
>>>> 
>>>> -	if (tap_type) {
>>>> +	if (pmd->type == ETH_TUNTAP_TYPE_TAP) {
>>>> 		memset(&ifr, 0, sizeof(struct ifreq));
>>>> 		ifr.ifr_hwaddr.sa_family = AF_LOCAL;
>>>> 		rte_memcpy(ifr.ifr_hwaddr.sa_data, &pmd->eth_addr, @@ -
>>>> 1812,7 +1821,9 @@ rte_pmd_tap_remove(struct rte_vdev_device *dev)
>>>> 	struct pmd_internals *internals;
>>>> 	int i;
>>>> 
>>>> -	TAP_LOG(DEBUG, "Closing TUN/TAP Ethernet device on numa %u",
>>>> +	internals = eth_dev->data->dev_private;
>>>> +	TAP_LOG(DEBUG, "Closing %s Ethernet device on numa %u",
>>>> +		(internals->type == ETH_TUNTAP_TYPE_TAP) ? "TAP" :
>>>> "TUN",
>>>> 		rte_socket_id());
>>>> 
>>>> 	/* find the ethdev entry */
>>>> @@ -1820,7 +1831,6 @@ rte_pmd_tap_remove(struct rte_vdev_device
>>>> *dev)
>>>> 	if (!eth_dev)
>>>> 		return 0;
>>>> 
>>>> -	internals = eth_dev->data->dev_private;
>>>> 	if (internals->nlsk_fd) {
>>>> 		tap_flow_flush(eth_dev, NULL);
>>>> 		tap_flow_implicit_flush(internals, NULL); diff --git
>>>> a/drivers/net/tap/rte_eth_tap.h b/drivers/net/tap/rte_eth_tap.h
>>>> index 67c9d4b..8b0da5a 100644
>>>> --- a/drivers/net/tap/rte_eth_tap.h
>>>> +++ b/drivers/net/tap/rte_eth_tap.h
>>>> @@ -23,6 +23,13 @@
>>>> #define RTE_PMD_TAP_MAX_QUEUES	1
>>>> #endif
>>>> 
>>>> +enum rte_tuntap_type {
>>>> +	ETH_TUNTAP_TYPE_UNKNOWN,
>>>> +	ETH_TUNTAP_TYPE_TUN,
>>>> +	ETH_TUNTAP_TYPE_TAP,
>>>> +	ETH_TUNTAP_TYPE_MAX,
>>>> +};
>>>> +
>>>> struct pkt_stats {
>>>> 	uint64_t opackets;              /* Number of output packets */
>>>> 	uint64_t ipackets;              /* Number of input packets */
>>>> @@ -38,6 +45,7 @@ struct rx_queue {
>>>> 	uint32_t trigger_seen;          /* Last seen Rx trigger value */
>>>> 	uint16_t in_port;               /* Port ID */
>>>> 	int fd;
>>>> +	int type;			  /* Type field - TUN|TAP */
>>> 
>>> In seems there is no usage for 'type' field from rx_queue struct in
>>> the code. If so
>>> - can you please remove it?
>>> 
>>>> 	struct pkt_stats stats;         /* Stats for this RX queue */
>>>> 	uint16_t nb_rx_desc;            /* max number of mbufs available */
>>>> 	struct rte_eth_rxmode *rxmode;  /* RX features */ @@ -48,6 +56,7
>>> @@
>>>> struct rx_queue {
>>>> 
>>>> struct tx_queue {
>>>> 	int fd;
>>>> +	int type;			  /* Type field - TUN|TAP */
>>> 
>>> Please use spaces (like all other fields in this struct) instead of
>>> tabs, so all fields look aligned.
>>> 
>>>> 	uint16_t *mtu;                  /* Pointer to MTU from dev_data */
>>>> 	uint16_t csum:1;                /* Enable checksum offloading */
>>>> 	struct pkt_stats stats;         /* Stats for this TX queue */
>>>> @@ -57,6 +66,7 @@ struct pmd_internals {
>>>> 	struct rte_eth_dev *dev;          /* Ethernet device. */
>>>> 	char remote_iface[RTE_ETH_NAME_MAX_LEN]; /* Remote netdevice
>>> name */
>>>> 	char name[RTE_ETH_NAME_MAX_LEN];  /* Internal Tap device name */
>>>> +	int type;			  /* Type field - TUN|TAP */
>>> 
>>> Same comment here about replacing tabs with spaces
>>> 
>>>> 	struct ether_addr eth_addr;       /* Mac address of the device port */
>>>> 	struct ifreq remote_initial_flags;   /* Remote netdevice flags on init
>>>> */
>>>> 	int remote_if_index;              /* remote netdevice IF_INDEX */
>>>> --
>>>> 2.7.4
> 

Regards,
Keith

Re: [dpdk-dev] [PATCH v3] net/tap: perform proto field update for tun only

[Ophir Munk]

Hi,
Overall it looks good.
Please note a few more comments below.

> -----Original Message-----
> From: Vipin Varghese [mailto:vipin.varghese@intel.com]
> Sent: Tuesday, May 22, 2018 1:47 PM
> To: dev@dpdk.org; ferruh.yigit@intel.com; keith.wiles@intel.com; Ophir
> Munk <ophirmu@mellanox.com>
> Cc: Thomas Monjalon <thomas@monjalon.net>; Olga Shern
> <olgas@mellanox.com>; Vipin Varghese <vipin.varghese@intel.com>
> Subject: [PATCH v3] net/tap: perform proto field update for tun only
> 
> The TX function is shared between TAP and TUN PMD. Checking TUN-TAP
> type field will ensure the TAP PMD will always have protocol field as 0.
> 
> Signed-off-by: Vipin Varghese <vipin.varghese@intel.com>
> Suggested-by: Ferruh Yigit <ferruh.yigit@intel.com>
> ---

Shouldn't it be a fix commit?
If so - please update the commit title, add Fixes: ... , Cc: stable@dpdk.org...

> Changes in V3:
> - remove type field from rx struct - Ophir Munk
> - add space for comment in struct - Ophir Munk
> - pass type field into eth_dev_tap_create - Ophir Munk
> - replace with enum value for type - Ophir Munk
> - return as 'not supported' for mac_set - Vipin Varghese
> 
> Changes in V2:
> - updated in comment wording - Keith Wiles
> - remove debug print from tx code - Keith Wiles
> ---
>  drivers/net/tap/rte_eth_tap.c | 61 +++++++++++++++++++++++++-------------
> -----
>  drivers/net/tap/rte_eth_tap.h |  9 +++++++
>  2 files changed, 45 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
> index 01552fa..8d8f67b 100644
> --- a/drivers/net/tap/rte_eth_tap.c
> +++ b/drivers/net/tap/rte_eth_tap.c
> @@ -68,7 +68,6 @@ static const char *valid_arguments[] = {  static int
> tap_unit;  static unsigned int tun_unit;
> 
> -static int tap_type;
>  static char tuntap_name[8];
> 
>  static volatile uint32_t tap_trigger;	/* Rx trigger */
> @@ -116,7 +115,8 @@ tun_alloc(struct pmd_internals *pmd)
>  	 * Do not set IFF_NO_PI as packet information header will be needed
>  	 * to check if a received packet has been truncated.
>  	 */
> -	ifr.ifr_flags = (tap_type) ? IFF_TAP : IFF_TUN | IFF_POINTOPOINT;
> +	ifr.ifr_flags = (pmd->type == ETH_TUNTAP_TYPE_TAP) ?
> +		IFF_TAP : IFF_TUN | IFF_POINTOPOINT;
>  	snprintf(ifr.ifr_name, IFNAMSIZ, "%s", pmd->name);
> 
>  	TAP_LOG(DEBUG, "ifr_name '%s'", ifr.ifr_name); @@ -472,20
> +472,22 @@ pmd_tx_burst(void *queue, struct rte_mbuf **bufs, uint16_t
> nb_pkts)
>  		if (rte_pktmbuf_pkt_len(mbuf) > max_size)
>  			break;
> 
> -		/*
> -		 * TUN and TAP are created with IFF_NO_PI disabled.
> -		 * For TUN PMD this mandatory as fields are used by
> -		 * Kernel tun.c to determine whether its IP or non IP
> -		 * packets.
> -		 *
> -		 * The logic fetches the first byte of data from mbuf.
> -		 * compares whether its v4 or v6. If none matches default
> -		 * value 0x00 is taken for protocol field.
> -		 */
> -		char *buff_data = rte_pktmbuf_mtod(seg, void *);
> -		j = (*buff_data & 0xf0);
> -		pi.proto = (j == 0x40) ? 0x0008 :
> +		if (txq->type == ETH_TUNTAP_TYPE_TUN) {
> +			/*
> +			 * TUN and TAP are created with IFF_NO_PI disabled.
> +			 * For TUN PMD this mandatory as fields are used by
> +			 * Kernel tun.c to determine whether its IP or non IP
> +			 * packets.
> +			 *
> +			 * The logic fetches the first byte of data from mbuf
> +			 * then compares whether its v4 or v6. If first byte
> +			 * is 4 or 6, then protocol field is updated.
> +			 */
> +			char *buff_data = rte_pktmbuf_mtod(seg, void *);
> +			j = (*buff_data & 0xf0);
> +			pi.proto = (j == 0x40) ? 0x0008 :
>  				(j == 0x60) ? 0xdd86 : 0x00;
> +		}
> 
>  		iovecs[0].iov_base = &pi;
>  		iovecs[0].iov_len = sizeof(pi);
> @@ -928,6 +930,9 @@ tap_mac_set(struct rte_eth_dev *dev, struct
> ether_addr *mac_addr)
>  	struct ifreq ifr;
>  	int ret;
> 
> +	if (pmd->type == ETH_TUNTAP_TYPE_TUN)
> +		return -ENOTSUP;


Can you please add a TAP_LOG(ERR, ...) to log this error similar to other error cases in this function?

> +
>  	if (is_zero_ether_addr(mac_addr)) {
>  		TAP_LOG(ERR, "%s: can't set an empty MAC address",
>  			dev->device->name);
> @@ -1025,6 +1030,8 @@ tap_setup_queue(struct rte_eth_dev *dev,
>  	tx->mtu = &dev->data->mtu;
>  	rx->rxmode = &dev->data->dev_conf.rxmode;
> 
> +	tx->type = pmd->type;
> +
>  	return *fd;
>  }
> 
> @@ -1342,7 +1349,8 @@ static const struct eth_dev_ops ops = {
> 
>  static int
>  eth_dev_tap_create(struct rte_vdev_device *vdev, char *tap_name,
> -		   char *remote_iface, struct ether_addr *mac_addr)
> +		   char *remote_iface, struct ether_addr *mac_addr,
> +		   enum rte_tuntap_type type)
>  {
>  	int numa_node = rte_socket_id();
>  	struct rte_eth_dev *dev;
> @@ -1364,6 +1372,7 @@ eth_dev_tap_create(struct rte_vdev_device
> *vdev, char *tap_name,
>  	pmd = dev->data->dev_private;
>  	pmd->dev = dev;
>  	snprintf(pmd->name, sizeof(pmd->name), "%s", tap_name);
> +	pmd->type = type;
> 
>  	pmd->ioctl_sock = socket(AF_INET, SOCK_DGRAM, 0);
>  	if (pmd->ioctl_sock == -1) {
> @@ -1400,7 +1409,7 @@ eth_dev_tap_create(struct rte_vdev_device
> *vdev, char *tap_name,
>  		pmd->txq[i].fd = -1;
>  	}
> 
> -	if (tap_type) {
> +	if (pmd->type == ETH_TUNTAP_TYPE_TAP) {
>  		if (is_zero_ether_addr(mac_addr))
>  			eth_random_addr((uint8_t *)&pmd->eth_addr);
>  		else
> @@ -1423,7 +1432,7 @@ eth_dev_tap_create(struct rte_vdev_device
> *vdev, char *tap_name,
>  	if (tap_ioctl(pmd, SIOCSIFMTU, &ifr, 1, LOCAL_AND_REMOTE) < 0)
>  		goto error_exit;
> 
> -	if (tap_type) {
> +	if (pmd->type == ETH_TUNTAP_TYPE_TAP) {
>  		memset(&ifr, 0, sizeof(struct ifreq));
>  		ifr.ifr_hwaddr.sa_family = AF_LOCAL;
>  		rte_memcpy(ifr.ifr_hwaddr.sa_data, &pmd->eth_addr, @@ -
> 1642,7 +1651,6 @@ rte_pmd_tun_probe(struct rte_vdev_device *dev)
>  	char tun_name[RTE_ETH_NAME_MAX_LEN];
>  	char remote_iface[RTE_ETH_NAME_MAX_LEN];
> 
> -	tap_type = 0;
>  	strcpy(tuntap_name, "TUN");
> 
>  	name = rte_vdev_device_name(dev);
> @@ -1673,7 +1681,8 @@ rte_pmd_tun_probe(struct rte_vdev_device *dev)
>  	TAP_LOG(NOTICE, "Initializing pmd_tun for %s as %s",
>  		name, tun_name);
> 
> -	ret = eth_dev_tap_create(dev, tun_name, remote_iface, 0);
> +	ret = eth_dev_tap_create(dev, tun_name, remote_iface, 0,
> +		ETH_TUNTAP_TYPE_TUN);
> 
>  leave:
>  	if (ret == -1) {
> @@ -1700,7 +1709,6 @@ rte_pmd_tap_probe(struct rte_vdev_device *dev)
>  	struct ether_addr user_mac = { .addr_bytes = {0} };
>  	struct rte_eth_dev *eth_dev;
> 
> -	tap_type = 1;
>  	strcpy(tuntap_name, "TAP");
> 
>  	name = rte_vdev_device_name(dev);
> @@ -1762,7 +1770,8 @@ rte_pmd_tap_probe(struct rte_vdev_device *dev)
>  	TAP_LOG(NOTICE, "Initializing pmd_tap for %s as %s",
>  		name, tap_name);
> 
> -	ret = eth_dev_tap_create(dev, tap_name, remote_iface,
> &user_mac);
> +	ret = eth_dev_tap_create(dev, tap_name, remote_iface, &user_mac,
> +		ETH_TUNTAP_TYPE_TAP);
> 
>  leave:
>  	if (ret == -1) {
> @@ -1784,15 +1793,17 @@ rte_pmd_tap_remove(struct rte_vdev_device
> *dev)
>  	struct pmd_internals *internals;
>  	int i;
> 
> -	TAP_LOG(DEBUG, "Closing TUN/TAP Ethernet device on numa %u",
> -		rte_socket_id());
> -
>  	/* find the ethdev entry */
>  	eth_dev = rte_eth_dev_allocated(rte_vdev_device_name(dev));
>  	if (!eth_dev)
>  		return 0;
> 
>  	internals = eth_dev->data->dev_private;
> +
> +	TAP_LOG(DEBUG, "Closing %s Ethernet device on numa %u",
> +		(internals->type == ETH_TUNTAP_TYPE_TAP) ? "TAP" :
> "TUN",
> +		rte_socket_id());
> +
>  	if (internals->nlsk_fd) {
>  		tap_flow_flush(eth_dev, NULL);
>  		tap_flow_implicit_flush(internals, NULL); diff --git
> a/drivers/net/tap/rte_eth_tap.h b/drivers/net/tap/rte_eth_tap.h index
> c87a0b8..7b21d0d 100644
> --- a/drivers/net/tap/rte_eth_tap.h
> +++ b/drivers/net/tap/rte_eth_tap.h
> @@ -23,6 +23,13 @@
>  #define RTE_PMD_TAP_MAX_QUEUES	1
>  #endif
> 
> +enum rte_tuntap_type {
> +	ETH_TUNTAP_TYPE_UNKNOWN,
> +	ETH_TUNTAP_TYPE_TUN,
> +	ETH_TUNTAP_TYPE_TAP,
> +	ETH_TUNTAP_TYPE_MAX,
> +};
> +
>  struct pkt_stats {
>  	uint64_t opackets;              /* Number of output packets */
>  	uint64_t ipackets;              /* Number of input packets */
> @@ -48,6 +55,7 @@ struct rx_queue {
> 
>  struct tx_queue {
>  	int fd;
> +	int type;                       /* Type field - TUN|TAP */
>  	uint16_t *mtu;                  /* Pointer to MTU from dev_data */
>  	uint16_t csum:1;                /* Enable checksum offloading */
>  	struct pkt_stats stats;         /* Stats for this TX queue */
> @@ -57,6 +65,7 @@ struct pmd_internals {
>  	struct rte_eth_dev *dev;          /* Ethernet device. */
>  	char remote_iface[RTE_ETH_NAME_MAX_LEN]; /* Remote
> netdevice name */
>  	char name[RTE_ETH_NAME_MAX_LEN];  /* Internal Tap device
> name */
> +	int type;                         /* Type field - TUN|TAP */
>  	struct ether_addr eth_addr;       /* Mac address of the device port */
>  	struct ifreq remote_initial_flags;   /* Remote netdevice flags on init
> */
>  	int remote_if_index;              /* remote netdevice IF_INDEX */
> --
> 2.7.4

[dpdk-dev] [PATCH v4] net/tap: perform proto field update for tun only

[Ferruh Yigit]

From: Vipin Varghese <vipin.varghese@intel.com>

The TX function is shared between TAP and TUN PMD. Checking TUN-TAP
type field will ensure the TAP PMD will always have protocol field
as 0.

Signed-off-by: Vipin Varghese <vipin.varghese@intel.com>
Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
---
Cc: Ophir Munk <ophirmu@mellanox.com>
---
 drivers/net/tap/rte_eth_tap.c | 66 ++++++++++++++++++++++++++-----------------
 drivers/net/tap/rte_eth_tap.h |  9 ++++++
 2 files changed, 49 insertions(+), 26 deletions(-)

diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
index 01552fa4f..6bb7962bb 100644
--- a/drivers/net/tap/rte_eth_tap.c
+++ b/drivers/net/tap/rte_eth_tap.c
@@ -68,7 +68,6 @@ static const char *valid_arguments[] = {
 static int tap_unit;
 static unsigned int tun_unit;
 
-static int tap_type;
 static char tuntap_name[8];
 
 static volatile uint32_t tap_trigger;	/* Rx trigger */
@@ -116,7 +115,8 @@ tun_alloc(struct pmd_internals *pmd)
 	 * Do not set IFF_NO_PI as packet information header will be needed
 	 * to check if a received packet has been truncated.
 	 */
-	ifr.ifr_flags = (tap_type) ? IFF_TAP : IFF_TUN | IFF_POINTOPOINT;
+	ifr.ifr_flags = (pmd->type == ETH_TUNTAP_TYPE_TAP) ?
+		IFF_TAP : IFF_TUN | IFF_POINTOPOINT;
 	snprintf(ifr.ifr_name, IFNAMSIZ, "%s", pmd->name);
 
 	TAP_LOG(DEBUG, "ifr_name '%s'", ifr.ifr_name);
@@ -472,20 +472,22 @@ pmd_tx_burst(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
 		if (rte_pktmbuf_pkt_len(mbuf) > max_size)
 			break;
 
-		/*
-		 * TUN and TAP are created with IFF_NO_PI disabled.
-		 * For TUN PMD this mandatory as fields are used by
-		 * Kernel tun.c to determine whether its IP or non IP
-		 * packets.
-		 *
-		 * The logic fetches the first byte of data from mbuf.
-		 * compares whether its v4 or v6. If none matches default
-		 * value 0x00 is taken for protocol field.
-		 */
-		char *buff_data = rte_pktmbuf_mtod(seg, void *);
-		j = (*buff_data & 0xf0);
-		pi.proto = (j == 0x40) ? 0x0008 :
-				(j == 0x60) ? 0xdd86 : 0x00;
+		if (txq->type == ETH_TUNTAP_TYPE_TUN) {
+			/*
+			 * TUN and TAP are created with IFF_NO_PI disabled.
+			 * For TUN PMD this mandatory as fields are used by
+			 * Kernel tun.c to determine whether its IP or non IP
+			 * packets.
+			 *
+			 * The logic fetches the first byte of data from mbuf
+			 * then compares whether its v4 or v6. If first byte
+			 * is 4 or 6, then protocol field is updated.
+			 */
+			char *buff_data = rte_pktmbuf_mtod(seg, void *);
+			j = (*buff_data & 0xf0);
+			pi.proto = (j == 0x40) ? rte_cpu_to_be_16(ETHER_TYPE_IPv4) :
+				(j == 0x60) ? rte_cpu_to_be_16(ETHER_TYPE_IPv6) : 0x00;
+		}
 
 		iovecs[0].iov_base = &pi;
 		iovecs[0].iov_len = sizeof(pi);
@@ -928,6 +930,12 @@ tap_mac_set(struct rte_eth_dev *dev, struct ether_addr *mac_addr)
 	struct ifreq ifr;
 	int ret;
 
+	if (pmd->type == ETH_TUNTAP_TYPE_TUN) {
+		TAP_LOG(ERR, "%s: can't MAC address for TUN",
+			dev->device->name);
+		return -ENOTSUP;
+	}
+
 	if (is_zero_ether_addr(mac_addr)) {
 		TAP_LOG(ERR, "%s: can't set an empty MAC address",
 			dev->device->name);
@@ -1025,6 +1033,8 @@ tap_setup_queue(struct rte_eth_dev *dev,
 	tx->mtu = &dev->data->mtu;
 	rx->rxmode = &dev->data->dev_conf.rxmode;
 
+	tx->type = pmd->type;
+
 	return *fd;
 }
 
@@ -1342,7 +1352,8 @@ static const struct eth_dev_ops ops = {
 
 static int
 eth_dev_tap_create(struct rte_vdev_device *vdev, char *tap_name,
-		   char *remote_iface, struct ether_addr *mac_addr)
+		   char *remote_iface, struct ether_addr *mac_addr,
+		   enum rte_tuntap_type type)
 {
 	int numa_node = rte_socket_id();
 	struct rte_eth_dev *dev;
@@ -1364,6 +1375,7 @@ eth_dev_tap_create(struct rte_vdev_device *vdev, char *tap_name,
 	pmd = dev->data->dev_private;
 	pmd->dev = dev;
 	snprintf(pmd->name, sizeof(pmd->name), "%s", tap_name);
+	pmd->type = type;
 
 	pmd->ioctl_sock = socket(AF_INET, SOCK_DGRAM, 0);
 	if (pmd->ioctl_sock == -1) {
@@ -1400,7 +1412,7 @@ eth_dev_tap_create(struct rte_vdev_device *vdev, char *tap_name,
 		pmd->txq[i].fd = -1;
 	}
 
-	if (tap_type) {
+	if (pmd->type == ETH_TUNTAP_TYPE_TAP) {
 		if (is_zero_ether_addr(mac_addr))
 			eth_random_addr((uint8_t *)&pmd->eth_addr);
 		else
@@ -1423,7 +1435,7 @@ eth_dev_tap_create(struct rte_vdev_device *vdev, char *tap_name,
 	if (tap_ioctl(pmd, SIOCSIFMTU, &ifr, 1, LOCAL_AND_REMOTE) < 0)
 		goto error_exit;
 
-	if (tap_type) {
+	if (pmd->type == ETH_TUNTAP_TYPE_TAP) {
 		memset(&ifr, 0, sizeof(struct ifreq));
 		ifr.ifr_hwaddr.sa_family = AF_LOCAL;
 		rte_memcpy(ifr.ifr_hwaddr.sa_data, &pmd->eth_addr,
@@ -1642,7 +1654,6 @@ rte_pmd_tun_probe(struct rte_vdev_device *dev)
 	char tun_name[RTE_ETH_NAME_MAX_LEN];
 	char remote_iface[RTE_ETH_NAME_MAX_LEN];
 
-	tap_type = 0;
 	strcpy(tuntap_name, "TUN");
 
 	name = rte_vdev_device_name(dev);
@@ -1673,7 +1684,8 @@ rte_pmd_tun_probe(struct rte_vdev_device *dev)
 	TAP_LOG(NOTICE, "Initializing pmd_tun for %s as %s",
 		name, tun_name);
 
-	ret = eth_dev_tap_create(dev, tun_name, remote_iface, 0);
+	ret = eth_dev_tap_create(dev, tun_name, remote_iface, 0,
+		ETH_TUNTAP_TYPE_TUN);
 
 leave:
 	if (ret == -1) {
@@ -1700,7 +1712,6 @@ rte_pmd_tap_probe(struct rte_vdev_device *dev)
 	struct ether_addr user_mac = { .addr_bytes = {0} };
 	struct rte_eth_dev *eth_dev;
 
-	tap_type = 1;
 	strcpy(tuntap_name, "TAP");
 
 	name = rte_vdev_device_name(dev);
@@ -1762,7 +1773,8 @@ rte_pmd_tap_probe(struct rte_vdev_device *dev)
 	TAP_LOG(NOTICE, "Initializing pmd_tap for %s as %s",
 		name, tap_name);
 
-	ret = eth_dev_tap_create(dev, tap_name, remote_iface, &user_mac);
+	ret = eth_dev_tap_create(dev, tap_name, remote_iface, &user_mac,
+		ETH_TUNTAP_TYPE_TAP);
 
 leave:
 	if (ret == -1) {
@@ -1784,15 +1796,17 @@ rte_pmd_tap_remove(struct rte_vdev_device *dev)
 	struct pmd_internals *internals;
 	int i;
 
-	TAP_LOG(DEBUG, "Closing TUN/TAP Ethernet device on numa %u",
-		rte_socket_id());
-
 	/* find the ethdev entry */
 	eth_dev = rte_eth_dev_allocated(rte_vdev_device_name(dev));
 	if (!eth_dev)
 		return 0;
 
 	internals = eth_dev->data->dev_private;
+
+	TAP_LOG(DEBUG, "Closing %s Ethernet device on numa %u",
+		(internals->type == ETH_TUNTAP_TYPE_TAP) ? "TAP" : "TUN",
+		rte_socket_id());
+
 	if (internals->nlsk_fd) {
 		tap_flow_flush(eth_dev, NULL);
 		tap_flow_implicit_flush(internals, NULL);
diff --git a/drivers/net/tap/rte_eth_tap.h b/drivers/net/tap/rte_eth_tap.h
index c87a0b815..7b21d0d8b 100644
--- a/drivers/net/tap/rte_eth_tap.h
+++ b/drivers/net/tap/rte_eth_tap.h
@@ -23,6 +23,13 @@
 #define RTE_PMD_TAP_MAX_QUEUES	1
 #endif
 
+enum rte_tuntap_type {
+	ETH_TUNTAP_TYPE_UNKNOWN,
+	ETH_TUNTAP_TYPE_TUN,
+	ETH_TUNTAP_TYPE_TAP,
+	ETH_TUNTAP_TYPE_MAX,
+};
+
 struct pkt_stats {
 	uint64_t opackets;              /* Number of output packets */
 	uint64_t ipackets;              /* Number of input packets */
@@ -48,6 +55,7 @@ struct rx_queue {
 
 struct tx_queue {
 	int fd;
+	int type;                       /* Type field - TUN|TAP */
 	uint16_t *mtu;                  /* Pointer to MTU from dev_data */
 	uint16_t csum:1;                /* Enable checksum offloading */
 	struct pkt_stats stats;         /* Stats for this TX queue */
@@ -57,6 +65,7 @@ struct pmd_internals {
 	struct rte_eth_dev *dev;          /* Ethernet device. */
 	char remote_iface[RTE_ETH_NAME_MAX_LEN]; /* Remote netdevice name */
 	char name[RTE_ETH_NAME_MAX_LEN];  /* Internal Tap device name */
+	int type;                         /* Type field - TUN|TAP */
 	struct ether_addr eth_addr;       /* Mac address of the device port */
 	struct ifreq remote_initial_flags;   /* Remote netdevice flags on init */
 	int remote_if_index;              /* remote netdevice IF_INDEX */
-- 
2.14.3

Re: [dpdk-dev] [PATCH v4] net/tap: perform proto field update for tun only

[Ferruh Yigit]

On 5/22/2018 11:04 PM, Ferruh Yigit wrote:
> From: Vipin Varghese <vipin.varghese@intel.com>
> 
> The TX function is shared between TAP and TUN PMD. Checking TUN-TAP
> type field will ensure the TAP PMD will always have protocol field
> as 0.
> 
> Signed-off-by: Vipin Varghese <vipin.varghese@intel.com>
> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
> ---
> Cc: Ophir Munk <ophirmu@mellanox.com>

Self Nack. Forget to rename patch title, will send a new version soon.

[dpdk-dev] [PATCH v5] net/tap: fix proto field for tun

[Ferruh Yigit]

From: Vipin Varghese <vipin.varghese@intel.com>

The TX function is shared between TAP and TUN PMD. Checking TUN-TAP
type field will ensure the TAP PMD will always have protocol field
as 0.

Fixes: 204d026a3922 ("net/tap: support tun")
Cc: vipin.varghese@intel.com

Signed-off-by: Vipin Varghese <vipin.varghese@intel.com>
Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
---
Cc: Ophir Munk <ophirmu@mellanox.com>
---
 drivers/net/tap/rte_eth_tap.c | 66 ++++++++++++++++++++++++++-----------------
 drivers/net/tap/rte_eth_tap.h |  9 ++++++
 2 files changed, 49 insertions(+), 26 deletions(-)

diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
index 01552fa4f..6bb7962bb 100644
--- a/drivers/net/tap/rte_eth_tap.c
+++ b/drivers/net/tap/rte_eth_tap.c
@@ -68,7 +68,6 @@ static const char *valid_arguments[] = {
 static int tap_unit;
 static unsigned int tun_unit;
 
-static int tap_type;
 static char tuntap_name[8];
 
 static volatile uint32_t tap_trigger;	/* Rx trigger */
@@ -116,7 +115,8 @@ tun_alloc(struct pmd_internals *pmd)
 	 * Do not set IFF_NO_PI as packet information header will be needed
 	 * to check if a received packet has been truncated.
 	 */
-	ifr.ifr_flags = (tap_type) ? IFF_TAP : IFF_TUN | IFF_POINTOPOINT;
+	ifr.ifr_flags = (pmd->type == ETH_TUNTAP_TYPE_TAP) ?
+		IFF_TAP : IFF_TUN | IFF_POINTOPOINT;
 	snprintf(ifr.ifr_name, IFNAMSIZ, "%s", pmd->name);
 
 	TAP_LOG(DEBUG, "ifr_name '%s'", ifr.ifr_name);
@@ -472,20 +472,22 @@ pmd_tx_burst(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
 		if (rte_pktmbuf_pkt_len(mbuf) > max_size)
 			break;
 
-		/*
-		 * TUN and TAP are created with IFF_NO_PI disabled.
-		 * For TUN PMD this mandatory as fields are used by
-		 * Kernel tun.c to determine whether its IP or non IP
-		 * packets.
-		 *
-		 * The logic fetches the first byte of data from mbuf.
-		 * compares whether its v4 or v6. If none matches default
-		 * value 0x00 is taken for protocol field.
-		 */
-		char *buff_data = rte_pktmbuf_mtod(seg, void *);
-		j = (*buff_data & 0xf0);
-		pi.proto = (j == 0x40) ? 0x0008 :
-				(j == 0x60) ? 0xdd86 : 0x00;
+		if (txq->type == ETH_TUNTAP_TYPE_TUN) {
+			/*
+			 * TUN and TAP are created with IFF_NO_PI disabled.
+			 * For TUN PMD this mandatory as fields are used by
+			 * Kernel tun.c to determine whether its IP or non IP
+			 * packets.
+			 *
+			 * The logic fetches the first byte of data from mbuf
+			 * then compares whether its v4 or v6. If first byte
+			 * is 4 or 6, then protocol field is updated.
+			 */
+			char *buff_data = rte_pktmbuf_mtod(seg, void *);
+			j = (*buff_data & 0xf0);
+			pi.proto = (j == 0x40) ? rte_cpu_to_be_16(ETHER_TYPE_IPv4) :
+				(j == 0x60) ? rte_cpu_to_be_16(ETHER_TYPE_IPv6) : 0x00;
+		}
 
 		iovecs[0].iov_base = &pi;
 		iovecs[0].iov_len = sizeof(pi);
@@ -928,6 +930,12 @@ tap_mac_set(struct rte_eth_dev *dev, struct ether_addr *mac_addr)
 	struct ifreq ifr;
 	int ret;
 
+	if (pmd->type == ETH_TUNTAP_TYPE_TUN) {
+		TAP_LOG(ERR, "%s: can't MAC address for TUN",
+			dev->device->name);
+		return -ENOTSUP;
+	}
+
 	if (is_zero_ether_addr(mac_addr)) {
 		TAP_LOG(ERR, "%s: can't set an empty MAC address",
 			dev->device->name);
@@ -1025,6 +1033,8 @@ tap_setup_queue(struct rte_eth_dev *dev,
 	tx->mtu = &dev->data->mtu;
 	rx->rxmode = &dev->data->dev_conf.rxmode;
 
+	tx->type = pmd->type;
+
 	return *fd;
 }
 
@@ -1342,7 +1352,8 @@ static const struct eth_dev_ops ops = {
 
 static int
 eth_dev_tap_create(struct rte_vdev_device *vdev, char *tap_name,
-		   char *remote_iface, struct ether_addr *mac_addr)
+		   char *remote_iface, struct ether_addr *mac_addr,
+		   enum rte_tuntap_type type)
 {
 	int numa_node = rte_socket_id();
 	struct rte_eth_dev *dev;
@@ -1364,6 +1375,7 @@ eth_dev_tap_create(struct rte_vdev_device *vdev, char *tap_name,
 	pmd = dev->data->dev_private;
 	pmd->dev = dev;
 	snprintf(pmd->name, sizeof(pmd->name), "%s", tap_name);
+	pmd->type = type;
 
 	pmd->ioctl_sock = socket(AF_INET, SOCK_DGRAM, 0);
 	if (pmd->ioctl_sock == -1) {
@@ -1400,7 +1412,7 @@ eth_dev_tap_create(struct rte_vdev_device *vdev, char *tap_name,
 		pmd->txq[i].fd = -1;
 	}
 
-	if (tap_type) {
+	if (pmd->type == ETH_TUNTAP_TYPE_TAP) {
 		if (is_zero_ether_addr(mac_addr))
 			eth_random_addr((uint8_t *)&pmd->eth_addr);
 		else
@@ -1423,7 +1435,7 @@ eth_dev_tap_create(struct rte_vdev_device *vdev, char *tap_name,
 	if (tap_ioctl(pmd, SIOCSIFMTU, &ifr, 1, LOCAL_AND_REMOTE) < 0)
 		goto error_exit;
 
-	if (tap_type) {
+	if (pmd->type == ETH_TUNTAP_TYPE_TAP) {
 		memset(&ifr, 0, sizeof(struct ifreq));
 		ifr.ifr_hwaddr.sa_family = AF_LOCAL;
 		rte_memcpy(ifr.ifr_hwaddr.sa_data, &pmd->eth_addr,
@@ -1642,7 +1654,6 @@ rte_pmd_tun_probe(struct rte_vdev_device *dev)
 	char tun_name[RTE_ETH_NAME_MAX_LEN];
 	char remote_iface[RTE_ETH_NAME_MAX_LEN];
 
-	tap_type = 0;
 	strcpy(tuntap_name, "TUN");
 
 	name = rte_vdev_device_name(dev);
@@ -1673,7 +1684,8 @@ rte_pmd_tun_probe(struct rte_vdev_device *dev)
 	TAP_LOG(NOTICE, "Initializing pmd_tun for %s as %s",
 		name, tun_name);
 
-	ret = eth_dev_tap_create(dev, tun_name, remote_iface, 0);
+	ret = eth_dev_tap_create(dev, tun_name, remote_iface, 0,
+		ETH_TUNTAP_TYPE_TUN);
 
 leave:
 	if (ret == -1) {
@@ -1700,7 +1712,6 @@ rte_pmd_tap_probe(struct rte_vdev_device *dev)
 	struct ether_addr user_mac = { .addr_bytes = {0} };
 	struct rte_eth_dev *eth_dev;
 
-	tap_type = 1;
 	strcpy(tuntap_name, "TAP");
 
 	name = rte_vdev_device_name(dev);
@@ -1762,7 +1773,8 @@ rte_pmd_tap_probe(struct rte_vdev_device *dev)
 	TAP_LOG(NOTICE, "Initializing pmd_tap for %s as %s",
 		name, tap_name);
 
-	ret = eth_dev_tap_create(dev, tap_name, remote_iface, &user_mac);
+	ret = eth_dev_tap_create(dev, tap_name, remote_iface, &user_mac,
+		ETH_TUNTAP_TYPE_TAP);
 
 leave:
 	if (ret == -1) {
@@ -1784,15 +1796,17 @@ rte_pmd_tap_remove(struct rte_vdev_device *dev)
 	struct pmd_internals *internals;
 	int i;
 
-	TAP_LOG(DEBUG, "Closing TUN/TAP Ethernet device on numa %u",
-		rte_socket_id());
-
 	/* find the ethdev entry */
 	eth_dev = rte_eth_dev_allocated(rte_vdev_device_name(dev));
 	if (!eth_dev)
 		return 0;
 
 	internals = eth_dev->data->dev_private;
+
+	TAP_LOG(DEBUG, "Closing %s Ethernet device on numa %u",
+		(internals->type == ETH_TUNTAP_TYPE_TAP) ? "TAP" : "TUN",
+		rte_socket_id());
+
 	if (internals->nlsk_fd) {
 		tap_flow_flush(eth_dev, NULL);
 		tap_flow_implicit_flush(internals, NULL);
diff --git a/drivers/net/tap/rte_eth_tap.h b/drivers/net/tap/rte_eth_tap.h
index c87a0b815..7b21d0d8b 100644
--- a/drivers/net/tap/rte_eth_tap.h
+++ b/drivers/net/tap/rte_eth_tap.h
@@ -23,6 +23,13 @@
 #define RTE_PMD_TAP_MAX_QUEUES	1
 #endif
 
+enum rte_tuntap_type {
+	ETH_TUNTAP_TYPE_UNKNOWN,
+	ETH_TUNTAP_TYPE_TUN,
+	ETH_TUNTAP_TYPE_TAP,
+	ETH_TUNTAP_TYPE_MAX,
+};
+
 struct pkt_stats {
 	uint64_t opackets;              /* Number of output packets */
 	uint64_t ipackets;              /* Number of input packets */
@@ -48,6 +55,7 @@ struct rx_queue {
 
 struct tx_queue {
 	int fd;
+	int type;                       /* Type field - TUN|TAP */
 	uint16_t *mtu;                  /* Pointer to MTU from dev_data */
 	uint16_t csum:1;                /* Enable checksum offloading */
 	struct pkt_stats stats;         /* Stats for this TX queue */
@@ -57,6 +65,7 @@ struct pmd_internals {
 	struct rte_eth_dev *dev;          /* Ethernet device. */
 	char remote_iface[RTE_ETH_NAME_MAX_LEN]; /* Remote netdevice name */
 	char name[RTE_ETH_NAME_MAX_LEN];  /* Internal Tap device name */
+	int type;                         /* Type field - TUN|TAP */
 	struct ether_addr eth_addr;       /* Mac address of the device port */
 	struct ifreq remote_initial_flags;   /* Remote netdevice flags on init */
 	int remote_if_index;              /* remote netdevice IF_INDEX */
-- 
2.14.3

Re: [dpdk-dev] [PATCH v5] net/tap: fix proto field for tun

[Ferruh Yigit]

On 5/22/2018 11:09 PM, Ferruh Yigit wrote:
> From: Vipin Varghese <vipin.varghese@intel.com>
> 

Proto filed is fixed for tap, so title should be:
net/tap: fix proto field for tap

Will fix while merging.

> The TX function is shared between TAP and TUN PMD. Checking TUN-TAP
> type field will ensure the TAP PMD will always have protocol field
> as 0.
> 
> Fixes: 204d026a3922 ("net/tap: support tun")
> Cc: vipin.varghese@intel.com
> 
> Signed-off-by: Vipin Varghese <vipin.varghese@intel.com>
> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
> ---
> Cc: Ophir Munk <ophirmu@mellanox.com>

<...>

Re: [dpdk-dev] [PATCH v5] net/tap: fix proto field for tun

[Ferruh Yigit]

On 5/22/2018 11:19 PM, Ferruh Yigit wrote:
> On 5/22/2018 11:09 PM, Ferruh Yigit wrote:
>> From: Vipin Varghese <vipin.varghese@intel.com>
>>
> 
> Proto filed is fixed for tap, so title should be:
> net/tap: fix proto field for tap
> 
> Will fix while merging.
> 
>> The TX function is shared between TAP and TUN PMD. Checking TUN-TAP
>> type field will ensure the TAP PMD will always have protocol field
>> as 0.
>>
>> Fixes: 204d026a3922 ("net/tap: support tun")
>> Cc: vipin.varghese@intel.com
>>
>> Signed-off-by: Vipin Varghese <vipin.varghese@intel.com>
>> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>

Applied to dpdk-next-net/master, thanks.

Re: [dpdk-dev] [PATCH v3] net/tap: perform proto field update for tun only

[Varghese, Vipin]

HI Keith,

Thanks for pointing out, please find my answer and update below

<Snipped> 

> > +			/*
> > +			 * TUN and TAP are created with IFF_NO_PI disabled.
> > +			 * For TUN PMD this mandatory as fields are used by
> > +			 * Kernel tun.c to determine whether its IP or non IP
> > +			 * packets.
> > +			 *
> > +			 * The logic fetches the first byte of data from mbuf
> > +			 * then compares whether its v4 or v6. If first byte
> > +			 * is 4 or 6, then protocol field is updated.
> > +			 */
> > +			char *buff_data = rte_pktmbuf_mtod(seg, void *);
> > +			j = (*buff_data & 0xf0);
> > +			pi.proto = (j == 0x40) ? 0x0008 :
> > 				(j == 0x60) ? 0xdd86 : 0x00;
> 
> Warning Will Robinson: Magic numbers :-)
> 
> Can we use the correct values here ETHERTYPE_IPV6 and ETHERTYPE_IP and
> then use htons() on the values please.

Earlier I refrained from doing this assuming htnos comparison is done for each packet leading to extra cycles. So updated the comments with explanation for logic explanation and readability. 

But I am ok to in co-operate the idea of using MACRO with htnos. I have used ETHER_TYPE_IPv4 and ETHER_TYPE_IPv6 which does not require extra include. The changes are available in v4 version.

> 
> > +		}


<Snipped>

Re: [dpdk-dev] [PATCH v3] net/tap: perform proto field update for tun only

[Varghese, Vipin]

Hi Ophir,

Thanks for inputs, please find my answers inline to the mail

> -----Original Message-----
> From: Ophir Munk [mailto:ophirmu@mellanox.com]
> Sent: Tuesday, May 22, 2018 8:35 PM
> To: Varghese, Vipin <vipin.varghese@intel.com>; dev@dpdk.org; Yigit, Ferruh
> <ferruh.yigit@intel.com>; Wiles, Keith <keith.wiles@intel.com>
> Cc: Thomas Monjalon <thomas@monjalon.net>; Olga Shern
> <olgas@mellanox.com>
> Subject: RE: [PATCH v3] net/tap: perform proto field update for tun only
> 
> Hi,
> Overall it looks good.
> Please note a few more comments below.
> 
> > -----Original Message-----
> > From: Vipin Varghese [mailto:vipin.varghese@intel.com]
> > Sent: Tuesday, May 22, 2018 1:47 PM
> > To: dev@dpdk.org; ferruh.yigit@intel.com; keith.wiles@intel.com; Ophir
> > Munk <ophirmu@mellanox.com>
> > Cc: Thomas Monjalon <thomas@monjalon.net>; Olga Shern
> > <olgas@mellanox.com>; Vipin Varghese <vipin.varghese@intel.com>
> > Subject: [PATCH v3] net/tap: perform proto field update for tun only
> >
> > The TX function is shared between TAP and TUN PMD. Checking TUN-TAP
> > type field will ensure the TAP PMD will always have protocol field as 0.
> >
> > Signed-off-by: Vipin Varghese <vipin.varghese@intel.com>
> > Suggested-by: Ferruh Yigit <ferruh.yigit@intel.com>
> > ---
> 
> Shouldn't it be a fix commit?

The following code base is not merged with stable, dev or next-net branch. Hence do we 'fix commit' this?

> If so - please update the commit title, add Fixes: ... , Cc: stable@dpdk.org...
> 
> > Changes in V3:
> > - remove type field from rx struct - Ophir Munk
> > - add space for comment in struct - Ophir Munk
> > - pass type field into eth_dev_tap_create - Ophir Munk
> > - replace with enum value for type - Ophir Munk
> > - return as 'not supported' for mac_set - Vipin Varghese
> >
> > Changes in V2:
> > - updated in comment wording - Keith Wiles
> > - remove debug print from tx code - Keith Wiles
> > ---
> >  drivers/net/tap/rte_eth_tap.c | 61
> > +++++++++++++++++++++++++-------------
> > -----
> >  drivers/net/tap/rte_eth_tap.h |  9 +++++++
> >  2 files changed, 45 insertions(+), 25 deletions(-)
> >

<Snipped>

> >
> > +	if (pmd->type == ETH_TUNTAP_TYPE_TUN)
> > +		return -ENOTSUP;
> 
> 
> Can you please add a TAP_LOG(ERR, ...) to log this error similar to other error
> cases in this function?

Yes, I will add the change in v4 and share the same.

> 
> > +

<Snipped>