[dpdk-dev] [PATCH v3 1/5] gso: add Generic Segmentation Offload API framework

[Ananyev, Konstantin]

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.  

> +		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.


> +
> +/**
> + * 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.

> +	 */
> +	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.

> +	/**< 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_ */