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 CAAB1A00E6 for ; Thu, 18 Apr 2019 14:30:56 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 7CB1D1B9E0; Thu, 18 Apr 2019 14:30:55 +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 3E58A1B140 for ; Thu, 18 Apr 2019 14:30:54 +0200 (CEST) Received: by mail-wm1-f68.google.com with SMTP id r186so6846292wmf.1 for ; Thu, 18 Apr 2019 05:30:54 -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=Y8j6rRWbS7puJRBYTJ23OAqWdoi655xhnXjBC5smshA=; b=Yn/C7VHjFhkajJBh28vl/ckGBrId21YyMatLDRWuCnS1FGl6/obxz0LOvmPQj4UfLJ /3oTPzEaAJxR//hSsGKKcHewNft47piYBKwbKDi/d2v8s5fKpqJNug1b3AbmYyTrgVlQ G/KJ1VRtjWAnC3JIXuSXYok6bKOe/moygt1Mdse83grg6+goGwdTdWNRUmh1h8FGZJAQ otoNxZ42XYXmgmMQcw2BCpXGaNmm9+nA568M5NGGQqoPI5irn2Y+uhArFBlagM8PdvN2 PCiXeLIKuKNWZFKhoDOUrubEWuNnmRTaLa04xtVoLOyVUnMesHl2vv8hh9C4DHXVwAAU n5/A== 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=Y8j6rRWbS7puJRBYTJ23OAqWdoi655xhnXjBC5smshA=; b=Gmd1Cn7klasUk5qlW5ZCfhKgs7Bu3xsMZyEdI5TwqdNMz80dwN68GJfmq3dtknkbLl KA+ANF16S0gC/K8kXem5X4fK6+A4MxEMtrBd3LhrBMwRWFl4EJAtPL4w5ULaAy/g0jAF hWyHH9uxHmoCi48THT+mcDWdlwCfwIyWT6LFSfFxpNy2xuhsKfvW8JKOWqXKhELkTwZA mspo6FYMUuA1LFkIbrnl113TDGqyvtQZCpwNZk6DV059HXrq7AzYwTpx3+BVn9U4I0Xn b9+ea9bDCpEmaCBvQizVuTFnggZXe4t2c5lGh0WB8ZojhU6oU4rHAvnEPMPgcrMtsgxv +ySg== X-Gm-Message-State: APjAAAWjmaiqr7MxwFT4wgf7dMqaOQKCD2xvEMLhG/fpb92+NId6UjHZ kemw+0su+t9tzcP5mauVNJNVFA== X-Google-Smtp-Source: APXvYqwKKZVJbHjuBAg8Ae1RqerpYB2p51wXiCWK0WbrwW+cMpDZY8Esbi2/LaPvglSOa3HlCELA7g== X-Received: by 2002:a1c:e1d4:: with SMTP id y203mr2929931wmg.142.1555590653926; Thu, 18 Apr 2019 05:30:53 -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 y9sm3600266wrn.18.2019.04.18.05.30.53 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 18 Apr 2019 05:30:53 -0700 (PDT) Date: Thu, 18 Apr 2019 14:30:51 +0200 From: Adrien Mazarguil To: Dekel Peled Cc: wenzhuo.lu@intel.com, jingjing.wu@intel.com, bernard.iremonger@intel.com, yskoh@mellanox.com, shahafs@mellanox.com, arybchenko@solarflare.com, dev@dpdk.org, orika@mellanox.com Message-ID: <20190418123051.GD4889@6wind.com> References: MIME-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Content-Disposition: inline In-Reply-To: Subject: Re: [dpdk-dev] [PATCH v4 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: <20190418123051.BAY2n24GymzgfP3M3ayix9ieEn0GM0QDt3_Boxeqv5w@z> On Wed, Apr 10, 2019 at 02:50:41PM +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 Almost there... Some changes were not made as discussed, please see below. > diff --git a/doc/guides/prog_guide/rte_flow.rst b/doc/guides/prog_guide/rte_flow.rst > index 0203f4f..fc234de 100644 > --- a/doc/guides/prog_guide/rte_flow.rst > +++ b/doc/guides/prog_guide/rte_flow.rst > @@ -2345,6 +2345,78 @@ Otherwise, RTE_FLOW_ERROR_TYPE_ACTION error will be returned. > | ``mac_addr`` | MAC address | > +--------------+---------------+ > > +Action: ``INC_TCP_SEQ`` > +^^^^^^^^^^^^^^^^^^^^^^^ > + > +Increase sequence number in the outermost TCP header. > + > +Using this action on non-matching traffic will result in undefined behavior, > +depending on PMD implementation. OK, "depending on PMD implementation" looks a bit redundant though. > + > +.. _table_rte_flow_action_inc_tcp_seq: > + > +.. table:: INC_TCP_SEQ > + > + +-----------+------------------------------------------+ > + | Field | Value | > + +===========+==========================================+ > + | ``value`` | Value to increase TCP sequence number by | > + +-----------+------------------------------------------+ Configuration object documentation needs updating since you changed its type and fields. These comments also apply to DEC_TCP_SEQ, INC_TCP_ACK and DEC_TCP_ACK. > diff --git a/lib/librte_ethdev/rte_flow.h b/lib/librte_ethdev/rte_flow.h > index c0fe879..e3f6210 100644 > --- a/lib/librte_ethdev/rte_flow.h > +++ b/lib/librte_ethdev/rte_flow.h > @@ -1651,6 +1651,46 @@ enum rte_flow_action_type { > * See struct rte_flow_action_set_mac. > */ > RTE_FLOW_ACTION_TYPE_SET_MAC_DST, > + > + /** > + * Increase sequence number in the outermost TCP header. > + * > + * Using this action on non-matching traffic will result in > + * undefined behavior, depending on PMD implementation. "depending on PMD implementation" also redundant here. > /* > + * @warning > + * @b EXPERIMENTAL: this union may change without prior notice > + * > + * General integer type, can be expanded as needed. > + */ > +union rte_flow_integer { > + rte_be32_t be32; > +}; You must include the extra fields you don't use (64/16/32/8 and le/be/host/signed variants as described in previous messages) to limit the risk of ABI breakage next time someone needs a field that isn't there yet. This object must be able to accommodate the largest supported integer and have the proper alignment constraints for any of these types, hence the union with all of them from the start. > + > +/** > + * @warning > + * @b EXPERIMENTAL: this structure may change without prior notice > + * > + * General struct, for use by actions that require a single integer value. > + */ > +struct rte_flow_general_action { > + union rte_flow_integer integer; > +}; Seriously, why can't actions take "union rte_flow_integer" directly? Besides "rte_flow_general_action" is vague as heck. Seems like you made this structure so it could be extended later, but forgot that doing so is not an option since ABIs are set in stone. You must make new APIs as exhaustive and restrict their scope as much as possible from the start because of that. Note if you don't want to use union rte_flow_integer directly, it's also fine to document your actions as taking (const rte_be32_t *) directly through their conf pointer. -- Adrien Mazarguil 6WIND