From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by dpdk.org (Postfix) with ESMTP id BE3585961 for ; Tue, 5 Jan 2016 17:51:11 +0100 (CET) Received: from orsmga002.jf.intel.com ([10.7.209.21]) by orsmga101.jf.intel.com with ESMTP; 05 Jan 2016 08:51:10 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.20,525,1444719600"; d="scan'208";a="884253295" Received: from irsmsx109.ger.corp.intel.com ([163.33.3.23]) by orsmga002.jf.intel.com with ESMTP; 05 Jan 2016 08:51:09 -0800 Received: from irsmsx105.ger.corp.intel.com ([169.254.7.104]) by IRSMSX109.ger.corp.intel.com ([163.33.3.23]) with mapi id 14.03.0248.002; Tue, 5 Jan 2016 16:50:31 +0000 From: "Ananyev, Konstantin" To: =?iso-8859-1?Q?N=E9lio_Laranjeiro?= , "Tan, Jianfeng" Thread-Topic: [dpdk-dev] [PATCH 01/12] ethdev: add API to query what/if packet type is set Thread-Index: AQHRQ9KoIugEgpKcLkGtpAxtk9NTop7rQSUAgAAxMWCAAa5MgIAABAAg Date: Tue, 5 Jan 2016 16:50:31 +0000 Message-ID: <2601191342CEEE43887BDE71AB97725836AE18E3@irsmsx105.ger.corp.intel.com> References: <1451544799-70776-1-git-send-email-jianfeng.tan@intel.com> <1451544799-70776-2-git-send-email-jianfeng.tan@intel.com> <20160104113814.GT3806@6wind.com> <2601191342CEEE43887BDE71AB97725836AE1002@irsmsx105.ger.corp.intel.com> <20160105161423.GE4712@autoinstall.dev.6wind.com> In-Reply-To: <20160105161423.GE4712@autoinstall.dev.6wind.com> Accept-Language: en-IE, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiMjYyN2I3ODMtZGQxMC00YWMxLTgzMjgtOTVlYmVkNDBjMzFkIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX0lDIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE1LjQuMTAuMTkiLCJUcnVzdGVkTGFiZWxIYXNoIjoiSHN4ZmFRRDNOU3dKeklZZ0V4K2R5d01HUm9xV1ZqclwvdG9nWUZEK0lScTA9In0= x-ctpclassification: CTP_IC x-originating-ip: [163.33.239.182] Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Cc: "dev@dpdk.org" Subject: Re: [dpdk-dev] [PATCH 01/12] ethdev: add API to query what/if packet type is set X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 05 Jan 2016 16:51:12 -0000 Hi Neilo, > -----Original Message----- > From: N=E9lio Laranjeiro [mailto:nelio.laranjeiro@6wind.com] > Sent: Tuesday, January 05, 2016 4:14 PM > To: Tan, Jianfeng > Cc: Adrien Mazarguil; Ananyev, Konstantin; dev@dpdk.org > Subject: Re: [dpdk-dev] [PATCH 01/12] ethdev: add API to query what/if pa= cket type is set >=20 > On Mon, Jan 04, 2016 at 02:36:14PM +0000, Ananyev, Konstantin wrote: > > > > > > > -----Original Message----- > > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Adrien Mazarguil > > > Sent: Monday, January 04, 2016 11:38 AM > > > To: Tan, Jianfeng > > > Cc: dev@dpdk.org > > > Subject: Re: [dpdk-dev] [PATCH 01/12] ethdev: add API to query what/i= f packet type is set > > > > > > I'm not sure about the usefulness of this new callback, but one issue= I see > > > with rte_eth_dev_get_ptype_info() is that determining the proper size= for > > > ptypes[] according to a mask is awkward. For instance suppose > > > RTE_PTYPE_L4_MASK is redefined to a different size at some point, the= caller > > > must dynamically adjust its ptypes[] array size to avoid a possible > > > overflow, just in case. > > > > > > I suggest one of these solutions: > > > > > > - A callback to query for a single type at once instead (easiest meth= od in > > > my opinion). > > > > > > - An additional argument with the number of entries in ptypes[], in w= hich > > > case rte_eth_dev_get_ptype_info() should return the number of entri= es that > > > would have been filled regardless, a bit like snprintf(). > > > > +1 for the second option. > > Also not sure you really need: RTE_PTYPE_*_MAX_NUM macros. > > Konstantin >=20 > +1 for the second option. But see below. >=20 > > > > > > On Thu, Dec 31, 2015 at 02:53:08PM +0800, Jianfeng Tan wrote: > > > > Add a new API rte_eth_dev_get_ptype_info to query what/if packet ty= pe will > > > > be set by current rx burst function. > > > > > > > > Signed-off-by: Jianfeng Tan > > > > --- > > > > lib/librte_ether/rte_ethdev.c | 12 ++++++++++++ > > > > lib/librte_ether/rte_ethdev.h | 22 ++++++++++++++++++++++ > > > > lib/librte_mbuf/rte_mbuf.h | 13 +++++++++++++ > > > > 3 files changed, 47 insertions(+) > > > > > > > > diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_e= thdev.c > > > > index ed971b4..1885374 100644 > > > > --- a/lib/librte_ether/rte_ethdev.c > > > > +++ b/lib/librte_ether/rte_ethdev.c > > > > @@ -1614,6 +1614,18 @@ rte_eth_dev_info_get(uint8_t port_id, struct= rte_eth_dev_info *dev_info) > > > > dev_info->driver_name =3D dev->data->drv_name; > > > > } > > > > > > > > +int > > > > +rte_eth_dev_get_ptype_info(uint8_t port_id, uint32_t ptype_mask, > > > > + uint32_t ptypes[]) > > > > +{ > > > > + struct rte_eth_dev *dev; > > > > + > > > > + RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV); > > > > + dev =3D &rte_eth_devices[port_id]; > > > > + RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_ptype_info_get, -ENOTS= UP); > > > > + return (*dev->dev_ops->dev_ptype_info_get)(dev, ptype_mask, ptype= s); > > > > +} > > > > + > > > > void > > > > rte_eth_macaddr_get(uint8_t port_id, struct ether_addr *mac_addr) > > > > { > > > > diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_e= thdev.h > > > > index bada8ad..e97b632 100644 > > > > --- a/lib/librte_ether/rte_ethdev.h > > > > +++ b/lib/librte_ether/rte_ethdev.h > > > > @@ -1021,6 +1021,10 @@ typedef void (*eth_dev_infos_get_t)(struct r= te_eth_dev *dev, > > > > struct rte_eth_dev_info *dev_info); > > > > /**< @internal Get specific informations of an Ethernet device. */ > > > > > > > > +typedef int (*eth_dev_ptype_info_get_t)(struct rte_eth_dev *dev, > > > > + uint32_t ptype_mask, uint32_t ptypes[]); > > > > +/**< @internal Get ptype info of eth_rx_burst_t. */ > > > > + > > > > typedef int (*eth_queue_start_t)(struct rte_eth_dev *dev, > > > > uint16_t queue_id); > > > > /**< @internal Start rx and tx of a queue of an Ethernet device. *= / > > > > @@ -1347,6 +1351,7 @@ struct eth_dev_ops { > > > > eth_queue_stats_mapping_set_t queue_stats_mapping_set; > > > > /**< Configure per queue stat counter mapping. */ > > > > eth_dev_infos_get_t dev_infos_get; /**< Get device info. *= / > > > > + eth_dev_ptype_info_get_t dev_ptype_info_get; /** Get ptype info= */ > > > > mtu_set_t mtu_set; /**< Set MTU. */ > > > > vlan_filter_set_t vlan_filter_set; /**< Filter VLAN Set= up. */ > > > > vlan_tpid_set_t vlan_tpid_set; /**< Outer VLAN TP= ID Setup. */ > > > > @@ -2273,6 +2278,23 @@ extern void rte_eth_dev_info_get(uint8_t por= t_id, > > > > struct rte_eth_dev_info *dev_info); > > > > > > > > /** > > > > + * Retrieve the contextual information of an Ethernet device. > > > > + * > > > > + * @param port_id > > > > + * The port identifier of the Ethernet device. > > > > + * @param ptype_mask > > > > + * A hint of what kind of packet type which the caller is intere= sted in > > > > + * @param ptypes > > > > + * An array of packet types to be filled with > > > > + * @return > > > > + * - (>=3D0) if successful. Indicate number of valid values in p= types array. > > > > + * - (-ENOTSUP) if hardware-assisted VLAN stripping not configur= ed. > > > > + * - (-ENODEV) if *port_id* invalid. > > > > + */ > > > > +extern int rte_eth_dev_get_ptype_info(uint8_t port_id, > > > > + uint32_t ptype_mask, uint32_t ptypes[]); > > > > + > > > > +/** > > > > * Retrieve the MTU of an Ethernet device. > > > > * > > > > * @param port_id > > > > diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.= h > > > > index f234ac9..21d4aa2 100644 > > > > --- a/lib/librte_mbuf/rte_mbuf.h > > > > +++ b/lib/librte_mbuf/rte_mbuf.h > > > > @@ -282,6 +282,8 @@ extern "C" { > > > > * It is used for outer packet for tunneling cases. > > > > */ > > > > #define RTE_PTYPE_L2_MASK 0x0000000f > > > > + > > > > +#define RTE_PTYPE_L2_MAX_NUM 4 > > > > /** > > > > * IP (Internet Protocol) version 4 packet type. > > > > * It is used for outer packet for tunneling cases, and does not c= ontain any > > > > @@ -349,6 +351,8 @@ extern "C" { > > > > * It is used for outer packet for tunneling cases. > > > > */ > > > > #define RTE_PTYPE_L3_MASK 0x000000f0 > > > > + > > > > +#define RTE_PTYPE_L3_MAX_NUM 6 > > > > /** > > > > * TCP (Transmission Control Protocol) packet type. > > > > * It is used for outer packet for tunneling cases. > > > > @@ -435,6 +439,8 @@ extern "C" { > > > > * It is used for outer packet for tunneling cases. > > > > */ > > > > #define RTE_PTYPE_L4_MASK 0x00000f00 > > > > + > > > > +#define RTE_PTYPE_L4_MAX_NUM 6 > > > > /** > > > > * IP (Internet Protocol) in IP (Internet Protocol) tunneling pack= et type. > > > > * > > > > @@ -508,6 +514,8 @@ extern "C" { > > > > * Mask of tunneling packet types. > > > > */ > > > > #define RTE_PTYPE_TUNNEL_MASK 0x0000f000 > > > > + > > > > +#define RTE_PTYPE_TUNNEL_MAX_NUM 6 > > > > /** > > > > * Ethernet packet type. > > > > * It is used for inner packet type only. > > > > @@ -527,6 +535,8 @@ extern "C" { > > > > * Mask of inner layer 2 packet types. > > > > */ > > > > #define RTE_PTYPE_INNER_L2_MASK 0x000f0000 > > > > + > > > > +#define RTE_PTYPE_INNER_L2_MAX_NUM 2 > > > > /** > > > > * IP (Internet Protocol) version 4 packet type. > > > > * It is used for inner packet only, and does not contain any head= er option. > > > > @@ -588,6 +598,8 @@ extern "C" { > > > > * Mask of inner layer 3 packet types. > > > > */ > > > > #define RTE_PTYPE_INNER_L3_MASK 0x00f00000 > > > > + > > > > +#define RTE_PTYPE_INNER_L3_MAX_NUM 6 > > > > /** > > > > * TCP (Transmission Control Protocol) packet type. > > > > * It is used for inner packet only. > > > > @@ -666,6 +678,7 @@ extern "C" { > > > > */ > > > > #define RTE_PTYPE_INNER_L4_MASK 0x0f000000 > > > > > > > > +#define RTE_PTYPE_INNER_L4_MAX_NUM 6 > > > > /** > > > > * Check if the (outer) L3 header is IPv4. To avoid comparing IPv4= types one by > > > > * one, bit 4 is selected to be used for IPv4 only. Then checking = bit 4 can >=20 > I think we miss a comment here in how those 2/6/4 values are chosen > because, according to the mask, I expect 16 possibilities but I get > less. It will help a lot anyone who needs to add a new type. >=20 > Extending the snprintf behavior above, it is best to remove the mask > argument altogether and have rte_eth_dev_get_ptype_info() return the > entire list every time. Applications need to iterate on the result in > any case. I think we'd better keep mask argument. In many cases upper layer only interested in some particular subset of all packet types that HW can recognise. Let say l3fwd only cares about RTE_PTYPE_L3_MASK, it is not interested in = L4, tunnelling packet types, etc. If caller needs to know all recognised ptypes, he can set mask=3D=3D-1, In that case all supported packet types will be returned. >=20 > rte_eth_dev_get_ptype_info(uint8_t port_id, uint32_t ptypes[], > size_t max_entries) >=20 >=20 >=20 > Another point, I have read the example patch (l3fwd) but I don't > understand why the PMD is not responsible for filling the packet type in > the MBUF (packet parsing is done by parse_packet_type()). Why the extra > computation? As I understand there are 3 possibilities now: 1. HW supports ptype recognition and SW ptype parsing is never done (--parse-ptype is not specified). 2. HW supports ptype recognition, but and SW ptype parsing is done anyway (--parse-ptype is specified). 3. HW doesn't support and ptype recognition, and and SW ptype parsing is do= ne (--parse-ptype is specified). I suppose the question is what for introduce '--parse-ptype' at all? My thought because of #2, so people can easily check what will be the perfo= rmance impact of SW parsing.=20 Konstantin >=20 > I see it more like an offload request (as checksum, etc...) and if the > NIC does not support it then the application does the necessary overload. >=20 > Best regards, >=20 > -- > N=E9lio Laranjeiro > 6WIND