Re: [PATCH v2 01/12] ethdev: add API to query packet type filling info
From: Adrien Mazarguil <hidden>
Date: 2016-01-15 13:59:07
Hi Jianfeng, a few comments below. On Fri, Jan 15, 2016 at 01:45:48PM +0800, Jianfeng Tan wrote:
quoted hunk ↗ jump to hunk
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 <redacted> --- 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?
quoted hunk ↗ jump to hunk
+ 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_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,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_iddiff --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 0x0f000000 /** + * 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
This macro should not be needed if the num argument is kept. Applications should only rely on returned values.
+ +/** * 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
-- Adrien Mazarguil 6WIND