Re: [dpdk-dev] [EXT] Re: [PATCH] RFC: ethdev: add reassembly offload

[Akhil Goyal <gakhil@marvell.com>]

> On 8/23/21 1:02 PM, Akhil Goyal wrote:
> > Reassembly is a costly operation if it is done in
> > software, however, if it is offloaded to HW, it can
> > considerably save application cycles.
> > The operation becomes even more costlier if IP fragmants
> > are encrypted.
> >
> > To resolve above two issues, a new offload
> > DEV_RX_OFFLOAD_REASSEMBLY is introduced in ethdev for
> > devices which can attempt reassembly of packets in hardware.
> > rte_eth_dev_info is added with the reassembly capabilities
> > which a device can support.
> > Now, if IP fragments are encrypted, reassembly can also be
> > attempted while doing inline IPsec processing.
> > This is controlled by a flag in rte_security_ipsec_sa_options
> > to enable reassembly of encrypted IP fragments in the inline
> > path.
> >
> > 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 is
> > updated with an ol_flag PKT_RX_REASSEMBLY_INCOMPLETE and
> > mbuf is returned as is. Now application may decide the fate
> > of the packet to wait more for fragments to come or drop.
> >
> > Signed-off-by: Akhil Goyal <gakhil@marvell.com>
> 
> Is it IPv4 only or IPv6 as well? I guess IPv4 only to start
> with. If so, I think offload name should say so. See below.
> 
We can update spec for both and update capabilities for both.
See below.

> I'd say that the feature should be added to
> doc/guides/nics/features.rst

OK will update in next version
> 
> Do we really need RX_REASSEMBLY_INCOMPLETE if we provide
> buffered packets for incomplete reassembly anyway?
> I guess it is sufficient to cover simply reassembly case
> only in HW when there is no overlapping fragments etc.
> Everything else should be handled in SW anyway as without
> the offload support at all.
> 
In that case, application would need to again parse the packet
to check whether it is a fragment or not even when the reassembly
is not required. However, we would consider your suggestion in
implementation.

> > ---
> >  lib/ethdev/rte_ethdev.c     |  1 +
> >  lib/ethdev/rte_ethdev.h     | 18 +++++++++++++++++-
> >  lib/mbuf/rte_mbuf_core.h    |  3 ++-
> >  lib/security/rte_security.h | 10 ++++++++++
> >  4 files changed, 30 insertions(+), 2 deletions(-)
> >
> > diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
> > index 9d95cd11e1..1ab3a093cf 100644
> > --- a/lib/ethdev/rte_ethdev.c
> > +++ b/lib/ethdev/rte_ethdev.c
> > @@ -119,6 +119,7 @@ static const struct {
> >  	RTE_RX_OFFLOAD_BIT2STR(VLAN_FILTER),
> >  	RTE_RX_OFFLOAD_BIT2STR(VLAN_EXTEND),
> >  	RTE_RX_OFFLOAD_BIT2STR(JUMBO_FRAME),
> > +	RTE_RX_OFFLOAD_BIT2STR(REASSEMBLY),
> >  	RTE_RX_OFFLOAD_BIT2STR(SCATTER),
> >  	RTE_RX_OFFLOAD_BIT2STR(TIMESTAMP),
> >  	RTE_RX_OFFLOAD_BIT2STR(SECURITY),
> > diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
> > index d2b27c351f..e89a4dc1eb 100644
> > --- a/lib/ethdev/rte_ethdev.h
> > +++ b/lib/ethdev/rte_ethdev.h
> > @@ -1360,6 +1360,7 @@ struct rte_eth_conf {
> >  #define DEV_RX_OFFLOAD_VLAN_FILTER	0x00000200
> >  #define DEV_RX_OFFLOAD_VLAN_EXTEND	0x00000400
> >  #define DEV_RX_OFFLOAD_JUMBO_FRAME	0x00000800
> > +#define DEV_RX_OFFLOAD_REASSEMBLY	0x00001000
> 
> I think it should be:
> RTE_ETH_RX_OFFLOAD_IPV4_REASSEMBLY
> 
> i.e. have correct prefix similar to
> RTE_ETH_RX_OFFLOAD_BUFFER_SPLIT and mention IPv4.
> 
> If we'd like to cover IPv6 as well, it could be
> RTE_ETH_RX_OFFLOAD_IP_REASSEMBLY and have IPv4/6
> support bits in the offload capabilities below.

Intention is to update spec for both.
Will update the capabilities accordingly to have both IPv4 and IPv6.

> 
> >  #define DEV_RX_OFFLOAD_SCATTER		0x00002000
> >  /**
> >   * Timestamp is set by the driver in
> RTE_MBUF_DYNFIELD_TIMESTAMP_NAME
> > @@ -1477,6 +1478,20 @@ struct rte_eth_dev_portconf {
> >   */
> >  #define RTE_ETH_DEV_SWITCH_DOMAIN_ID_INVALID
> 	(UINT16_MAX)
> >
> > +/**
> > + * Reassembly capabilities that a device can support.
> > + * The device which can support reassembly offload should set
> > + * DEV_RX_OFFLOAD_REASSEMBLY
> > + */
> > +struct rte_eth_reass_capa {
> > +	/** Maximum time in ns that a fragment can wait for further
> fragments */
> > +	uint64_t reass_timeout;
> > +	/** Maximum number of fragments that device can reassemble */
> > +	uint16_t max_frags;
> > +	/** Reserved for future capabilities */
> > +	uint16_t reserved[3];
> > +};
> > +
> >  /**
> >   * Ethernet device associated switch information
> >   */
> > @@ -1582,8 +1597,9 @@ struct rte_eth_dev_info {
> >  	 * embedded managed interconnect/switch.
> >  	 */
> >  	struct rte_eth_switch_info switch_info;
> > +	/* Reassembly capabilities of a device for reassembly offload */
> > +	struct rte_eth_reass_capa reass_capa;
> >
> > -	uint64_t reserved_64s[2]; /**< Reserved for future fields */
> >  	void *reserved_ptrs[2];   /**< Reserved for future fields */
> >  };
> >
> > diff --git a/lib/mbuf/rte_mbuf_core.h b/lib/mbuf/rte_mbuf_core.h
> > index bb38d7f581..cea25c87f7 100644
> > --- a/lib/mbuf/rte_mbuf_core.h
> > +++ b/lib/mbuf/rte_mbuf_core.h
> > @@ -200,10 +200,11 @@ extern "C" {
> >  #define PKT_RX_OUTER_L4_CKSUM_BAD	(1ULL << 21)
> >  #define PKT_RX_OUTER_L4_CKSUM_GOOD	(1ULL << 22)
> >  #define PKT_RX_OUTER_L4_CKSUM_INVALID	((1ULL << 21) | (1ULL << 22))
> > +#define PKT_RX_REASSEMBLY_INCOMPLETE	(1ULL << 23)
> 
> In accordance with deprecation notice it should be
> RTE_MBUF_F_RX_REASSEMBLY_INCOMPLETE
> 
Ok will correct in next version.

> >
> >  /* add new RX flags here, don't forget to update PKT_FIRST_FREE */
> >
> > -#define PKT_FIRST_FREE (1ULL << 23)
> > +#define PKT_FIRST_FREE (1ULL << 24)
> >  #define PKT_LAST_FREE (1ULL << 40)
> >
> >  /* add new TX flags here, don't forget to update PKT_LAST_FREE  */
> > diff --git a/lib/security/rte_security.h b/lib/security/rte_security.h
> > index 88d31de0a6..364eeb5cd4 100644
> > --- a/lib/security/rte_security.h
> > +++ b/lib/security/rte_security.h
> > @@ -181,6 +181,16 @@ struct rte_security_ipsec_sa_options {
> >  	 * * 0: Disable per session security statistics collection for this SA.
> >  	 */
> >  	uint32_t stats : 1;
> > +
> > +	/** Enable reassembly on incoming packets.
> > +	 *
> > +	 * * 1: Enable driver to try reassembly of encrypted IP packets for
> > +	 *      this SA, if supported by the driver. This feature will work
> > +	 *      only if rx_offload DEV_RX_OFFLOAD_REASSEMBLY is set in
> > +	 *      inline ethernet device.
> 
> ethernet -> Ethernet
> 
> > +	 * * 0: Disable reassembly of packets (default).
> > +	 */
> > +	uint32_t reass_en : 1;
> >  };
> >
> >  /** IPSec security association direction */
> >