From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by dpdk.space (Postfix) with ESMTP id C023CA0679 for ; Thu, 4 Apr 2019 15:26:02 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 3AB301B1F8; Thu, 4 Apr 2019 15:26:01 +0200 (CEST) Received: from mail-wr1-f68.google.com (mail-wr1-f68.google.com [209.85.221.68]) by dpdk.org (Postfix) with ESMTP id CB8691B1E8 for ; Thu, 4 Apr 2019 15:25:59 +0200 (CEST) Received: by mail-wr1-f68.google.com with SMTP id k11so3819447wro.5 for ; Thu, 04 Apr 2019 06:25:59 -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=7z821qO5XDRdz3mOwTNwlO/+TCEcee9a8OpmBU+Hv10=; b=MYDkAShBLywbstfTlqtmWoSyfW9UcGptn5Qwa4f/58z7GQyn2eZLR0rTjSSiWcUxop j+3u7QQpGcANvEPm+CmvPVximyl01lljD7xOaVJ8Iu3/Pw5tB4mV0hpv7Kn7jFRlS9/h 3cNRtGcVoqXkIwLMvYpxujZnkDaGe2dsA92232kGn6YNPTyBrxO0upCCSMNARBJdTt6r f5XY/DpHfH+K9pYkZUvryxemBfzik8uYLK08HP6UN8nxUaco5eEngzH7ofMMZC5i3SRD KES1g5YGe7MscwfOyq1Xv4QcvbTVDfFbKNuOUm/4KiM2C24y9kdOL9+fINSL+WqeRyw4 ulEw== 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=7z821qO5XDRdz3mOwTNwlO/+TCEcee9a8OpmBU+Hv10=; b=fBJkKW5IVNOAUrtL19g01pn7Swho5ELpnvCbN7W8wFaNu01U17W+jq+D92UZS8E2Bk aSUJN/EsUYM3f26s3S7OaNrB+aK3UMJd2yRgDLF9helrl5Tfel2ZMhUngZ+uOX+OuYgL d8DV0tXoZfBaaBer+qkUvLiyzAWcZx93BwJMjgh9w3hWCGbjRxozkCtv7v7NRa5GVaFe 6xJe/8a3Lk2TGsvFQ+luY9rplMCUQi9hWqPGeqjEwzVeposFS3QOT0F2BYccZ/udRWVt fn8dMP+1s1x9XxADmK4/xYjv0+jquzoae58qLJmP40yC8a4aRCxrrOGX2HK0QNMuCn6A Kh8w== X-Gm-Message-State: APjAAAVDdnEcQ5/Nkevc4YTQwpwDLSl6XoMn7/m2dH2C21q5kLzPZAmT ilmeQwOAuVwfUDSljAv6vyVJfQ== X-Google-Smtp-Source: APXvYqwX/RWeytnw6LfmEehytLm2RNKCKCnjWWpm4xMmaM9AbZNEJOoBGVFEX3jcICPhFHKvLMiNFQ== X-Received: by 2002:a5d:4a8d:: with SMTP id o13mr4026954wrq.209.1554384359536; Thu, 04 Apr 2019 06:25:59 -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 y17sm21536843wrh.60.2019.04.04.06.25.58 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 04 Apr 2019 06:25:58 -0700 (PDT) Date: Thu, 4 Apr 2019 15:25:56 +0200 From: Adrien Mazarguil To: Ori Kam Cc: Dekel Peled , "wenzhuo.lu@intel.com" , "jingjing.wu@intel.com" , "bernard.iremonger@intel.com" , Yongseok Koh , Shahaf Shuler , "dev@dpdk.org" Message-ID: <20190404132556.GS4889@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> <20190403124921.GR4889@6wind.com> MIME-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Content-Disposition: inline In-Reply-To: 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 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" Message-ID: <20190404132556.4LKmmQ5B9pG-AjdLcB6NFt831Vq3Mj9JnNc5sMWwesM@z> Hi Ori, (trimming message down a bit) On Thu, Apr 04, 2019 at 09:01:52AM +0000, Ori Kam wrote: > Hi Adrien, > > PSB > > > From: Adrien Mazarguil > > On Wed, Apr 03, 2019 at 10:49:09AM +0000, Dekel Peled wrote: > > > Thanks, PSB. > > > > From: Adrien Mazarguil > > > > 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. > > > I agree that changing API is not easy. This is why I think we should keep Dekel patch, > there is a number of API and consistency is very important. Also the PMD is based on the current > description that such command should fail. > > So lets keep it this way if you want to change all API then and only then this API should be changed. Wait, I'm not asking Delek to modify existing code/APIs right now, only to document these new actions properly from the start so we don't have to do it later (you even acknowledged it's more difficult that way). So I fail to understand why it's so important for their documentation to be consistent with unrelated and badly documented actions? Note the change I'm asking for at the API level doesn't affect PMD code, which remains free to put extra limitations (namely the presence of TCP pattern items). It's just that these limitations have nothing to do in the API itself. > > > 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? > > > > Like my previous comment. I understand your idea, but it has no huge advantage compared to the > suggested one by Dekel which also match all other API. > > Currently for each action we have a direct command, which is easy to understand by using your idea we break this concept. Yes, although not all actions have a configuration structure. Those that do indeed have a rte_flow_action_* counterpart, but it doesn't have to be unique, see RTE_FLOW_ITEM_GTP/GTPC/GTPU for instance. Likewise this patch adds struct rte_flow_action_modify_tcp_seq shared by RTE_FLOW_ACTION_TYPE_INC_TCP_SEQ and RTE_FLOW_ACTION_TYPE_DEC_TCP_SEQ although they lack a common prefix (inc_tcp/dec_tcp vs. modify_tcp). The type to use is covered by documentation and that's fine. So why not go a little further and share the exact same structure with RTE_FLOW_ACTION_TYPE_INC_TCP_ACK and RTE_FLOW_ACTION_TYPE_DEC_TCP_ACK? And while there, why not plan for subsequent actions that take a single integer value of some kind, because modifying existing APIs once upstream is complicated... See where I'm going? > There is no issue with having a large number of actions, it is even easer to read and document if each action is dedicated, > as you can also see from OVS. I'm actually fine with a large number of actions (rte_flow can support 2^31 unique actions). Not so much with a large number of identical configuration structures that only differ by name associated with them. This is what I'd like to avoid before it's too late. > So I vote to keep Dekel patch as is. I don't, I guess another vote is needed to decide :) -- Adrien Mazarguil 6WIND