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

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

Hi Konstantin,

Thanks for your quick feedbacks. Replies are inline.

Thanks,
Jiayu

On Tue, Sep 12, 2017 at 06:36:41PM +0800, Ananyev, Konstantin wrote:
> Hi Jiayu,
> Few comments from be inline.
> Konstantin
> 
> > diff --git a/lib/librte_gso/rte_gso.c b/lib/librte_gso/rte_gso.c
> > new file mode 100644
> > index 0000000..dda50ee
> > --- /dev/null
> > +++ b/lib/librte_gso/rte_gso.c
> > @@ -0,0 +1,50 @@
> > +/*-
> > + *   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 <errno.h>
> > +
> > +#include "rte_gso.h"
> > +
> > +int
> > +rte_gso_segment(struct rte_mbuf *pkt,
> > +		struct rte_gso_ctx gso_ctx __rte_unused,
> 
> No need to pass parameter by value here.
> struct rte_gso_ctx *gso_ctx would do.
> Even better - const struct rte_gso_ctx *, in case it doesn't need to need
> to be updated inside that function.  

Yes, agree. I will use rte_gso_ctx *gso_ctx instead.

> 
> > +		struct rte_mbuf **pkts_out,
> > +		uint16_t nb_pkts_out)
> > +{
> > +	if (pkt == NULL || pkts_out == NULL || nb_pkts_out < 1)
> > +		return -EINVAL;
> > +
> > +	pkts_out[0] = pkt;
> > +
> > +	return 1;
> > +}
> > diff --git a/lib/librte_gso/rte_gso.h b/lib/librte_gso/rte_gso.h
> > new file mode 100644
> > index 0000000..db757d6
> > --- /dev/null
> > +++ b/lib/librte_gso/rte_gso.h
> > @@ -0,0 +1,133 @@
> > +/*-
> > + *   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 IP id flags for the IPv4 header */
> > +#define RTE_GSO_IPID_FIXED (1 << 0)
> > +/**< Use fixed IP ids for output GSO segments */
> > +#define RTE_GSO_IPID_INCREASE (1 << 1)
> > +/**< Use incremental IP ids for output GSO segments */
> 
> As values above are mutually exclusive, I think you don't need both flags.
> Just one seems enough.

Agree, I will remove RTE_GSO_IPID_INCREASE.

> 
> 
> > +
> > +/**
> > + * 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.
> > +	 */
> > +	uint32_t gso_types;
> > +	/**< packet types to perform GSO. For example, if applications
> > +	 * want to segment TCP/IPv4 packets, may set (RTE_PTYPE_L2_ETHER |
> > +	 * RTE_PTYPE_L3_IPV4 | RTE_PTYPE_L4_TCP) to gso_types.
> 
> 
> Actually after another thought - it probably should be no ptype mask, but mask
> of rte_ethdev DEV_TX_OFFLOAD_*_TSO flags that are used to advertise real HW TSO offloads.
> Let say for GSO that supports TSO over IPv4 it would be:
> PKT_TX_TCP_SEG | PKT_TX_IPV4.
> That would allow user to use GSO and TSO in a transparent way,
> plus ptype is not actually a proper bitmask, but a set of enums,
> so it not always possible to distinguish what ptype is supported just by bitmask.
> Sorry for causing confusion here.

Yes, agree. Reusing packet_type indeed introduces lots of macros to applications.
DEV_TX_OFFLOAD_*_TSO is a better choice, and it can also make HW offload and SW
offload consistent.

> 
> > +	 */
> > +	uint16_t gso_size;
> > +	/**< maximum size of an output GSO segment, including packet
> > +	 * header and payload, measured in bytes.
> > +	 */
> > +	uint8_t ipid_flag;
> 
> I'd suggest uint32_t flags (or even uint64_t).
> Who knows what extra flags we'll need in future here.

Make sense. I will use uint64_t.

> 
> > +	/**< flag to indicate GSO uses fixed or incremental IP ids for
> > +	 * IPv4 headers of output GSO segments.
> > +	 */
> > +};
> > +
> > +/**
> > + * Segmentation function, which supports processing of both single- and
> > + * multi- segment packets.
> > + *
> > + * Note that we refer to the packets that are segmented from the input
> > + * packet as 'GSO segments'. rte_gso_segment() assumes the input packet
> > + * has correct checksums, and it doesn't update checksums for output
> > + * GSO segments. Additionally, it doesn't process IP fragment packets.
> > + *
> > + * Each of the newly-created GSO segments is organized as a two-segment
> > + * MBUF, where the first segment is a standard MBUF, which stores a copy
> > + * of packet header, and the second is an indirect MBUF which points to
> > + * a section of data in the input packet. Since each GSO segment has
> > + * multiple MBUFs (i.e. 2 MBUFs), the driver of the interface which the
> > + * GSO segments are sent to should support to transmit multi-segment
> > + * packets.
> > + *
> > + * 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 memory space in pkts_out or MBUF pools is insufficient, this
> > + * function fails, and it returns (-1) * errno. Otherwise, GSO successes,
> > + * and this function returns the number of output GSO segments filled in
> > + * pkts_out.
> > + *
> > + * @param pkt
> > + *  The packet mbuf to segment.
> > + * @param ctx
> > + *  GSO context object.
> > + * @param pkts_out
> > + *  Pointer array used to store the MBUF addresses of output GSO
> > + *  segments, when rte_gso_segment() successes.
> > + * @param nb_pkts_out
> > + *  The max number of items that pkts_out can keep.
> > + *
> > + * @return
> > + *  - The number of GSO segments filled in pkts_out on success.
> > + *  - 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_ */