[dpdk-dev] [PATCH 1/2] net/tap: add tun support

[Varghese, Vipin]

Hi Ophir,

Please find my answers inline to the queries.

> -----Original Message-----
> From: Ophir Munk [mailto:ophirmu at mellanox.com]
> Sent: Thursday, April 12, 2018 5:19 PM
> To: Varghese, Vipin <vipin.varghese at intel.com>; dev at dpdk.org;
> pascal.mazon at 6wind.com; Yigit, Ferruh <ferruh.yigit at intel.com>; Thomas
> Monjalon <thomas at monjalon.net>; Olga Shern <olgas at mellanox.com>;
> Shahaf Shuler <shahafs at mellanox.com>
> Subject: RE: [dpdk-dev] [PATCH 1/2] net/tap: add tun support
> 
> Hi Vipin,
> This patch (adding TUN to TAP) has been Acked and accepted in next-net
> branch.
> I have some questions regarding the implementation (please find below).
> 
> > -----Original Message-----
> > From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Vipin Varghese
> > Sent: Tuesday, April 03, 2018 12:38 AM
> > To: dev at dpdk.org; pascal.mazon at 6wind.com; ferruh.yigit at intel.com
> > Cc: Vipin Varghese <vipin.varghese at intel.com>
> > Subject: [dpdk-dev] [PATCH 1/2] net/tap: add tun support
> >
> > The change adds functional TUN PMD logic to the existing TAP PMD.
> > TUN PMD can be initialized with 'net_tunX' where 'X' represents unique id.
> > PMD supports argument interface, while MAC address and remote are not
> > supported.
> >
> 
> [...]
> 
> >
> > +		/*
> > +		 * 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);
> > +		if (j & (0x40 | 0x60))
> > +			pi.proto = (j == 0x40) ? 0x0008 : 0xdd86;
> > +
> 
> 1. Accessing the first byte here assumes it is the first IP header byte (layer 3)
> which is correct for TUN.
> For TAP however the first byte belongs to Ethernet destination address
> (layer 2).
> Please explain how this logic will work for TAP.

Based on linux code base '/driver/net/tap.c' and '/driver/net/tun.c' from 3.13. to  4.16, 

Please find my observation below
1. File: tun.c, function: tun_get_user, check for 'tun->flags & TUN_TYPE_MASK' is done and if non ip is taken counter 'rx_dropped' is updated.
2. File: tap.c, there are no checks for 'tap->flags' for IFF_NO_PI in rx data path. Counter 'rx_dropped' is updated in 'tap_handle_frame'. 

Please find my reasoning below
1. First approach was to have separate function for tap and tun TX and RX. But this will introduce code duplication, hence reworked the code as above.
2. During my internal testing assigning dummy value for protocol field in TAP packets, did not show a difference in behaviour. May be there are some specific cases this failing. 

If there difference in behaviour, can please share the same?

> 
> 2. If the first TUN byte contains 0x2X (which is neither IPv4 nor IPv6) it will
> end up by setting ip.proto as 0xdd86.
> Please explain how this logic will work for non-IP packets in TUN

I see your point. You are correct about this. Thanks for pointing out, may I send correction for this as

"""
-		if (j & (0x40 | 0x60))
-			pi.proto = (j == 0x40) ? 0x0008 : 0xdd86;
+		pi.proto = (j == 0x40) ? 0x0008 : 
+					(j == 0x60) ? 0xdd86 :
+					0x00;
"""