From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <konstantin.ananyev@intel.com>
Received: from mga06.intel.com (mga06.intel.com [134.134.136.31])
 by dpdk.org (Postfix) with ESMTP id 03471E5D
 for <dev@dpdk.org>; Thu,  7 Dec 2017 01:19:49 +0100 (CET)
Received: from orsmga002.jf.intel.com ([10.7.209.21])
 by orsmga104.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384;
 06 Dec 2017 16:19:48 -0800
X-ExtLoop1: 1
X-IronPort-AV: E=Sophos;i="5.45,370,1508828400"; d="scan'208";a="16099457"
Received: from irsmsx105.ger.corp.intel.com ([163.33.3.28])
 by orsmga002.jf.intel.com with ESMTP; 06 Dec 2017 16:19:48 -0800
Received: from irsmsx155.ger.corp.intel.com (163.33.192.3) by
 irsmsx105.ger.corp.intel.com (163.33.3.28) with Microsoft SMTP Server (TLS)
 id 14.3.319.2; Thu, 7 Dec 2017 00:19:47 +0000
Received: from irsmsx105.ger.corp.intel.com ([169.254.7.67]) by
 irsmsx155.ger.corp.intel.com ([169.254.14.169]) with mapi id 14.03.0319.002;
 Thu, 7 Dec 2017 00:19:46 +0000
From: "Ananyev, Konstantin" <konstantin.ananyev@intel.com>
To: Stephen Hemminger <stephen@networkplumber.org>, Ilya Matveychikov
 <matvejchikov@gmail.com>
CC: "dev@dpdk.org" <dev@dpdk.org>, "Hu, Jiayu" <jiayu.hu@intel.com>
Thread-Topic: [dpdk-dev] A question about GRO neighbor packet matching
Thread-Index: AQHTbprxvjvHdrmMEUK//NCFeevZt6M2njUAgAAHUgCAAE18AIAADhSg
Date: Thu, 7 Dec 2017 00:19:46 +0000
Message-ID: <2601191342CEEE43887BDE71AB9772585FAC57C1@irsmsx105.ger.corp.intel.com>
References: <4F9781B2-338C-4154-BDA1-BC24D1B2B689@gmail.com>
 <20171206101200.031afa39@shemminger-XPS-13-9360>
 <2111ED2C-DB90-4AE3-893E-2406EFE129AD@gmail.com>
 <20171206151532.3abaf2fb@xeon-e3>
In-Reply-To: <20171206151532.3abaf2fb@xeon-e3>
Accept-Language: en-IE, en-US
Content-Language: en-US
X-MS-Has-Attach: 
X-MS-TNEF-Correlator: 
x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiZDk1N2NiN2YtNzA4Zi00NmMxLTg1ZWYtMzlhZjkyOTFkYjNjIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX0lDIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE2LjUuOS4zIiwiVHJ1c3RlZExhYmVsSGFzaCI6Ikc5TWFxV29cL3dLMnZ0TDFPT29LYWNFMmZRVjhUXC9EZ1Urak13RlFGXC9xWTg9In0=
x-ctpclassification: CTP_IC
dlp-product: dlpe-windows
dlp-version: 11.0.0.116
dlp-reaction: no-action
x-originating-ip: [163.33.239.180]
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
Subject: Re: [dpdk-dev] A question about GRO neighbor packet matching
X-BeenThere: dev@dpdk.org
X-Mailman-Version: 2.1.15
Precedence: list
List-Id: DPDK patches and discussions <dev.dpdk.org>
List-Unsubscribe: <http://dpdk.org/ml/options/dev>,
 <mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://dpdk.org/ml/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <http://dpdk.org/ml/listinfo/dev>,
 <mailto:dev-request@dpdk.org?subject=subscribe>
X-List-Received-Date: Thu, 07 Dec 2017 00:19:50 -0000



> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Stephen Hemminger
> Sent: Wednesday, December 6, 2017 11:16 PM
> To: Ilya Matveychikov <matvejchikov@gmail.com>
> Cc: dev@dpdk.org; Hu, Jiayu <jiayu.hu@intel.com>
> Subject: Re: [dpdk-dev] A question about GRO neighbor packet matching
>=20
> On Wed, 6 Dec 2017 22:38:12 +0400
> Ilya Matveychikov <matvejchikov@gmail.com> wrote:
>=20
> > > On Dec 6, 2017, at 10:12 PM, Stephen Hemminger <stephen@networkplumbe=
r.org> wrote:
> > >
> > > On Wed, 6 Dec 2017 18:02:21 +0400
> > > Ilya Matveychikov <matvejchikov@gmail.com> wrote:
> > >
> > >> Hello all,
> > >>
> > >>
> > >> My question is about neighbor packet matching algorithm for TCP. Is =
it
> > >> correct to expect that IP packets should have continuous ID enumerat=
ion
> > >> (i.e. iph-next.id =3D iph-prev.id + 1)?
> > >
> > >
> > > No.
> > >
> > >> ~~~
> > >> lib/librte_gro/gro_tcp4.c:check_seq_option()
> > >> 	...
> > >> 	/* check if the two packets are neighbors */
> > >> 	tcp_dl0 =3D pkt0->pkt_len - pkt0->l2_len - pkt0->l3_len - tcp_hl0;
> > >> 	if ((sent_seq =3D=3D (item->sent_seq + tcp_dl0)) &&
> > >> 			(ip_id =3D=3D (item->ip_id + 1)))
> > >> 		/* append the new packet */
> > >> 		return 1;
> > >> 	else if (((sent_seq + tcp_dl) =3D=3D item->sent_seq) &&
> > >> 			((ip_id + item->nb_merged) =3D=3D item->ip_id))
> > >> 		/* pre-pend the new packet */
> > >> 		return -1;
> > >> 	else
> > >> 		return 0;
> > >> ~~~
> > >>
> > >> As per RFC791:
> > >>
> > >>  Identification:  16 bits
> > >>
> > >>    An identifying value assigned by the sender to aid in assembling =
the
> > >>    fragments of a datagram.
> > >
> > > The IP header id is meaningless in most TCP sessions.
> > > Good TCP implementations use PMTU discovery which sets the Don't Frag=
ment bit.
> > > With DF, the IP id is unused (since no fragmentation).
> > > Many implementations just send 0 since generating unique IP id requir=
es an
> > > atomic operation which is potential bottleneck.
> >
> > So, is my question correct and the code is wrong?
> >
>=20
> Yes. This code is wrong on several areas.
> * The ip_id on TCP flows is irrelevant.
> * packet should only be merged if TCP flags are the same.
>=20
>=20
> The author should look at Linux net/ipv4/tcp_offload.c

As I remember, linux GRO implementation *does* require that IP IDs
of the merging packets to be continuous.

net/ipv4/af_inet.c:
static struct sk_buff **inet_gro_receive(struct sk_buff **head,
					 struct sk_buff *skb)
{
  	...
 	id =3D ntohl(*(__be32 *)&iph->id);
	flush =3D (u16)((ntohl(*(__be32 *)iph) ^ skb_gro_len(skb)) | (id & ~IP_DF)=
);
	id >>=3D 16;

	...

	NAPI_GRO_CB(p)->flush_id =3D
			    ((u16)(ntohs(iph2->id) + NAPI_GRO_CB(p)->count) ^ id);
	NAPI_GRO_CB(p)->flush |=3D flush;
               ....

And then at net/ipv4/tcp_offload.c:
struct sk_buff **tcp_gro_receive(struct sk_buff **head, struct sk_buff *skb=
)
{
	...
	/* Include the IP ID check below from the inner most IP hdr */
	flush =3D NAPI_GRO_CB(p)->flush | NAPI_GRO_CB(p)->flush_id;
	...
	if (flush || skb_gro_receive(head, skb)) {
 	...

The reason why we do need to check that IP ID is continuous -=20
DPDK GRO library doesn't strip off IPv4 header, instead it has to merge the=
m into one.
If IP ID would be non-contiguous it is unclear which one should be to used.
By same reason packets with different IP/TCP options are not allowed.
So in that case GRO lib makes a decision that it isn't safe to merge these =
packets.
As I understand linux does pretty much the same.
Konstantin=20

=20