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

["Wiles, Keith" <keith.wiles@intel.com>]

> 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