From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id A25A8A00C4; Sat, 22 Jan 2022 08:38:54 +0100 (CET) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 1EBE042727; Sat, 22 Jan 2022 08:38:54 +0100 (CET) Received: from shelob.oktetlabs.ru (shelob.oktetlabs.ru [91.220.146.113]) by mails.dpdk.org (Postfix) with ESMTP id 5E78840040 for ; Sat, 22 Jan 2022 08:38:52 +0100 (CET) Received: from [192.168.90.214] (unknown [188.170.77.254]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by shelob.oktetlabs.ru (Postfix) with ESMTPSA id 2991938; Sat, 22 Jan 2022 10:38:51 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 shelob.oktetlabs.ru 2991938 DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=oktetlabs.ru; s=default; t=1642837131; bh=4J4BDn05pSsDNULnjiU9Bqsye5GjGKKhII77dX4GOGY=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=FdhjjGYAYn9rlAuMejk/BzVWrd+Q1VLLa2XgS89+gHds19/lzSPFM4MTYQQuf1IOU MlKN5vVccvIDiOOkPvgDJSCsY2jZo4ME3D7Ih1EiDiPJrsRFkgyb/AoHzZXcs6DI7p p7lwCA1CL6GHzeJDi9OgTPDptsxYlwMzWeeymC2w= Message-ID: Date: Sat, 22 Jan 2022 10:38:42 +0300 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.5.0 Subject: Re: [PATCH 1/8] ethdev: introduce IP reassembly offload Content-Language: en-US To: Akhil Goyal , dev@dpdk.org Cc: anoobj@marvell.com, radu.nicolau@intel.com, declan.doherty@intel.com, hemant.agrawal@nxp.com, matan@nvidia.com, konstantin.ananyev@intel.com, thomas@monjalon.net, ferruh.yigit@intel.com, olivier.matz@6wind.com, rosen.xu@intel.com References: <20210823100259.1619886-1-gakhil@marvell.com> <20220103150813.1694888-1-gakhil@marvell.com> <20220103150813.1694888-2-gakhil@marvell.com> From: Andrew Rybchenko In-Reply-To: <20220103150813.1694888-2-gakhil@marvell.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org On 1/3/22 18:08, Akhil Goyal wrote: > IP Reassembly is a costly operation if it is done in software. > The operation becomes even more costlier if IP fragmants are encrypted. > However, if it is offloaded to HW, it can considerably save application cycles. > > Hence, a new offload RTE_ETH_RX_OFFLOAD_IP_REASSEMBLY is introduced in > ethdev for devices which can attempt reassembly of packets in hardware. > rte_eth_dev_info is updated with the reassembly capabilities which a device > can support. Yes, reassembly is really complicated process taking possibility to have overlapping fragments, out-of-order etc. There are network attacks based on IP reassembly. Will it simply result in IP reassembly failure if no buffers are left for IP fragments? What will be reported in mbuf if some packets overlap? Just raw packets as is or reassembly result with holes? I think behavour should be specified/ > The resulting reassembled packet would be a typical segmented mbuf in > case of success. > > And if reassembly of fragments is failed or is incomplete (if fragments do > not come before the reass_timeout), the mbuf ol_flags can be updated. > This is updated in a subsequent patch. > > Signed-off-by: Akhil Goyal > --- > doc/guides/nics/features.rst | 12 ++++++++++++ > lib/ethdev/rte_ethdev.c | 1 + > lib/ethdev/rte_ethdev.h | 32 +++++++++++++++++++++++++++++++- > 3 files changed, 44 insertions(+), 1 deletion(-) > > diff --git a/doc/guides/nics/features.rst b/doc/guides/nics/features.rst > index 27be2d2576..1dfdee9602 100644 > --- a/doc/guides/nics/features.rst > +++ b/doc/guides/nics/features.rst > @@ -602,6 +602,18 @@ Supports inner packet L4 checksum. > ``tx_offload_capa,tx_queue_offload_capa:RTE_ETH_TX_OFFLOAD_OUTER_UDP_CKSUM``. > > > +.. _nic_features_ip_reassembly: > + > +IP reassembly > +------------- > + > +Supports IP reassembly in hardware. > + > +* **[uses] rte_eth_rxconf,rte_eth_rxmode**: ``offloads:RTE_ETH_RX_OFFLOAD_IP_REASSEMBLY``. Looking at the patch I see no changes and usage of rte_eth_rxconf and rte_eth_rxmode. It should be added here later if corresponding changes come in subsequent patches. > +* **[provides] mbuf**: ``mbuf.ol_flags:RTE_MBUF_F_RX_IP_REASSEMBLY_INCOMPLETE`` Same here. The flag is not defined yet. So, it must not be mentioned in the patch. . > +* **[provides] rte_eth_dev_info**: ``reass_capa``. > + > + > .. _nic_features_shared_rx_queue: > > Shared Rx queue > diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h > index fa299c8ad7..11427b2e4d 100644 > --- a/lib/ethdev/rte_ethdev.h > +++ b/lib/ethdev/rte_ethdev.h [snip] > @@ -1781,6 +1782,33 @@ enum rte_eth_representor_type { > RTE_ETH_REPRESENTOR_PF, /**< representor of Physical Function. */ > }; > > +/* Flag to offload IP reassembly for IPv4 packets. */ > +#define RTE_ETH_DEV_REASSEMBLY_F_IPV4 (RTE_BIT32(0)) > +/* Flag to offload IP reassembly for IPv6 packets. */ > +#define RTE_ETH_DEV_REASSEMBLY_F_IPV6 (RTE_BIT32(1)) > +/** > + * @warning > + * @b EXPERIMENTAL: this structure may change without prior notice. > + * > + * A structure used to set IP reassembly configuration. In the patch the structure is used to provide capabilities, not to set configuration. If you are going to use the same structure in capabilities and configuration, it could be handy, but really confusing since interpretation of fields should be different. As a bare minimum the difference must be specified in comments. Right now all fields makes sense in capabilities and configuration: maximum possible vs actual value, however, not everything could be really configurable and it will become confusing. It is really hard to discuss right now since the patch does not provide usage of the structure for the configuration. > + * > + * If RTE_ETH_RX_OFFLOAD_IP_REASSEMBLY flag is set in offloads field, > + * the PMD will attempt IP reassembly for the received packets as per > + * properties defined in this structure: > + * > + */ > +struct rte_eth_ip_reass_params { > + /** Maximum time in ms which PMD can wait for other fragments. */ > + uint32_t reass_timeout; Please, specify units. May be even in field name. E.g. reass_timeout_ms. > + /** Maximum number of fragments that can be reassembled. */ > + uint16_t max_frags; > + /** > + * Flags to enable reassembly of packet types - > + * RTE_ETH_DEV_REASSEMBLY_F_xxx. > + */ > + uint16_t flags; If it is just for packet types, I'd suggest to name the field more precise. Also it will avoid flags vs frags misreading. Just an idea. Up to you. > +}; > + > /** > * A structure used to retrieve the contextual information of > * an Ethernet device, such as the controlling driver of the > @@ -1841,8 +1869,10 @@ struct rte_eth_dev_info { > * embedded managed interconnect/switch. > */ > struct rte_eth_switch_info switch_info; > + /** IP reassembly offload capabilities that a device can support. */ > + struct rte_eth_ip_reass_params reass_capa; > > - uint64_t reserved_64s[2]; /**< Reserved for future fields */ > + uint64_t reserved_64s[1]; /**< Reserved for future fields */ > void *reserved_ptrs[2]; /**< Reserved for future fields */ > }; >