From mboxrd@z Thu Jan 1 00:00:00 1970 From: =?iso-8859-1?Q?N=E9lio?= Laranjeiro Subject: Re: [PATCH 01/12] ethdev: add API to query what/if packet type is set Date: Tue, 5 Jan 2016 17:14:23 +0100 Message-ID: <20160105161423.GE4712@autoinstall.dev.6wind.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> Mime-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Transfer-Encoding: quoted-printable Cc: "dev@dpdk.org" To: "Tan, Jianfeng" Return-path: Received: from mail-wm0-f45.google.com (mail-wm0-f45.google.com [74.125.82.45]) by dpdk.org (Postfix) with ESMTP id 6BF3293A6 for ; Tue, 5 Jan 2016 17:14:43 +0100 (CET) Received: by mail-wm0-f45.google.com with SMTP id f206so29000965wmf.0 for ; Tue, 05 Jan 2016 08:14:43 -0800 (PST) Content-Disposition: inline In-Reply-To: <2601191342CEEE43887BDE71AB97725836AE1002@irsmsx105.ger.corp.intel.com> List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On Mon, Jan 04, 2016 at 02:36:14PM +0000, Ananyev, Konstantin wrote: >=20 >=20 > > -----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 > >=20 > > 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. > >=20 > > I suggest one of these solutions: > >=20 > > - A callback to query for a single type at once instead (easiest meth= od in > > my opinion). > >=20 > > - 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(). >=20 > +1 for the second option. > Also not sure you really need: RTE_PTYPE_*_MAX_NUM macros. > Konstantin +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 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. 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. rte_eth_dev_get_ptype_info(uint8_t port_id, uint32_t ptypes[], size_t max_entries) 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? 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. Best regards, --=20 N=E9lio Laranjeiro 6WIND