[dpdk-dev] [PATCH 01/12] ethdev: add API to query what/if packet type is set
Ananyev, Konstantin
konstantin.ananyev at intel.com
Tue Jan 5 17:50:31 CET 2016
Hi Neilo,
> -----Original Message-----
> From: Nélio Laranjeiro [mailto:nelio.laranjeiro at 6wind.com]
> Sent: Tuesday, January 05, 2016 4:14 PM
> To: Tan, Jianfeng
> Cc: Adrien Mazarguil; Ananyev, Konstantin; dev at dpdk.org
> Subject: Re: [dpdk-dev] [PATCH 01/12] ethdev: add API to query what/if packet type is set
>
> On Mon, Jan 04, 2016 at 02:36:14PM +0000, Ananyev, Konstantin wrote:
> >
> >
> > > -----Original Message-----
> > > From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Adrien Mazarguil
> > > Sent: Monday, January 04, 2016 11:38 AM
> > > To: Tan, Jianfeng
> > > Cc: dev at dpdk.org
> > > Subject: Re: [dpdk-dev] [PATCH 01/12] ethdev: add API to query what/if packet type is set
> > >
> > > I'm not sure about the usefulness of this new callback, but one issue I see
> > > with rte_eth_dev_get_ptype_info() is that determining the proper size for
> > > ptypes[] according to a mask is awkward. For instance suppose
> > > RTE_PTYPE_L4_MASK is redefined to a different size at some point, the caller
> > > must dynamically adjust its ptypes[] array size to avoid a possible
> > > overflow, just in case.
> > >
> > > I suggest one of these solutions:
> > >
> > > - A callback to query for a single type at once instead (easiest method in
> > > my opinion).
> > >
> > > - An additional argument with the number of entries in ptypes[], in which
> > > case rte_eth_dev_get_ptype_info() should return the number of entries that
> > > would have been filled regardless, a bit like snprintf().
> >
> > +1 for the second option.
> > Also not sure you really need: RTE_PTYPE_*_MAX_NUM macros.
> > Konstantin
>
> +1 for the second option. But see below.
>
> > >
> > > On Thu, Dec 31, 2015 at 02:53:08PM +0800, Jianfeng Tan wrote:
> > > > Add a new API rte_eth_dev_get_ptype_info to query what/if packet type will
> > > > be set by current rx burst function.
> > > >
> > > > Signed-off-by: Jianfeng Tan <jianfeng.tan at intel.com>
> > > > ---
> > > > lib/librte_ether/rte_ethdev.c | 12 ++++++++++++
> > > > lib/librte_ether/rte_ethdev.h | 22 ++++++++++++++++++++++
> > > > lib/librte_mbuf/rte_mbuf.h | 13 +++++++++++++
> > > > 3 files changed, 47 insertions(+)
> > > >
> > > > diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
> > > > index ed971b4..1885374 100644
> > > > --- a/lib/librte_ether/rte_ethdev.c
> > > > +++ b/lib/librte_ether/rte_ethdev.c
> > > > @@ -1614,6 +1614,18 @@ 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[])
> > > > +{
> > > > + struct rte_eth_dev *dev;
> > > > +
> > > > + 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);
> > > > + return (*dev->dev_ops->dev_ptype_info_get)(dev, ptype_mask, ptypes);
> > > > +}
> > > > +
> > > > 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..e97b632 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 ptype_mask, 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,23 @@ 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
> > > > + * @return
> > > > + * - (>=0) if successful. Indicate number of valid values in ptypes array.
> > > > + * - (-ENOTSUP) if hardware-assisted VLAN stripping not configured.
> > > > + * - (-ENODEV) if *port_id* invalid.
> > > > + */
> > > > +extern int rte_eth_dev_get_ptype_info(uint8_t port_id,
> > > > + uint32_t ptype_mask, uint32_t ptypes[]);
> > > > +
> > > > +/**
> > > > * 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..21d4aa2 100644
> > > > --- a/lib/librte_mbuf/rte_mbuf.h
> > > > +++ b/lib/librte_mbuf/rte_mbuf.h
> > > > @@ -282,6 +282,8 @@ extern "C" {
> > > > * It is used for outer packet for tunneling cases.
> > > > */
> > > > #define RTE_PTYPE_L2_MASK 0x0000000f
> > > > +
> > > > +#define RTE_PTYPE_L2_MAX_NUM 4
> > > > /**
> > > > * IP (Internet Protocol) version 4 packet type.
> > > > * It is used for outer packet for tunneling cases, and does not contain any
> > > > @@ -349,6 +351,8 @@ extern "C" {
> > > > * It is used for outer packet for tunneling cases.
> > > > */
> > > > #define RTE_PTYPE_L3_MASK 0x000000f0
> > > > +
> > > > +#define RTE_PTYPE_L3_MAX_NUM 6
> > > > /**
> > > > * TCP (Transmission Control Protocol) packet type.
> > > > * It is used for outer packet for tunneling cases.
> > > > @@ -435,6 +439,8 @@ extern "C" {
> > > > * It is used for outer packet for tunneling cases.
> > > > */
> > > > #define RTE_PTYPE_L4_MASK 0x00000f00
> > > > +
> > > > +#define RTE_PTYPE_L4_MAX_NUM 6
> > > > /**
> > > > * IP (Internet Protocol) in IP (Internet Protocol) tunneling packet type.
> > > > *
> > > > @@ -508,6 +514,8 @@ extern "C" {
> > > > * Mask of tunneling packet types.
> > > > */
> > > > #define RTE_PTYPE_TUNNEL_MASK 0x0000f000
> > > > +
> > > > +#define RTE_PTYPE_TUNNEL_MAX_NUM 6
> > > > /**
> > > > * Ethernet packet type.
> > > > * It is used for inner packet type only.
> > > > @@ -527,6 +535,8 @@ extern "C" {
> > > > * Mask of inner layer 2 packet types.
> > > > */
> > > > #define RTE_PTYPE_INNER_L2_MASK 0x000f0000
> > > > +
> > > > +#define RTE_PTYPE_INNER_L2_MAX_NUM 2
> > > > /**
> > > > * IP (Internet Protocol) version 4 packet type.
> > > > * It is used for inner packet only, and does not contain any header option.
> > > > @@ -588,6 +598,8 @@ extern "C" {
> > > > * Mask of inner layer 3 packet types.
> > > > */
> > > > #define RTE_PTYPE_INNER_L3_MASK 0x00f00000
> > > > +
> > > > +#define RTE_PTYPE_INNER_L3_MAX_NUM 6
> > > > /**
> > > > * TCP (Transmission Control Protocol) packet type.
> > > > * It is used for inner packet only.
> > > > @@ -666,6 +678,7 @@ extern "C" {
> > > > */
> > > > #define RTE_PTYPE_INNER_L4_MASK 0x0f000000
> > > >
> > > > +#define RTE_PTYPE_INNER_L4_MAX_NUM 6
> > > > /**
> > > > * 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
>
> I think we miss a comment here in how those 2/6/4 values are chosen
> because, according to the mask, I expect 16 possibilities but I get
> less. It will help a lot anyone who needs to add a new type.
>
> Extending the snprintf behavior above, it is best to remove the mask
> argument altogether and have rte_eth_dev_get_ptype_info() return the
> entire list every time. Applications need to iterate on the result in
> any case.
I think we'd better keep mask argument.
In many cases upper layer only interested in some particular subset of
all packet types that HW can recognise.
Let say l3fwd only cares about RTE_PTYPE_L3_MASK, it is not interested in L4,
tunnelling packet types, etc.
If caller needs to know all recognised ptypes, he can set mask==-1,
In that case all supported packet types will be returned.
>
> rte_eth_dev_get_ptype_info(uint8_t port_id, uint32_t ptypes[],
> size_t max_entries)
>
>
>
> Another point, I have read the example patch (l3fwd) but I don't
> understand why the PMD is not responsible for filling the packet type in
> the MBUF (packet parsing is done by parse_packet_type()). Why the extra
> computation?
As I understand there are 3 possibilities now:
1. HW supports ptype recognition and SW ptype parsing is never done
(--parse-ptype is not specified).
2. HW supports ptype recognition, but and SW ptype parsing is done anyway
(--parse-ptype is specified).
3. HW doesn't support and ptype recognition, and and SW ptype parsing is done
(--parse-ptype is specified).
I suppose the question is what for introduce '--parse-ptype' at all?
My thought because of #2, so people can easily check what will be the performance impact of SW parsing.
Konstantin
>
> I see it more like an offload request (as checksum, etc...) and if the
> NIC does not support it then the application does the necessary overload.
>
> Best regards,
>
> --
> Nélio Laranjeiro
> 6WIND
More information about the dev
mailing list