From mboxrd@z Thu Jan 1 00:00:00 1970 From: Adrien Mazarguil Subject: Re: [PATCH v2 1/3] ethdev: add actions to modify TCP header fields Date: Wed, 3 Apr 2019 14:49:21 +0200 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> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: "wenzhuo.lu@intel.com" , "jingjing.wu@intel.com" , "bernard.iremonger@intel.com" , Yongseok Koh , Shahaf Shuler , "dev@dpdk.org" , Ori Kam To: Dekel Peled Return-path: 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 ; Wed, 3 Apr 2019 14:49:25 +0200 (CEST) Received: by mail-wm1-f68.google.com with SMTP id n25so8040799wmk.4 for ; Wed, 03 Apr 2019 05:49:25 -0700 (PDT) Content-Disposition: inline In-Reply-To: List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On Wed, Apr 03, 2019 at 10:49:09AM +0000, Dekel Peled wrote: > Thanks, PSB. > > > -----Original Message----- > > From: Adrien Mazarguil > > Sent: Wednesday, April 3, 2019 12:15 PM > > To: Dekel Peled > > Cc: wenzhuo.lu@intel.com; jingjing.wu@intel.com; > > bernard.iremonger@intel.com; Yongseok Koh ; > > Shahaf Shuler ; dev@dpdk.org; Ori Kam > > > > 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 > > > > > +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. > > > > > +/** > > > + * @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