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