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

2016-02-25 Thread Tan, Jianfeng 


On 2/25/2016 7:17 PM, Ananyev, Konstantin wrote:
 +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 = _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;
>>> I think it needs to be something like:
>>>
>>> j = 0;
>>> for (i = 0, j = 0; i < ret; ++i) {
>>>if (all_ptypes[i] & ptype_mask) {
>>> if (j < num)
>>>  ptypes[j] = all_ptypes[i];
>>> j++;
>>>  }
>>> }
>>>
>>> return j;
>>>
>>> Konstantin
>>>
>> You are right, my previous code is wrong.
>> But I have a concern about your code above: under the condition that the
>> caller does not provide big enough array to store adequate ptypes, it
>> has no way to return the not-enough-memory message back to caller.
>>
>> So under that condition, how about we just return -ENOMEM?
>>
> As I remember, the agreement was - we don't return an -ENOMEM in that case.
> What we do return - number of entries in ptypes[] that would be required to
> store all adequate ptypes (similar to what snprinf() does).
> So the user can do something like that (if he needs to):
>
> num = rte_eth_dev_get_ptype_info(port,  ptype_mask, NULL, 0);
> if (num < 0) {/*error handling*/}
> ptypes = alloca(num * ptypes[0]);
> n = rte_eth_dev_get_ptype_info(port,  ptype_mask, ptypes, num);
> ...
>
> Konstantin
>

Oh, yes. Sorry, I previously misunderstood your code.

But I still have a concern of above way that this APIs should be called 
two times. I suggest to use a way, like strdup, callee to malloc, caller 
to free. I send out v3 right now, please have a look at if it's OK.

Thanks,
Jianfeng

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

2016-02-25 Thread Tan, Jianfeng 
Hi Konstantin,

On 1/15/2016 11:03 PM, Ananyev, Konstantin wrote:
>
>> -Original Message-
>> From: Tan, Jianfeng
>> Sent: Friday, January 15, 2016 5:46 AM
>> To: dev at dpdk.org
>> Cc: Zhang, Helin; Ananyev, Konstantin; Tan, Jianfeng
>> Subject: [PATCH v2 01/12] ethdev: add API to query packet type filling info
>>
>> 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 
>> ---
>>   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 = _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;
> I think it needs to be something like:
>
> j = 0;
> for (i = 0, j = 0; i < ret; ++i) {
>   if (all_ptypes[i] & ptype_mask) {
>if (j < num)
> ptypes[j] = all_ptypes[i];
>j++;
> }
> }
>
> return j;
>
> Konstantin
>

You are right, my previous code is wrong.
But I have a concern about your code above: under the condition that the 
caller does not provide big enough array to store adequate ptypes, it 
has no way to return the not-enough-memory message back to caller.

So under that condition, how about we just return -ENOMEM?

Thanks,
Jianfeng

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

2016-02-25 Thread Ananyev, Konstantin 

> >> +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 = _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;
> > I think it needs to be something like:
> >
> > j = 0;
> > for (i = 0, j = 0; i < ret; ++i) {
> >   if (all_ptypes[i] & ptype_mask) {
> >if (j < num)
> > ptypes[j] = all_ptypes[i];
> >j++;
> > }
> > }
> >
> > return j;
> >
> > Konstantin
> >
> 
> You are right, my previous code is wrong.
> But I have a concern about your code above: under the condition that the
> caller does not provide big enough array to store adequate ptypes, it
> has no way to return the not-enough-memory message back to caller.
> 
> So under that condition, how about we just return -ENOMEM?
> 

As I remember, the agreement was - we don't return an -ENOMEM in that case.
What we do return - number of entries in ptypes[] that would be required to 
store all adequate ptypes (similar to what snprinf() does).
So the user can do something like that (if he needs to): 

num = rte_eth_dev_get_ptype_info(port,  ptype_mask, NULL, 0);
if (num < 0) {/*error handling*/}
ptypes = alloca(num * ptypes[0]);
n = rte_eth_dev_get_ptype_info(port,  ptype_mask, ptypes, num);
...

Konstantin

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

2016-01-15 Thread Adrien Mazarguil 
On Fri, Jan 15, 2016 at 03:11:18PM +, Ananyev, Konstantin wrote:
> Hi lads,
> 
> > -Original Message-
> > From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Adrien Mazarguil
> > Sent: Friday, January 15, 2016 1:59 PM
> > To: Tan, Jianfeng
> > Cc: dev at 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 
> > > ---
> > >  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 = _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

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

2016-01-15 Thread Ananyev, Konstantin 
Hi lads,

> -Original Message-
> From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Adrien Mazarguil
> Sent: Friday, January 15, 2016 1:59 PM
> To: Tan, Jianfeng
> Cc: dev at 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 
> > ---
> >  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 = _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;

Konstantin

> 
> > +
> >  void
> >  rte_eth_macaddr_get(uint8_t port_id, struct ether_addr *mac_addr)
> >

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

2016-01-15 Thread Ananyev, Konstantin 


> -Original Message-
> From: Tan, Jianfeng
> Sent: Friday, January 15, 2016 5:46 AM
> To: dev at dpdk.org
> Cc: Zhang, Helin; Ananyev, Konstantin; Tan, Jianfeng
> Subject: [PATCH v2 01/12] ethdev: add API to query packet type filling info
> 
> 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 
> ---
>  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 = _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;

I think it needs to be something like:

j = 0;
for (i = 0, j = 0; i < ret; ++i) {
 if (all_ptypes[i] & ptype_mask) {
  if (j < num)
   ptypes[j] = all_ptypes[i];
  j++;
   }
}

return j;

Konstantin

> +}
> +
>  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..42f8188 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 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_tdev_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_tvlan_tpid_set;  /**< Outer VLAN TPID 
> Setup. */
> @@ -2273,6 +2278,28 @@ 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.
> + * @param num
> + *   The size of ptypes array.
> + * @return
> + *   - (>0) Number of ptypes supported. May be greater than param num and
> + *   caller needs to check that.
> + *   - (0 or -ENOTSUP) if PMD does not fill the specified ptype.
> + *   - (-ENODEV) if *port_id* invalid.
> + */
> +extern int rte_eth_dev_get_ptype_info(uint8_t port_id,
> +   uint32_t ptype_mask,
> +   uint32_t ptypes[],
> +   int num);
> +
> +/**
>   * 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..d116711 100644
> --- a/lib/librte_mbuf/rte_mbuf.h
> +++ b/lib/librte_mbuf/rte_mbuf.h
> @@ -667,6 +667,12 @@ extern "C" {
>  #define RTE_PTYPE_INNER_L4_MASK 0x0f00
> 
>  /**
> +  * Total number of all kinds of RTE_PTYPE_*, except from *_MASK, is 37 for 
> now
> +  * and reserve some space for new ptypes
> +  */
> +#define RTE_PTYPE_MAX_NUM64
> +
> +/**
>   * 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.

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

2016-01-15 Thread Adrien Mazarguil 
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 
> ---
>  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 = _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?

> +
>  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..42f8188 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 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_tdev_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_tvlan_tpid_set;  /**< Outer VLAN TPID 
> Setup. */
> @@ -2273,6 +2278,28 @@ 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.
> + *
> + *

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

2016-01-15 Thread Jianfeng Tan 
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 
---
 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 = _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;
+}
+
 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..42f8188 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 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_tdev_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_tvlan_tpid_set;  /**< Outer VLAN TPID 
Setup. */
@@ -2273,6 +2278,28 @@ 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.
+ * @param num
+ *   The size of ptypes array.
+ * @return
+ *   - (>0) Number of ptypes supported. May be greater than param num and
+ * caller needs to check that.
+ *   - (0 or -ENOTSUP) if PMD does not fill the specified ptype.
+ *   - (-ENODEV) if *port_id* invalid.
+ */
+extern int rte_eth_dev_get_ptype_info(uint8_t port_id,
+ uint32_t ptype_mask,
+ uint32_t ptypes[],
+ int num);
+
+/**
  * 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..d116711 100644
--- a/lib/librte_mbuf/rte_mbuf.h
+++ b/lib/librte_mbuf/rte_mbuf.h
@@ -667,6 +667,12 @@ extern "C" {
 #define RTE_PTYPE_INNER_L4_MASK 0x0f00

 /**
+  * Total number of all kinds of RTE_PTYPE_*, except from *_MASK, is 37 for now
+  * and reserve some space for new ptypes
+  */
+#define RTE_PTYPE_MAX_NUM  64
+
+/**
  * 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
  * determine if it is an IPV4 packet.
-- 
2.1.4