From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by dpdk.org (Postfix) with ESMTP id 131661041 for ; Wed, 13 Sep 2017 04:08:23 +0200 (CEST) Received: from fmsmga006.fm.intel.com ([10.253.24.20]) by fmsmga103.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 12 Sep 2017 19:08:19 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.42,385,1500966000"; d="scan'208";a="151193982" Received: from dpdk15.sh.intel.com ([10.67.111.77]) by fmsmga006.fm.intel.com with ESMTP; 12 Sep 2017 19:08:17 -0700 Date: Wed, 13 Sep 2017 10:11:07 +0800 From: Jiayu Hu To: "Ananyev, Konstantin" Cc: "dev@dpdk.org" , "Kavanagh, Mark B" , "Tan, Jianfeng" Message-ID: <20170913021107.GA44293@dpdk15.sh.intel.com> References: <1504598270-60080-1-git-send-email-jiayu.hu@intel.com> <1505184211-36728-1-git-send-email-jiayu.hu@intel.com> <1505184211-36728-2-git-send-email-jiayu.hu@intel.com> <2601191342CEEE43887BDE71AB9772584F249E60@irsmsx105.ger.corp.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <2601191342CEEE43887BDE71AB9772584F249E60@irsmsx105.ger.corp.intel.com> User-Agent: Mutt/1.7.1 (2016-10-04) Subject: Re: [dpdk-dev] [PATCH v3 1/5] gso: add Generic Segmentation Offload API framework X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 13 Sep 2017 02:08:24 -0000 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 > > + > > +#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 > > +#include > > + > > +/* 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_ */