From: "Varghese, Vipin" <vipin.varghese@intel.com>
To: Ophir Munk <ophirmu@mellanox.com>, "dev@dpdk.org" <dev@dpdk.org>,
"pascal.mazon@6wind.com" <pascal.mazon@6wind.com>,
"Yigit, Ferruh" <ferruh.yigit@intel.com>,
Thomas Monjalon <thomas@monjalon.net>,
Olga Shern <olgas@mellanox.com>,
Shahaf Shuler <shahafs@mellanox.com>
Subject: Re: [dpdk-dev] [PATCH 1/2] net/tap: add tun support
Date: Fri, 13 Apr 2018 03:18:17 +0000 [thread overview]
Message-ID: <4C9E0AB70F954A408CC4ADDBF0F8FA7D4D1C8BAB@BGSMSX101.gar.corp.intel.com> (raw)
In-Reply-To: <HE1PR0501MB23140998CA897E2033F32B0AD1BC0@HE1PR0501MB2314.eurprd05.prod.outlook.com>
Hi Ophir,
Please find my answers inline to the queries.
> -----Original Message-----
> From: Ophir Munk [mailto:ophirmu@mellanox.com]
> Sent: Thursday, April 12, 2018 5:19 PM
> To: Varghese, Vipin <vipin.varghese@intel.com>; dev@dpdk.org;
> pascal.mazon@6wind.com; Yigit, Ferruh <ferruh.yigit@intel.com>; Thomas
> Monjalon <thomas@monjalon.net>; Olga Shern <olgas@mellanox.com>;
> Shahaf Shuler <shahafs@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@dpdk.org] On Behalf Of Vipin Varghese
> > Sent: Tuesday, April 03, 2018 12:38 AM
> > To: dev@dpdk.org; pascal.mazon@6wind.com; ferruh.yigit@intel.com
> > Cc: Vipin Varghese <vipin.varghese@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;
"""
next prev parent reply other threads:[~2018-04-13 3:18 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-02-20 23:12 [dpdk-dev] [PATCH v1] " Vipin Varghese
2018-02-22 12:22 ` [dpdk-dev] [dpdk-dev,v1] " Pascal Mazon
2018-02-26 11:01 ` Varghese, Vipin
2018-02-26 6:15 ` [dpdk-dev] [PATCH v2] " Vipin Varghese
2018-02-27 13:06 ` Pascal Mazon
2018-04-02 21:37 ` [dpdk-dev] [PATCH 1/2] " Vipin Varghese
2018-04-02 21:37 ` [dpdk-dev] [PATCH 2/2] net/tap: add tun log and documnetation Vipin Varghese
2018-04-03 8:27 ` Pascal Mazon
2018-04-03 10:05 ` Ferruh Yigit
2018-04-03 10:05 ` Ferruh Yigit
2018-04-03 10:53 ` Pascal Mazon
2018-04-06 17:11 ` [dpdk-dev] [PATCH 1/2] net/tap: add tun support Ferruh Yigit
2018-04-06 17:19 ` Ferruh Yigit
2018-04-12 11:49 ` Ophir Munk
2018-04-13 3:18 ` Varghese, Vipin [this message]
2018-04-20 21:49 ` Ophir Munk
2018-04-20 21:58 ` Ophir Munk
2018-04-21 15:09 ` Varghese, Vipin
2018-04-23 12:58 ` Varghese, Vipin
2018-04-23 15:42 ` Ferruh Yigit
2018-05-03 5:59 ` Varghese, Vipin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4C9E0AB70F954A408CC4ADDBF0F8FA7D4D1C8BAB@BGSMSX101.gar.corp.intel.com \
--to=vipin.varghese@intel.com \
--cc=dev@dpdk.org \
--cc=ferruh.yigit@intel.com \
--cc=olgas@mellanox.com \
--cc=ophirmu@mellanox.com \
--cc=pascal.mazon@6wind.com \
--cc=shahafs@mellanox.com \
--cc=thomas@monjalon.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).