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 <jianfeng.tan at 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;
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_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_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 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
