Re: [PATCH v2 01/12] ethdev: add API to query packet type filling info

[Adrien Mazarguil <adrien.mazarguil@6wind.com>]

On Fri, Jan 15, 2016 at 03:11:18PM +0000, Ananyev, Konstantin wrote:
> Hi lads,
> 
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Adrien Mazarguil
> > Sent: Friday, January 15, 2016 1:59 PM
> > To: Tan, Jianfeng
> > Cc: dev@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH v2 01/12] ethdev: add API to query packet type filling info
> > 
> > Hi Jianfeng, a few comments below.
> > 
> > On Fri, Jan 15, 2016 at 01:45:48PM +0800, Jianfeng Tan wrote:
> > > Add a new API rte_eth_dev_get_ptype_info to query wether/what packet type will
> > > be filled by given pmd rx burst function.
> > >
> > > Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>
> > > ---
> > >  lib/librte_ether/rte_ethdev.c | 20 ++++++++++++++++++++
> > >  lib/librte_ether/rte_ethdev.h | 27 +++++++++++++++++++++++++++
> > >  lib/librte_mbuf/rte_mbuf.h    |  6 ++++++
> > >  3 files changed, 53 insertions(+)
> > >
> > > diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
> > > index ed971b4..cd34f46 100644
> > > --- a/lib/librte_ether/rte_ethdev.c
> > > +++ b/lib/librte_ether/rte_ethdev.c
> > > @@ -1614,6 +1614,26 @@ 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[], int num)
> > > +{
> > > +	int ret, i, j;
> > > +	struct rte_eth_dev *dev;
> > > +	uint32_t all_ptypes[RTE_PTYPE_MAX_NUM];
> > > +
> > > +	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);
> > > +	ret = (*dev->dev_ops->dev_ptype_info_get)(dev, all_ptypes);
> > > +
> > > +	for (i = 0, j = 0; i < ret && j < num; ++i)
> > > +		if (all_ptypes[i] & ptype_mask)
> > > +			ptypes[j++] = all_ptypes[i];
> > > +
> > > +	return ret;
> > > +}
> > 
> > It's a good thing that the size of ptypes[] can be provided, but I think num
> > should be passed to the dev_ptype_info_get() callback as well.
> > 
> > If you really do not want to pass the size, you have to force the array type
> > size onto callbacks using a pointer to the array otherwise they look unsafe
> > (and are actually unsafe when not called from the rte_eth_dev wrapper),
> > something like this:
> > 
> >  int (*dev_ptype_info_get)(uint8_t port_id, uint32_t (*ptypes)[RTE_PTYPE_MAX_NUM]);
> > 
> > In which case you might as well drop the num argument from
> > rte_eth_dev_get_ptype_info() to use the same syntax. That way there is no
> > need to allocate a ptypes array on the stack twice.
> > 
> > But since people usually do not like this syntax, I think passing num and
> > letting callbacks check for overflow themselves on the user-provided ptypes
> > array directly is better. They have to return the total number of packet
> > types supported even when num is 0 (ptypes may be NULL in that case).
> > 
> > I understand the result needs to be temporarily stored somewhere for
> > filtering and for that purpose the entire size must be known in advance,
> > hence my previous suggestion for rte_eth_dev_get_ptype_info() to return
> > the total number of ptypes and providing a separate function for filtering
> > a ptypes array for applications that need it:
> > 
> >  /* Return remaining number entries in ptypes[] after filtering it
> >   * according to ptype_mask. */
> >  int rte_eth_dev_ptypes_filter(uint32_t ptype_mask, uint32_t ptypes[], int num);
> > 
> > Usage would be like:
> > 
> >  int ptypes_num = rte_eth_dev_get_ptype_info(42, NULL, 0);
> > 
> >  if (ptypes_num <= 0)
> >      goto err;
> > 
> >  uint32_t ptypes[ptypes_num];
> > 
> >  rte_eth_dev_get_ptype_info(42, ptypes, ptypes_num);
> >  ptypes_num = rte_eth_dev_ptypes_filter(RTE_PTYPE_INNER_L4_MASK, ptypes, ptypes_num);
> > 
> >  if (ptypes_num <= 0)
> >     goto err;
> > 
> >  /* process ptypes... */
> > 
> > What about this?
> 
> Actually while thinking about it, we probably can make:
> const uint32_t * (*dev_ptype_info_get)(uint8_t port_id); 
> So PMD return to ethdev layer a pointer to a constant array of supported ptypes,
> terminated by  RTE_PTYPE_UNKNOWN.   
>
> Then rte_eth_dev_get_ptype_info() will iterate over it, and fill array provided by the user.
> 
> all_pytpes = (*dev->dev_ops->dev_ptype_info_get)(dev);
> for (i = 0; all_ptypes[i] != RTE_PTYPE_UNKNOWN; i++) {
> if (all_ptypes[i] & ptype_mask) {
>           if (j < num)
>                ptypes[j] = all_ptypes[i];
>           j++;
> }
> return j;

Looks like a much simpler and better approach, that's the implementation we
need! (ignore my overengineered blob above)

-- 
Adrien Mazarguil
6WIND