Re: [PATCH 1/5] lib: add Generic Segmentation Offload API framework

[Jiayu Hu <jiayu.hu@intel.com>]

Hi Konstantin,

On Wed, Aug 30, 2017 at 09:38:02AM +0800, Ananyev, Konstantin wrote:
> Hi Jiayu,
> 
> > 
> > Generic Segmentation Offload (GSO) is a SW technique to split large
> > packets into small ones. Akin to TSO, GSO enables applications to
> > operate on large packets, thus reducing per-packet processing overhead.
> > 
> > To enable more flexibility to applications, DPDK GSO is implemented
> > as a standalone library. Applications explicitly use the GSO library
> > to segment packets. This patch introduces the GSO API framework to DPDK.
> > 
> > The GSO library provides a segmentation API, rte_gso_segment(), for
> > applications. It splits an input packet into small ones in each
> > invocation. The GSO library refers to these small packets generated
> > by rte_gso_segment() as GSO segments. When all GSO segments are freed,
> > the input packet is freed automatically.
> > 
> > Signed-off-by: Jiayu Hu <jiayu.hu@intel.com>
> > Signed-off-by: Mark Kavanagh <mark.b.kavanagh@intel.com>
> > ---
> >  config/common_base                 |   5 ++
> >  lib/Makefile                       |   2 +
> >  lib/librte_gso/Makefile            |  49 ++++++++++++++++
> >  lib/librte_gso/rte_gso.c           |  47 ++++++++++++++++
> >  lib/librte_gso/rte_gso.h           | 111 +++++++++++++++++++++++++++++++++++++
> >  lib/librte_gso/rte_gso_version.map |   7 +++
> >  mk/rte.app.mk                      |   1 +
> >  7 files changed, 222 insertions(+)
> >  create mode 100644 lib/librte_gso/Makefile
> >  create mode 100644 lib/librte_gso/rte_gso.c
> >  create mode 100644 lib/librte_gso/rte_gso.h
> >  create mode 100644 lib/librte_gso/rte_gso_version.map
> > 
> > diff --git a/config/common_base b/config/common_base
> > index 5e97a08..603e340 100644
> > --- a/config/common_base
> > +++ b/config/common_base
> > @@ -652,6 +652,11 @@ CONFIG_RTE_LIBRTE_IP_FRAG_TBL_STAT=n
> >  CONFIG_RTE_LIBRTE_GRO=y
> > 
> >  #
> > +# Compile GSO library
> > +#
> > +CONFIG_RTE_LIBRTE_GSO=y
> > +
> > +#
> >  # Compile librte_meter
> >  #
> >  CONFIG_RTE_LIBRTE_METER=y
> > diff --git a/lib/Makefile b/lib/Makefile
> > index 86caba1..3d123f4 100644
> > --- a/lib/Makefile
> > +++ b/lib/Makefile
> > @@ -108,6 +108,8 @@ DIRS-$(CONFIG_RTE_LIBRTE_REORDER) += librte_reorder
> >  DEPDIRS-librte_reorder := librte_eal librte_mempool librte_mbuf
> >  DIRS-$(CONFIG_RTE_LIBRTE_PDUMP) += librte_pdump
> >  DEPDIRS-librte_pdump := librte_eal librte_mempool librte_mbuf librte_ether
> > +DIRS-$(CONFIG_RTE_LIBRTE_GSO) += librte_gso
> > +DEPDIRS-librte_gso := librte_eal librte_mbuf librte_ether librte_net
> > 
> >  ifeq ($(CONFIG_RTE_EXEC_ENV_LINUXAPP),y)
> >  DIRS-$(CONFIG_RTE_LIBRTE_KNI) += librte_kni
> > diff --git a/lib/librte_gso/Makefile b/lib/librte_gso/Makefile
> > new file mode 100644
> > index 0000000..aeaacbc
> > --- /dev/null
> > +++ b/lib/librte_gso/Makefile
> > @@ -0,0 +1,49 @@
> > +#   BSD LICENSE
> > +#
> > +#   Copyright(c) 2017 Intel Corporation. All rights reserved.
> > +#   All rights reserved.
> > +#
> > +#   Redistribution and use in source and binary forms, with or without
> > +#   modification, are permitted provided that the following conditions
> > +#   are met:
> > +#
> > +#     * Redistributions of source code must retain the above copyright
> > +#       notice, this list of conditions and the following disclaimer.
> > +#     * Redistributions in binary form must reproduce the above copyright
> > +#       notice, this list of conditions and the following disclaimer in
> > +#       the documentation and/or other materials provided with the
> > +#       distribution.
> > +#     * Neither the name of Intel Corporation nor the names of its
> > +#       contributors may be used to endorse or promote products derived
> > +#       from this software without specific prior written permission.
> > +#
> > +#   THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
> > +#   "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> > +#   LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
> > +#   A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
> > +#   OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
> > +#   SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
> > +#   LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
> > +#   DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
> > +#   THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> > +#   (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
> > +#   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> > +
> > +include $(RTE_SDK)/mk/rte.vars.mk
> > +
> > +# library name
> > +LIB = librte_gso.a
> > +
> > +CFLAGS += $(WERROR_FLAGS) -I$(SRCDIR) -O3
> > +
> > +EXPORT_MAP := rte_gso_version.map
> > +
> > +LIBABIVER := 1
> > +
> > +#source files
> > +SRCS-$(CONFIG_RTE_LIBRTE_GSO) += rte_gso.c
> > +
> > +# install this header file
> > +SYMLINK-$(CONFIG_RTE_LIBRTE_GSO)-include += rte_gso.h
> > +
> > +include $(RTE_SDK)/mk/rte.lib.mk
> > diff --git a/lib/librte_gso/rte_gso.c b/lib/librte_gso/rte_gso.c
> > new file mode 100644
> > index 0000000..b81afce
> > --- /dev/null
> > +++ b/lib/librte_gso/rte_gso.c
> > @@ -0,0 +1,47 @@
> > +/*-
> > + *   BSD LICENSE
> > + *
> > + *   Copyright(c) 2017 Intel Corporation. All rights reserved.
> > + *   All rights reserved.
> > + *
> > + *   Redistribution and use in source and binary forms, with or without
> > + *   modification, are permitted provided that the following conditions
> > + *   are met:
> > + *
> > + *     * Redistributions of source code must retain the above copyright
> > + *       notice, this list of conditions and the following disclaimer.
> > + *     * Redistributions in binary form must reproduce the above copyright
> > + *       notice, this list of conditions and the following disclaimer in
> > + *       the documentation and/or other materials provided with the
> > + *       distribution.
> > + *     * Neither the name of Intel Corporation nor the names of its
> > + *       contributors may be used to endorse or promote products derived
> > + *       from this software without specific prior written permission.
> > + *
> > + *   THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
> > + *   "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> > + *   LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
> > + *   A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
> > + *   OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
> > + *   SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
> > + *   LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
> > + *   DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
> > + *   THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> > + *   (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
> > + *   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> > + */
> > +
> > +#include "rte_gso.h"
> > +
> > +int
> > +rte_gso_segment(struct rte_mbuf *pkt,
> > +		struct rte_gso_ctx gso_ctx,
> > +		struct rte_mbuf **pkts_out,
> > +		uint16_t nb_pkts_out __rte_unused)
> > +{
> > +	if (pkt == NULL || pkts_out == NULL || gso_ctx.direct_pool ==
> > +			NULL || gso_ctx.indirect_pool == NULL)
> > +		return -EINVAL;
> > +
> > +	return 1;
> > +}
> > diff --git a/lib/librte_gso/rte_gso.h b/lib/librte_gso/rte_gso.h
> > new file mode 100644
> > index 0000000..5a8389a
> > --- /dev/null
> > +++ b/lib/librte_gso/rte_gso.h
> > @@ -0,0 +1,111 @@
> > +/*-
> > + *   BSD LICENSE
> > + *
> > + *   Copyright(c) 2017 Intel Corporation. All rights reserved.
> > + *   All rights reserved.
> > + *
> > + *   Redistribution and use in source and binary forms, with or without
> > + *   modification, are permitted provided that the following conditions
> > + *   are met:
> > + *
> > + *     * Redistributions of source code must retain the above copyright
> > + *       notice, this list of conditions and the following disclaimer.
> > + *     * Redistributions in binary form must reproduce the above copyright
> > + *       notice, this list of conditions and the following disclaimer in
> > + *       the documentation and/or other materials provided with the
> > + *       distribution.
> > + *     * Neither the name of Intel Corporation nor the names of its
> > + *       contributors may be used to endorse or promote products derived
> > + *       from this software without specific prior written permission.
> > + *
> > + *   THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
> > + *   "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> > + *   LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
> > + *   A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
> > + *   OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
> > + *   SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
> > + *   LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
> > + *   DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
> > + *   THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> > + *   (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
> > + *   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> > + */
> > +
> > +#ifndef _RTE_GSO_H_
> > +#define _RTE_GSO_H_
> > +
> > +/**
> > + * @file
> > + * Interface to GSO library
> > + */
> > +
> > +#ifdef __cplusplus
> > +extern "C" {
> > +#endif
> > +
> > +#include <stdint.h>
> > +#include <rte_mbuf.h>
> > +
> > +/**
> > + * GSO context structure.
> > + */
> > +struct rte_gso_ctx {
> > +	struct rte_mempool *direct_pool;
> > +	/**< MBUF pool for allocating direct buffers, which are used
> > +	 * to store packet headers for GSO segments.
> > +	 */
> > +	struct rte_mempool *indirect_pool;
> > +	/**< MBUF pool for allocating indirect buffers, which are used
> > +	 * to locate packet payloads for GSO segments. The indirect
> > +	 * buffer doesn't contain any data, but simply points to an
> > +	 * offset within the packet to segment.
> > +	 */
> > +	uint64_t gso_types;
> > +	/**< GSO types to perform */
> 
> Looking at the way it is used right now - there seems not much value in it...
> Why not to make it a mask of ptypes for which GSO should be perfomed?
> Let say for gso_ctx that supports only ip4/tcp it would be:
> gso_types = (RTE_PTYPE_L2_ETHER | RTE_PTYPE_L3_IPV4 | RTE_PTYPE_L4_TCP)
> and then in rte_gso_segment() we can perfom gso only on packets of requested ptype:
> 
> if ((pkt->packet_type & gso_ctx->gso_types) == pkt->packet_type) {
>    /* do segmentation */
> } else {
>   /* skip segmentation for that packet */
> }

Yes, you are right. It's unnecessary to define GRO type macros. We
can reuse ptype. I will change it in the next version.

> 
> > +	uint16_t gso_size;
> > +	/**< maximum size of a GSO segment, measured in bytes */
> 
> Is that MSS or MTU?

MSS. It's the max length of a complete packet, including packet headers.

> 
> > +};
> > +
> > +/**
> > + * Segmentation function, which supports processing of both single- and
> > + * multi- segment packets. rte_gso_segment() assumes the input packet
> > + * has correct checksums, and it doesn't process IP fragment packets.
> > + * Additionally, it assumes that 'pkts_out' is large enough to hold all GSO
> > + * segments.
> > + *
> > + * We refer to the packets that are segmented from the input packet as 'GSO
> > + * segments'. If the input packet is GSOed, its mbuf refcnt reduces by 1.
> > + * Therefore, when all GSO segments are freed, the input packet is freed
> > + * automatically. If the input packet doesn't match the criteria for GSO
> > + * (e.g. 'pkt's length is small and doesn't need segmentation), the packet
> > + * is skipped and this function returns 1. If the available memory space
> > + * in MBUF pools is insufficient, the packet is skipped and return -ENOMEM.
> > + *
> > + * @param pkt
> > + *  The packet mbuf to segment.
> > + * @param ctx
> > + *  GSO context object.
> > + * @param pkts_out
> > + *  Pointer array used to stores the mbuf addresses of GSO segments.
> > + *  Applications must ensure pkts_out is large enough to hold all GSO
> > + *  segments. If the memory space in pkts_out is insufficient, the input
> > + *  packet is skipped and return -EINVAL.
> > + * @param nb_pkts_out
> > + *  The max number of items that pkts_out can keep.
> > + *
> > + * @return
> > + *  - The number of GSO segments created on success.
> > + *  - Return 1 if no GSO is performed.
> 
> Wouldn't it be better to return number of elems filled in pkts_out[] on success?

Agree. I will change it.

> 
> > + *  - Return -ENOMEM if run out of memory in MBUF pools.
> > + *  - Return -EINVAL for invalid parameters.
> > + */
> > +int rte_gso_segment(struct rte_mbuf *pkt,
> > +		struct rte_gso_ctx ctx,
> > +		struct rte_mbuf **pkts_out,
> > +		uint16_t nb_pkts_out);
> > +
> > +#ifdef __cplusplus
> > +}
> > +#endif
> > +
> > +#endif /* _RTE_GSO_H_ */
> > diff --git a/lib/librte_gso/rte_gso_version.map b/lib/librte_gso/rte_gso_version.map
> > new file mode 100644
> > index 0000000..e1fd453
> > --- /dev/null
> > +++ b/lib/librte_gso/rte_gso_version.map
> > @@ -0,0 +1,7 @@
> > +DPDK_17.11 {
> > +	global:
> > +
> > +	rte_gso_segment;
> > +
> > +	local: *;
> > +};
> > diff --git a/mk/rte.app.mk b/mk/rte.app.mk
> > index c25fdd9..d4c9873 100644
> > --- a/mk/rte.app.mk
> > +++ b/mk/rte.app.mk
> > @@ -66,6 +66,7 @@ _LDLIBS-$(CONFIG_RTE_LIBRTE_PDUMP)          += -lrte_pdump
> >  _LDLIBS-$(CONFIG_RTE_LIBRTE_DISTRIBUTOR)    += -lrte_distributor
> >  _LDLIBS-$(CONFIG_RTE_LIBRTE_IP_FRAG)        += -lrte_ip_frag
> >  _LDLIBS-$(CONFIG_RTE_LIBRTE_GRO)            += -lrte_gro
> > +_LDLIBS-$(CONFIG_RTE_LIBRTE_GSO)            += -lrte_gso
> >  _LDLIBS-$(CONFIG_RTE_LIBRTE_METER)          += -lrte_meter
> >  _LDLIBS-$(CONFIG_RTE_LIBRTE_SCHED)          += -lrte_sched
> >  _LDLIBS-$(CONFIG_RTE_LIBRTE_LPM)            += -lrte_lpm
> > --
> > 2.7.4