From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <nelio.laranjeiro@6wind.com>
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 <dev@dpdk.org>; Tue,  5 Jan 2016 17:14:43 +0100 (CET)
Received: by mail-wm0-f45.google.com with SMTP id f206so29000965wmf.0
 for <dev@dpdk.org>; Tue, 05 Jan 2016 08:14:43 -0800 (PST)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed;
 d=6wind-com.20150623.gappssmtp.com; s=20150623;
 h=date:from:to:cc:subject:message-id:references:mime-version
 :content-type:content-disposition:content-transfer-encoding
 :in-reply-to:user-agent;
 bh=mEH1c1Wy/0PkAFtfEbwXXS8fS+pk4PDq+JtgUILT65M=;
 b=FY6ESrlS8dbsaa1Yt+RwZsj8xwa78eUeqcJv64th5avYlixFMGZgOnnTdLNEaEkccD
 p+KwAJBemcJWeaCkDwXUaEVBKejMsv4Ot5IHwR44t95kcEICCNYxRng5P19xiOIQ6L1j
 z1Sjty7QNF8+6OxlY7oyoZQRhdiQHZwuBSQpoyd5o8XJlDSZWeStxMil9jadAmzVYzdP
 nX6wh3eW4XgjBlI7KFBO+oGglRSXgLcUYUqd5aOsB55tE7au7sH2D6EmvvWu+yT1QYmL
 0IEffujrtxEblGOcXk3+kUbI4sj6qWpDucsrxbMqzZO61pADOYacIM742kGzRKxhv+6d
 /Ljw==
X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed;
 d=1e100.net; s=20130820;
 h=x-gm-message-state:date:from:to:cc:subject:message-id:references
 :mime-version:content-type:content-disposition
 :content-transfer-encoding:in-reply-to:user-agent;
 bh=mEH1c1Wy/0PkAFtfEbwXXS8fS+pk4PDq+JtgUILT65M=;
 b=N9p2UWp8i4jchfFGO3TYEiak2kS1xuEH9TGn0qpf082r5iYvOIeqmkYoh3pApLpkI2
 +JiLH4CEnzik8SpWugQ7/nSIAVTjfN9grSK4T7kQCOPN7palIPBx2g8BLDEruCk28HFr
 MgA/cZKa73kX2vK1Jh9xwUwRNo49ECeZ+92CLeq1iacOcIYvbMwKylZESz5KlOpv2A18
 9xAdTP/XgYOcrgG61ugAtGGNGFnbYGPyzs1FJ7NwFvrRtrKealmxHOxOYW9wF1TPQhJ0
 c7WhXHYti3W8ZALWststbBu1XJ4KURlBGS7WWgz80rP74eijRpMdXNqUIDhk8huFHeXe
 gBHA==
X-Gm-Message-State: ALoCoQlAsUMna4g5sTiMd82Qu+Jm43fR0iW/+EHv0Zy8pYVwBGDXdJ7FlaDnWye/ZrAJTjBNtC/gfm6CtZCQzYJsEaAS/EBffw==
X-Received: by 10.28.91.147 with SMTP id p141mr4920372wmb.19.1452010483163;
 Tue, 05 Jan 2016 08:14:43 -0800 (PST)
Received: from autoinstall.dev.6wind.com
 (guy78-3-82-239-227-177.fbx.proxad.net. [82.239.227.177])
 by smtp.gmail.com with ESMTPSA id ft4sm91502672wjb.37.2016.01.05.08.14.41
 (version=TLSv1/SSLv3 cipher=OTHER);
 Tue, 05 Jan 2016 08:14:42 -0800 (PST)
Date: Tue, 5 Jan 2016 17:14:23 +0100
From: =?iso-8859-1?Q?N=E9lio?= Laranjeiro <nelio.laranjeiro@6wind.com>
To: "Tan, Jianfeng" <jianfeng.tan@intel.com>
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-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <2601191342CEEE43887BDE71AB97725836AE1002@irsmsx105.ger.corp.intel.com>
User-Agent: Mutt/1.5.23 (2014-03-12)
Cc: "dev@dpdk.org" <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 <dev.dpdk.org>
List-Unsubscribe: <http://dpdk.org/ml/options/dev>,
 <mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://dpdk.org/ml/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <http://dpdk.org/ml/listinfo/dev>,
 <mailto:dev-request@dpdk.org?subject=subscribe>
X-List-Received-Date: Tue, 05 Jan 2016 16:14:43 -0000

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/if 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 method in
> >   my opinion).
> > 
> > - An additional argument with the number of entries in ptypes[], in which
> >   case rte_eth_dev_get_ptype_info() should return the number of entries 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

+1 for the second option.  But see below.

> > 
> > 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 type will
> > > be set by current rx burst function.
> > >
> > > Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>
> > > ---
> > >  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_ethdev.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 = 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 = &rte_eth_devices[port_id];
> > > +	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_ptype_info_get, -ENOTSUP);
> > > +	return (*dev->dev_ops->dev_ptype_info_get)(dev, ptype_mask, ptypes);
> > > +}
> > > +
> > >  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_ethdev.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 rte_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 Setup. */
> > >  	vlan_tpid_set_t            vlan_tpid_set;      /**< Outer VLAN TPID Setup. */
> > > @@ -2273,6 +2278,23 @@ extern void rte_eth_dev_info_get(uint8_t port_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 interested in
> > > + * @param ptypes
> > > + *   An array of packet types to be filled with
> > > + * @return
> > > + *   - (>=0) if successful. Indicate number of valid values in ptypes array.
> > > + *   - (-ENOTSUP) if hardware-assisted VLAN stripping not configured.
> > > + *   - (-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 contain 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 packet 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 header 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,

-- 
Nélio Laranjeiro
6WIND