From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <dev-bounces@dpdk.org>
Received: from dpdk.org (dpdk.org [92.243.14.124])
by dpdk.space (Postfix) with ESMTP id A1EB4A0679
for <public@inbox.dpdk.org>; Wed, 3 Apr 2019 14:49:28 +0200 (CEST)
Received: from [92.243.14.124] (localhost [127.0.0.1])
by dpdk.org (Postfix) with ESMTP id 99F891B3BC;
Wed, 3 Apr 2019 14:49:27 +0200 (CEST)
Received: from mail-wm1-f68.google.com (mail-wm1-f68.google.com
[209.85.128.68]) by dpdk.org (Postfix) with ESMTP id 5F78E1B3BB
for <dev@dpdk.org>; Wed, 3 Apr 2019 14:49:25 +0200 (CEST)
Received: by mail-wm1-f68.google.com with SMTP id n25so8040799wmk.4
for <dev@dpdk.org>; Wed, 03 Apr 2019 05:49:25 -0700 (PDT)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind.com; s=google;
h=date:from:to:cc:subject:message-id:references:mime-version
:content-disposition:in-reply-to;
bh=N1NrWRX3KBHVOaKwNyF4l/KN5oXHn6CSmtb3q8PNYSw=;
b=OttdbytELwKnusu5UVNr1pPpxOhCSAx6nQqSerp7wUYsBVp+y9VNWajs09Q3sXvW9l
7vlDiP+63ry6uWEWztOK1VaCLB2Mecon/ardqjWGgncOYDovOjLWQnz9mMFymwUiU01J
+vqTYxvdHRVrMDaJlb1y+2CHGl1aYzEhg/7C0KMkVQsuL4gEE9RpguwhTOLPQ3G33R0Y
elutTKVkcXcTmyyBaaw6RSzVFZ26vkYEG9oaHh3mV1B7MQX2P1wGMevlJMM8qa9KDOis
O7hmHALn+4md4yz/IlkW645vS5r+fouGuL0P85y53jPdtg3KZ79DbtNw4c6VarrGOJJX
dVUw==
X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed;
d=1e100.net; s=20161025;
h=x-gm-message-state:date:from:to:cc:subject:message-id:references
:mime-version:content-disposition:in-reply-to;
bh=N1NrWRX3KBHVOaKwNyF4l/KN5oXHn6CSmtb3q8PNYSw=;
b=uA72Xhwz6FWEvnTCA0Rsf5Qzjn/QsZF8zB9rTaEjnE4awGM0BR+kH4BYRnvoo+kkLR
zVPD+JXQDnmXzyPfWD8DI7JAo/sEVbWZdKwO2Q5Vz7HCBkJAB/hC1zLscngY+aqxqOPs
mV37XK6rXzgOJSshDn+z/oa1LzFyVWzPqvUyNAOSkqhnu3dftYm1Ado7Vf4wKqjMAZWs
6ZHi4JzTysB3wnl0u7EN8zWIY5HAuLq5SHj/vWZAlmwL/19r91o7U1zLxz/2DWARGsWO
MGt1SMF/+ffW4qOizDA06mQw6G5roYuEPv6VY/KkO1QIES00a6TfrG9ouUkMVqt034Gd
3XMw==
X-Gm-Message-State: APjAAAX7H9dBHRChkuvhCIICMOnnLJ00xFoKm1rGRagXSuD90F4pSNHo
6Fnh40pn4uf1sDvptjwE4nmraA==
X-Google-Smtp-Source: APXvYqzJ+Ae1WgCExZZetRfPCRxgOkRpKfYBgpQfSNrmtvNSYM+1tGrd7XmuZ5AXQcqWkRyb1Ydxpw==
X-Received: by 2002:a1c:a70f:: with SMTP id q15mr101473wme.28.1554295765037;
Wed, 03 Apr 2019 05:49:25 -0700 (PDT)
Received: from 6wind.com (host.78.145.23.62.rev.coltfrance.com. [62.23.145.78])
by smtp.gmail.com with ESMTPSA id s21sm18163951wmh.22.2019.04.03.05.49.23
(version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128);
Wed, 03 Apr 2019 05:49:23 -0700 (PDT)
Date: Wed, 3 Apr 2019 14:49:21 +0200
From: Adrien Mazarguil <adrien.mazarguil@6wind.com>
To: Dekel Peled <dekelp@mellanox.com>
Cc: "wenzhuo.lu@intel.com" <wenzhuo.lu@intel.com>,
"jingjing.wu@intel.com" <jingjing.wu@intel.com>,
"bernard.iremonger@intel.com" <bernard.iremonger@intel.com>,
Yongseok Koh <yskoh@mellanox.com>,
Shahaf Shuler <shahafs@mellanox.com>, "dev@dpdk.org" <dev@dpdk.org>,
Ori Kam <orika@mellanox.com>
Message-ID: <20190403124921.GR4889@6wind.com>
References: <1553177917-43297-1-git-send-email-dekelp@mellanox.com>
<1554218001-62012-2-git-send-email-dekelp@mellanox.com>
<20190403091432.GP4889@6wind.com>
<VI1PR05MB4224753382F523CD7C238A7EB6570@VI1PR05MB4224.eurprd05.prod.outlook.com>
MIME-Version: 1.0
Content-Type: text/plain; charset="UTF-8"
Content-Disposition: inline
In-Reply-To: <VI1PR05MB4224753382F523CD7C238A7EB6570@VI1PR05MB4224.eurprd05.prod.outlook.com>
Subject: Re: [dpdk-dev] [PATCH v2 1/3] ethdev: add actions to modify TCP
header fields
X-BeenThere: dev@dpdk.org
X-Mailman-Version: 2.1.15
Precedence: list
List-Id: DPDK patches and discussions <dev.dpdk.org>
List-Unsubscribe: <https://mails.dpdk.org/options/dev>,
<mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://mails.dpdk.org/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://mails.dpdk.org/listinfo/dev>,
<mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org
Sender: "dev" <dev-bounces@dpdk.org>
Message-ID: <20190403124921.JXv9lmnS1i9tnyTTycucFlozz7nuycD90p51Z894AZI@z>
On Wed, Apr 03, 2019 at 10:49:09AM +0000, Dekel Peled wrote:
> Thanks, PSB.
>
> > -----Original Message-----
> > From: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> > Sent: Wednesday, April 3, 2019 12:15 PM
> > To: Dekel Peled <dekelp@mellanox.com>
> > Cc: wenzhuo.lu@intel.com; jingjing.wu@intel.com;
> > bernard.iremonger@intel.com; Yongseok Koh <yskoh@mellanox.com>;
> > Shahaf Shuler <shahafs@mellanox.com>; dev@dpdk.org; Ori Kam
> > <orika@mellanox.com>
> > Subject: Re: [PATCH v2 1/3] ethdev: add actions to modify TCP header fields
> >
> > Hi Dekel,
> >
> > On Tue, Apr 02, 2019 at 06:13:19PM +0300, Dekel Peled wrote:
> > > Add actions:
> > > - INC_TCP_SEQ - Increase sequence number in the outermost TCP header.
> > > - DEC_TCP_SEQ - Decrease sequence number in the outermost TCP
> > header.
> > > - INC_TCP_ACK - Increase acknowledgment number in the outermost TCP
> > > header.
> > > - DEC_TCP_ACK - Decrease acknowledgment number in the outermost TCP
> > > header.
> > >
> > > Original work by Xiaoyu Min.
> > >
> > > Signed-off-by: Dekel Peled <dekelp@mellanox.com>
> > <snip>
> > > +Action: ``INC_TCP_SEQ``
> > > +^^^^^^^^^^^^^^^^^^^^^^^
> > > +
> > > +Increase sequence number in the outermost TCP header.
> > > +
> > > +If this action is used without a valid RTE_FLOW_ITEM_TYPE_TCP flow
> > > +pattern item, behavior is unspecified, depending on PMD
> > implementation.
> >
> > I still don't agree with the wording as it implies one must combine this action
> > with the TCP pattern item or else, while one should simply ensure the
> > presence of TCP traffic somehow. This may be done by a prior filtering rule.
> >
> > So here's a generic suggestion which could be used with pretty much all
> > modifying actions (other actions have the same problem and will have to be
> > fixed as well eventually):
> >
> > Using this action on non-matching traffic results in undefined behavior.
> >
> > This comment applies to all instances in this patch.
>
> I accept your suggestion, indeed the existing actions have the problematic condition.
> However I would like to currently leave this patch as-is for consistency.
> I will send a fix patch for next release, applying the updated text to all modify-header actions.
Please do it now as it's much more difficult to change an existing API
later (think deprecation notices and endless discussions); even seemingly
minor documentation issues like this one may affect applications.
> > <snip>
> > > +/**
> > > + * @warning
> > > + * @b EXPERIMENTAL: this structure may change without prior notice
> > > + *
> > > + * RTE_FLOW_ACTION_TYPE_INC_TCP_SEQ
> > > + * RTE_FLOW_ACTION_TYPE_DEC_TCP_SEQ
> > > + *
> > > + * Increase/Decrease outermost TCP sequence number */ struct
> > > +rte_flow_action_modify_tcp_seq {
> > > + rte_be32_t value; /**< Value to increase/decrease by. */ };
> > > +
> > > +/**
> > > + * @warning
> > > + * @b EXPERIMENTAL: this structure may change without prior notice
> > > + *
> > > + * RTE_FLOW_ACTION_TYPE_INC_TCP_ACK
> > > + * RTE_FLOW_ACTION_TYPE_DEC_TCP_ACK
> > > + *
> > > + * Increase/Decrease outermost TCP acknowledgment number.
> > > + */
> > > +struct rte_flow_action_modify_tcp_ack {
> > > + rte_be32_t value; /**< Value to increase/decrease by. */ };
> >
> > Thanks for adding experimental tags and comments, however you didn't
> > reply anything about using a single action, or at least a single structure for
> > add/sub/set? I'd like to hear your thoughts.
>
> It's either 2 actions with 1 parameters, or 1 action with 2 parameters.
> The current implementation is more straight-forward in my opinion.
I generally also prefer the one action per thing to do approach, but seeing
the kind of actions you're adding, I fear we'll soon end up with lots of
similar rte_flow_action_* structures modifying a single 32-bit value in some
way.
So for the same reasons as above, I think it's the right time to define a
shared structure to rule them all, or maybe even let users provide a
rte_be32_t/uint32_t/whatever pointer directly as a conf pointer (not
as straightforward to document though).
An object to rule them all would look something like that:
union rte_flow_integer {
rte_be64_t be64;
rte_le64_t le64;
uint64_t u64;
int64_t i64;
rte_be32_t be32;
rte_le32_t le32;
uint32_t u32;
int32_t i32;
uint8_t u8;
int8_t i8;
};
Then actions that need a single integer value only have to document which
field is relevant to them. How about that?
--
Adrien Mazarguil
6WIND