subject:"\[dpdk\-dev\] \[PATCH v3 01\/12\] ethdev\: add API to query packet type filling info"

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

2016-02-26 Thread Tan, Jianfeng

Hi Konstantin,

On 2/26/2016 1:16 AM, Ananyev, Konstantin wrote:
>
>> -Original Message-
>> From: Tan, Jianfeng
>> Sent: Thursday, February 25, 2016 4:36 PM
>> To: Ananyev, Konstantin; dev at dpdk.org
>> Cc: Zhang, Helin; nelio.laranjeiro at 6wind.com; adrien.mazarguil at 
>> 6wind.com; rahul.lakkireddy at chelsio.com
>> Subject: Re: [PATCH v3 01/12] ethdev: add API to query packet type filling 
>> info
>>
>> Hi Konstantin,
>>
>> On 2/25/2016 11:46 PM, Ananyev, Konstantin wrote:
>>> Hi Jainfeng,
>>>
 Add a new API rte_eth_dev_get_ptype_info to query whether/what packet
 type can be filled by given pmd rx burst function.

 Signed-off-by: Jianfeng Tan 
 ---
lib/librte_ether/rte_ethdev.c | 32 
lib/librte_ether/rte_ethdev.h | 23 +++
2 files changed, 55 insertions(+)

 diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
 index 1257965..b52555b 100644
 --- a/lib/librte_ether/rte_ethdev.c
 +++ b/lib/librte_ether/rte_ethdev.c
 @@ -1576,6 +1576,38 @@ 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 **p_ptypes)
 +{
 +  int i, j, ret;
 +  struct rte_eth_dev *dev;
 +  const uint32_t *all_ptypes;
 +
 +  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);
 +  all_ptypes = (*dev->dev_ops->dev_ptype_info_get)(dev);
 +
 +  if (!all_ptypes)
 +  return 0;
 +
 +  for (i = 0, ret = 0; all_ptypes[i] != RTE_PTYPE_UNKNOWN; ++i)
 +  if (all_ptypes[i] & ptype_mask)
 +  ret++;
 +  if (ret == 0)
 +  return 0;
 +
 +  *p_ptypes = (uint32_t *)malloc(sizeof(uint32_t) * ret);
 +  if (*p_ptypes == NULL)
 +  return -ENOMEM;
>>> I thought we all agreed to follow snprintf()-like logic and avoid any 
>>> memory allocations inside that function.
>>> So why malloc() again?
>>> Konstantin
>>>
>> Sorry for the setback. I still have concerns that snprintf()-like needs
>> to be called twice by an application to get the ptype info. So I write
>> this implementation for you to comment.
>>
>> So what's the reason why we should avoid memory allocations inside that
>> function?
> By the same reason none of other rte_ethdev get_info() style functions
> (rte_eth_dev_info_get, rte_eth_rx_queue_info_get, rte_eth_xstats_get,
> rte_eth_dev_rss_reta_query, etc) allocate space themselves.
> It is a good practice to let user to decide himself how/where to 
> allocate/free a space for that date:
> on a stack, inside a data segment (global variable), on heap (malloc),
> at hugepages via rte_malloc, somewhere else.
>
> BTW, if you had concerns about that approach, why didn't you provide any 
> arguments
> when it was discussed/agreed?
> Instead you came up a month later with same old approach that voids my and 
> other
> reviewers comments and even v2 of your own patch.
> Do you have any good reason for that?
>
> Konstantin
>

That makes sense for me now. Thanks for explanation. Will send out v4 as 
previous discussion result.

I'd like to say sorry for this delayed and wrong way to raise concern. 
Sorry for waste of your time.

Appreciate your comments on this.

Thanks,
Jianfeng

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

2016-02-25 Thread Ananyev, Konstantin



> -Original Message-
> From: Tan, Jianfeng
> Sent: Thursday, February 25, 2016 4:36 PM
> To: Ananyev, Konstantin; dev at dpdk.org
> Cc: Zhang, Helin; nelio.laranjeiro at 6wind.com; adrien.mazarguil at 
> 6wind.com; rahul.lakkireddy at chelsio.com
> Subject: Re: [PATCH v3 01/12] ethdev: add API to query packet type filling 
> info
> 
> Hi Konstantin,
> 
> On 2/25/2016 11:46 PM, Ananyev, Konstantin wrote:
> > Hi Jainfeng,
> >
> >> Add a new API rte_eth_dev_get_ptype_info to query whether/what packet
> >> type can be filled by given pmd rx burst function.
> >>
> >> Signed-off-by: Jianfeng Tan 
> >> ---
> >>   lib/librte_ether/rte_ethdev.c | 32 
> >>   lib/librte_ether/rte_ethdev.h | 23 +++
> >>   2 files changed, 55 insertions(+)
> >>
> >> diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
> >> index 1257965..b52555b 100644
> >> --- a/lib/librte_ether/rte_ethdev.c
> >> +++ b/lib/librte_ether/rte_ethdev.c
> >> @@ -1576,6 +1576,38 @@ 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 **p_ptypes)
> >> +{
> >> +  int i, j, ret;
> >> +  struct rte_eth_dev *dev;
> >> +  const uint32_t *all_ptypes;
> >> +
> >> +  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);
> >> +  all_ptypes = (*dev->dev_ops->dev_ptype_info_get)(dev);
> >> +
> >> +  if (!all_ptypes)
> >> +  return 0;
> >> +
> >> +  for (i = 0, ret = 0; all_ptypes[i] != RTE_PTYPE_UNKNOWN; ++i)
> >> +  if (all_ptypes[i] & ptype_mask)
> >> +  ret++;
> >> +  if (ret == 0)
> >> +  return 0;
> >> +
> >> +  *p_ptypes = (uint32_t *)malloc(sizeof(uint32_t) * ret);
> >> +  if (*p_ptypes == NULL)
> >> +  return -ENOMEM;
> >
> > I thought we all agreed to follow snprintf()-like logic and avoid any 
> > memory allocations inside that function.
> > So why malloc() again?
> > Konstantin
> >
> 
> Sorry for the setback. I still have concerns that snprintf()-like needs
> to be called twice by an application to get the ptype info. So I write
> this implementation for you to comment.
> 
> So what's the reason why we should avoid memory allocations inside that
> function?

By the same reason none of other rte_ethdev get_info() style functions
(rte_eth_dev_info_get, rte_eth_rx_queue_info_get, rte_eth_xstats_get,
rte_eth_dev_rss_reta_query, etc) allocate space themselves.
It is a good practice to let user to decide himself how/where to allocate/free 
a space for that date:
on a stack, inside a data segment (global variable), on heap (malloc),
at hugepages via rte_malloc, somewhere else.  

BTW, if you had concerns about that approach, why didn't you provide any 
arguments
when it was discussed/agreed?
Instead you came up a month later with same old approach that voids my and other
reviewers comments and even v2 of your own patch. 
Do you have any good reason for that?

Konstantin

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

2016-02-25 Thread Jianfeng Tan

Add a new API rte_eth_dev_get_ptype_info to query whether/what packet
type can be filled by given pmd rx burst function.

Signed-off-by: Jianfeng Tan 
---
 lib/librte_ether/rte_ethdev.c | 32 
 lib/librte_ether/rte_ethdev.h | 23 +++
 2 files changed, 55 insertions(+)

diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
index 1257965..b52555b 100644
--- a/lib/librte_ether/rte_ethdev.c
+++ b/lib/librte_ether/rte_ethdev.c
@@ -1576,6 +1576,38 @@ 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 **p_ptypes)
+{
+   int i, j, ret;
+   struct rte_eth_dev *dev;
+   const uint32_t *all_ptypes;
+
+   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);
+   all_ptypes = (*dev->dev_ops->dev_ptype_info_get)(dev);
+
+   if (!all_ptypes)
+   return 0;
+
+   for (i = 0, ret = 0; all_ptypes[i] != RTE_PTYPE_UNKNOWN; ++i)
+   if (all_ptypes[i] & ptype_mask)
+   ret++;
+   if (ret == 0)
+   return 0;
+
+   *p_ptypes = (uint32_t *)malloc(sizeof(uint32_t) * ret);
+   if (*p_ptypes == NULL)
+   return -ENOMEM;
+
+   for (i = 0, j = 0; all_ptypes[i] != RTE_PTYPE_UNKNOWN; ++i)
+   if (all_ptypes[i] & ptype_mask)
+   *p_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 16da821..341e2ff 100644
--- a/lib/librte_ether/rte_ethdev.h
+++ b/lib/librte_ether/rte_ethdev.h
@@ -1021,6 +1021,9 @@ 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 const uint32_t *(*eth_dev_ptype_info_get_t)(struct rte_eth_dev *dev);
+/**< @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 +1350,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. */
@@ -2268,6 +2272,25 @@ void rte_eth_macaddr_get(uint8_t port_id, struct 
ether_addr *mac_addr);
 void rte_eth_dev_info_get(uint8_t port_id, struct rte_eth_dev_info *dev_info);

 /**
+ * Retrieve the packet type 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 p_ptypes
+ *   A pointer to store address of adequent packet types array. Caller to free.
+ * @return
+ *   - (>0) Number of ptypes supported. Need caller to free the array.
+ *   - (0 or -ENOTSUP) if PMD does not fill the specified ptype.
+ *   - (-ENOMEM) if fail to malloc required memory to store ptypes.
+ *   - (-ENODEV) if *port_id* invalid.
+ */
+extern int rte_eth_dev_get_ptype_info(uint8_t port_id,
+ uint32_t ptype_mask,
+ uint32_t **p_ptypes);
+
+/**
  * Retrieve the MTU of an Ethernet device.
  *
  * @param port_id
-- 
2.1.4

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

2016-02-25 Thread Ananyev, Konstantin

Hi Jainfeng,

> 
> Add a new API rte_eth_dev_get_ptype_info to query whether/what packet
> type can be filled by given pmd rx burst function.
> 
> Signed-off-by: Jianfeng Tan 
> ---
>  lib/librte_ether/rte_ethdev.c | 32 
>  lib/librte_ether/rte_ethdev.h | 23 +++
>  2 files changed, 55 insertions(+)
> 
> diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
> index 1257965..b52555b 100644
> --- a/lib/librte_ether/rte_ethdev.c
> +++ b/lib/librte_ether/rte_ethdev.c
> @@ -1576,6 +1576,38 @@ 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 **p_ptypes)
> +{
> + int i, j, ret;
> + struct rte_eth_dev *dev;
> + const uint32_t *all_ptypes;
> +
> + 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);
> + all_ptypes = (*dev->dev_ops->dev_ptype_info_get)(dev);
> +
> + if (!all_ptypes)
> + return 0;
> +
> + for (i = 0, ret = 0; all_ptypes[i] != RTE_PTYPE_UNKNOWN; ++i)
> + if (all_ptypes[i] & ptype_mask)
> + ret++;
> + if (ret == 0)
> + return 0;
> +
> + *p_ptypes = (uint32_t *)malloc(sizeof(uint32_t) * ret);
> + if (*p_ptypes == NULL)
> + return -ENOMEM;


I thought we all agreed to follow snprintf()-like logic and avoid any memory 
allocations inside that function.
So why malloc() again?
Konstantin

> +
> + for (i = 0, j = 0; all_ptypes[i] != RTE_PTYPE_UNKNOWN; ++i)
> + if (all_ptypes[i] & ptype_mask)
> + *p_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 16da821..341e2ff 100644
> --- a/lib/librte_ether/rte_ethdev.h
> +++ b/lib/librte_ether/rte_ethdev.h
> @@ -1021,6 +1021,9 @@ 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 const uint32_t *(*eth_dev_ptype_info_get_t)(struct rte_eth_dev *dev);
> +/**< @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 +1350,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. */
> @@ -2268,6 +2272,25 @@ void rte_eth_macaddr_get(uint8_t port_id, struct 
> ether_addr *mac_addr);
>  void rte_eth_dev_info_get(uint8_t port_id, struct rte_eth_dev_info 
> *dev_info);
> 
>  /**
> + * Retrieve the packet type 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 p_ptypes
> + *   A pointer to store address of adequent packet types array. Caller to 
> free.
> + * @return
> + *   - (>0) Number of ptypes supported. Need caller to free the array.
> + *   - (0 or -ENOTSUP) if PMD does not fill the specified ptype.
> + *   - (-ENOMEM) if fail to malloc required memory to store ptypes.
> + *   - (-ENODEV) if *port_id* invalid.
> + */
> +extern int rte_eth_dev_get_ptype_info(uint8_t port_id,
> +   uint32_t ptype_mask,
> +   uint32_t **p_ptypes);
> +
> +/**
>   * Retrieve the MTU of an Ethernet device.
>   *
>   * @param port_id
> --
> 2.1.4

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

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

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

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

4 matches

Site Navigation

Mail list logo

Footer information