[dpdk-dev] [PATCH 01/12] ethdev: add API to query what/if packet type is set
Hi, According to the proposal, I'm going to fix the definition of this API as below: /** * 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 * Size of ptypes[] * @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[], uint32_t num); Unresolved issues: 1) When num is exceeded, we just stop there and return num, or return -ENOMEM? The first way has a bug when: what app is exactly asking for is not filled in ptypes[] because of exceeding num, but app believes this API returns with success. 2) if RTE_PTYPE_*_MAX_NUM macros necessary? Without them, we could calculate num through 2^(number of bit 1 in RTE_PTPE_*_MASK). Thanks, Jianfeng
[dpdk-dev] [PATCH 01/12] ethdev: add API to query what/if packet type is set
Hi Jianfeng, > -Original Message- > From: Tan, Jianfeng > Sent: Monday, January 11, 2016 7:39 AM > To: Ananyev, Konstantin; N?lio Laranjeiro; dev at dpdk.org > Subject: Re: [dpdk-dev] [PATCH 01/12] ethdev: add API to query what/if packet > type is set > > Hi, > > According to the proposal, I'm going to fix the definition of this API > as below: > /** > * 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 > * Size of ptypes[] > * @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[], > uint32_t num); > > Unresolved issues: > 1) When num is exceeded, we just stop there and return num, or return > -ENOMEM? I think when num is exceeded it should return number of entries enough to return all requested packet types. Same as snprintf() does when it has to truncate the output buffer. > The first way has a bug when: what app is exactly asking for is not > filled in ptypes[] because of > exceeding num, but app believes this API returns with success. It is a caller responsibility to check the return value and handle it properly. When return value exceeds num - caller can resize ptypes[] and call get_ptype_info() again. > > 2) if RTE_PTYPE_*_MAX_NUM macros necessary? Without them, we could calculate > num through 2^(number of bit 1 in RTE_PTPE_*_MASK). I don't think caller has to guess somehow what number of entries in ptypes[] he need . He can retrieve that information from get_ptype_info() itself. Something like that for example: num = rte_eth_dev_get_ptype_info(port, UINT32_MAX, NULL, 0); if (num < 0) return num; ptypes = alloca(num * sizeof(ptypes[0]); ret = rte_eth_dev_get_ptype_info(port, UINT32_MAX, ptypes, num); if (ret != num) return -1; Konstantin > > Thanks, > Jianfeng
[dpdk-dev] [PATCH 01/12] ethdev: add API to query what/if packet type is set
On Thu, Jan 07, 2016 at 10:24:19AM +, Ananyev, Konstantin wrote: > > > > -Original Message- > > From: Adrien Mazarguil [mailto:adrien.mazarguil at 6wind.com] > > Sent: Wednesday, January 06, 2016 5:23 PM > > To: Ananyev, Konstantin > > Cc: N?lio Laranjeiro; Tan, Jianfeng; dev at dpdk.org > > Subject: Re: [dpdk-dev] [PATCH 01/12] ethdev: add API to query what/if > > packet type is set > > > > On Wed, Jan 06, 2016 at 04:44:43PM +, Ananyev, Konstantin wrote: > > > > > > > > > > -Original Message- > > > > From: Adrien Mazarguil [mailto:adrien.mazarguil at 6wind.com] > > > > Sent: Wednesday, January 06, 2016 3:45 PM > > > > To: Ananyev, Konstantin > > > > Cc: N?lio Laranjeiro; Tan, Jianfeng; dev at dpdk.org > > > > Subject: Re: [dpdk-dev] [PATCH 01/12] ethdev: add API to query what/if > > > > packet type is set > > > > > > > > On Wed, Jan 06, 2016 at 02:29:07PM +, Ananyev, Konstantin wrote: > > > > > > > > > > > > > > > > -Original Message- > > > > > > From: Adrien Mazarguil [mailto:adrien.mazarguil at 6wind.com] > > > > > > Sent: Wednesday, January 06, 2016 10:01 AM > > > > > > To: Ananyev, Konstantin > > > > > > Cc: N?lio Laranjeiro; Tan, Jianfeng; dev at dpdk.org > > > > > > Subject: Re: [dpdk-dev] [PATCH 01/12] ethdev: add API to query > > > > > > what/if packet type is set > > > > > > > > > > > > On Tue, Jan 05, 2016 at 04:50:31PM +, Ananyev, Konstantin wrote: > > > > > > [...] > > > > > > > > -Original Message- > > > > > > > > From: N?lio Laranjeiro [mailto:nelio.laranjeiro at 6wind.com] > > > > > > [...] > > > > > > > > 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. > > > > > > > > > > > > There are other drawbacks to the mask argument in my opinion. The > > > > > > API will > > > > > > have to be updated again as soon as 32 bits aren't enough to > > > > > > represent all > > > > > > possible masks. We can't predict it will be large enough forever > > > > > > but on the > > > > > > other hand, using uint64_t seems overkill at this point. > > > > > > > > > > Inside rte_mbuf packet_type itself is a 32 bit value. > > > > > These 32 bits are divided into several fields to mark packet types, > > > > > i.e: bits [0-3] are for all possible L2 types, bits [4-7] for L3 > > > > > types, etc. > > > > > As long as packet_type itself is 32bits, 32bit mask is sufficient. > > > > > If we'll ever run out of 32 bits in packet_type itself, it will be > > > > > ABI change anyway. > > > > > > > > Sure, however why not do it now this issue has been raised so this > > > > function > > > > doesn't need updating the day it breaks? I know there's a million other > > > > places with a similar problem but I'm all for making new code future > > > > proof. > > > > > > If rte_mbuf packet_type will have to be increased to 64bit long, then > > > this function will have to change anyway (with or without mask parameter). > > > It will have to become: > > > > > > rte_eth_dev_get_ptype_info(uint8_t portid, uint64_t ptypes[], ...) > > > > > > So I think we don't have to worry about mask parameter itself. > > > > Well, yes, besides I overlooked ptypes[] itself is 32 bit, working around > > the type width of the mask wouldn't help much. > > > > > > Perhaps in this particular case there is no way to hit the limit > > > > (although > > > > there are only four unused bits left to extend RTE_PTYPE masks) but > > > > we've > > > > seen this happen too many times with subsequent ABI breakage. > > > > > > When ptype was introduced we tried to reserve some free space for each > > > layer (L2/L3/L4/...), > > > so it wouldn't be overrun immediately. > > > But of course if there would be a new HW that can recognise dozen new > > > packet types - it is possible. > > > Do you have any particular use-case in mind? > > > > No, that was just to illustrate my point. > > > > > > > > I think this use
[dpdk-dev] [PATCH 01/12] ethdev: add API to query what/if packet type is set
> -Original Message- > From: Adrien Mazarguil [mailto:adrien.mazarguil at 6wind.com] > Sent: Wednesday, January 06, 2016 5:23 PM > To: Ananyev, Konstantin > Cc: N?lio Laranjeiro; Tan, Jianfeng; dev at dpdk.org > Subject: Re: [dpdk-dev] [PATCH 01/12] ethdev: add API to query what/if packet > type is set > > On Wed, Jan 06, 2016 at 04:44:43PM +, Ananyev, Konstantin wrote: > > > > > > > -Original Message- > > > From: Adrien Mazarguil [mailto:adrien.mazarguil at 6wind.com] > > > Sent: Wednesday, January 06, 2016 3:45 PM > > > To: Ananyev, Konstantin > > > Cc: N?lio Laranjeiro; Tan, Jianfeng; dev at dpdk.org > > > Subject: Re: [dpdk-dev] [PATCH 01/12] ethdev: add API to query what/if > > > packet type is set > > > > > > On Wed, Jan 06, 2016 at 02:29:07PM +, Ananyev, Konstantin wrote: > > > > > > > > > > > > > -Original Message- > > > > > From: Adrien Mazarguil [mailto:adrien.mazarguil at 6wind.com] > > > > > Sent: Wednesday, January 06, 2016 10:01 AM > > > > > To: Ananyev, Konstantin > > > > > Cc: N?lio Laranjeiro; Tan, Jianfeng; dev at dpdk.org > > > > > Subject: Re: [dpdk-dev] [PATCH 01/12] ethdev: add API to query > > > > > what/if packet type is set > > > > > > > > > > On Tue, Jan 05, 2016 at 04:50:31PM +, Ananyev, Konstantin wrote: > > > > > [...] > > > > > > > -Original Message- > > > > > > > From: N?lio Laranjeiro [mailto:nelio.laranjeiro at 6wind.com] > > > > > [...] > > > > > > > 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. > > > > > > > > > > There are other drawbacks to the mask argument in my opinion. The API > > > > > will > > > > > have to be updated again as soon as 32 bits aren't enough to > > > > > represent all > > > > > possible masks. We can't predict it will be large enough forever but > > > > > on the > > > > > other hand, using uint64_t seems overkill at this point. > > > > > > > > Inside rte_mbuf packet_type itself is a 32 bit value. > > > > These 32 bits are divided into several fields to mark packet types, > > > > i.e: bits [0-3] are for all possible L2 types, bits [4-7] for L3 types, > > > > etc. > > > > As long as packet_type itself is 32bits, 32bit mask is sufficient. > > > > If we'll ever run out of 32 bits in packet_type itself, it will be ABI > > > > change anyway. > > > > > > Sure, however why not do it now this issue has been raised so this > > > function > > > doesn't need updating the day it breaks? I know there's a million other > > > places with a similar problem but I'm all for making new code future > > > proof. > > > > If rte_mbuf packet_type will have to be increased to 64bit long, then > > this function will have to change anyway (with or without mask parameter). > > It will have to become: > > > > rte_eth_dev_get_ptype_info(uint8_t portid, uint64_t ptypes[], ...) > > > > So I think we don't have to worry about mask parameter itself. > > Well, yes, besides I overlooked ptypes[] itself is 32 bit, working around > the type width of the mask wouldn't help much. > > > > Perhaps in this particular case there is no way to hit the limit (although > > > there are only four unused bits left to extend RTE_PTYPE masks) but we've > > > seen this happen too many times with subsequent ABI breakage. > > > > When ptype was introduced we tried to reserve some free space for each > > layer (L2/L3/L4/...), > > so it wouldn't be overrun immediately. > > But of course if there would be a new HW that can recognise dozen new > > packet types - it is possible. > > Do you have any particular use-case in mind? > > No, that was just to illustrate my point. > > > > > > I think this use for masks should be avoided when performance does not > > > > > matter much, as in this case, user application cannot know the number > > > > > of > > > > > entries in advance and must rely on the returned value to iterate. > > > > > > > > User doesn't know numbers of entries in advance anyway (with and > > > >
[dpdk-dev] [PATCH 01/12] ethdev: add API to query what/if packet type is set
On Wed, Jan 06, 2016 at 04:44:43PM +, Ananyev, Konstantin wrote: > > > > -Original Message- > > From: Adrien Mazarguil [mailto:adrien.mazarguil at 6wind.com] > > Sent: Wednesday, January 06, 2016 3:45 PM > > To: Ananyev, Konstantin > > Cc: N?lio Laranjeiro; Tan, Jianfeng; dev at dpdk.org > > Subject: Re: [dpdk-dev] [PATCH 01/12] ethdev: add API to query what/if > > packet type is set > > > > On Wed, Jan 06, 2016 at 02:29:07PM +, Ananyev, Konstantin wrote: > > > > > > > > > > -Original Message- > > > > From: Adrien Mazarguil [mailto:adrien.mazarguil at 6wind.com] > > > > Sent: Wednesday, January 06, 2016 10:01 AM > > > > To: Ananyev, Konstantin > > > > Cc: N?lio Laranjeiro; Tan, Jianfeng; dev at dpdk.org > > > > Subject: Re: [dpdk-dev] [PATCH 01/12] ethdev: add API to query what/if > > > > packet type is set > > > > > > > > On Tue, Jan 05, 2016 at 04:50:31PM +, Ananyev, Konstantin wrote: > > > > [...] > > > > > > -Original Message- > > > > > > From: N?lio Laranjeiro [mailto:nelio.laranjeiro at 6wind.com] > > > > [...] > > > > > > 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. > > > > > > > > There are other drawbacks to the mask argument in my opinion. The API > > > > will > > > > have to be updated again as soon as 32 bits aren't enough to represent > > > > all > > > > possible masks. We can't predict it will be large enough forever but on > > > > the > > > > other hand, using uint64_t seems overkill at this point. > > > > > > Inside rte_mbuf packet_type itself is a 32 bit value. > > > These 32 bits are divided into several fields to mark packet types, > > > i.e: bits [0-3] are for all possible L2 types, bits [4-7] for L3 types, > > > etc. > > > As long as packet_type itself is 32bits, 32bit mask is sufficient. > > > If we'll ever run out of 32 bits in packet_type itself, it will be ABI > > > change anyway. > > > > Sure, however why not do it now this issue has been raised so this function > > doesn't need updating the day it breaks? I know there's a million other > > places with a similar problem but I'm all for making new code future proof. > > If rte_mbuf packet_type will have to be increased to 64bit long, then > this function will have to change anyway (with or without mask parameter). > It will have to become: > > rte_eth_dev_get_ptype_info(uint8_t portid, uint64_t ptypes[], ...) > > So I think we don't have to worry about mask parameter itself. Well, yes, besides I overlooked ptypes[] itself is 32 bit, working around the type width of the mask wouldn't help much. > > Perhaps in this particular case there is no way to hit the limit (although > > there are only four unused bits left to extend RTE_PTYPE masks) but we've > > seen this happen too many times with subsequent ABI breakage. > > When ptype was introduced we tried to reserve some free space for each layer > (L2/L3/L4/...), > so it wouldn't be overrun immediately. > But of course if there would be a new HW that can recognise dozen new packet > types - it is possible. > Do you have any particular use-case in mind? No, that was just to illustrate my point. > > > > I think this use for masks should be avoided when performance does not > > > > matter much, as in this case, user application cannot know the number of > > > > entries in advance and must rely on the returned value to iterate. > > > > > > User doesn't know numbers of entries in advance anyway (with and without > > > the mask). > > > That's why this function was introduced at first place. > > > > > > With mask it just a bit more handy, in case user cares only about > > > particular subset of supported > > > packet types (only L2 let say). > > > > OK, so we definitely need something to let applications know the layer a > > given packet type belongs to, I'm sure it can be done in a convenient way > > that won't be limited to the underlying type of the mask. > > > > > > A helper function can be added to convert a RTE_PTYPE_* value to the > > > > layer > > > > it belongs to (using enum to
[dpdk-dev] [PATCH 01/12] ethdev: add API to query what/if packet type is set
> -Original Message- > From: Adrien Mazarguil [mailto:adrien.mazarguil at 6wind.com] > Sent: Wednesday, January 06, 2016 3:45 PM > To: Ananyev, Konstantin > Cc: N?lio Laranjeiro; Tan, Jianfeng; dev at dpdk.org > Subject: Re: [dpdk-dev] [PATCH 01/12] ethdev: add API to query what/if packet > type is set > > On Wed, Jan 06, 2016 at 02:29:07PM +, Ananyev, Konstantin wrote: > > > > > > > -Original Message- > > > From: Adrien Mazarguil [mailto:adrien.mazarguil at 6wind.com] > > > Sent: Wednesday, January 06, 2016 10:01 AM > > > To: Ananyev, Konstantin > > > Cc: N?lio Laranjeiro; Tan, Jianfeng; dev at dpdk.org > > > Subject: Re: [dpdk-dev] [PATCH 01/12] ethdev: add API to query what/if > > > packet type is set > > > > > > On Tue, Jan 05, 2016 at 04:50:31PM +, Ananyev, Konstantin wrote: > > > [...] > > > > > -Original Message- > > > > > From: N?lio Laranjeiro [mailto:nelio.laranjeiro at 6wind.com] > > > [...] > > > > > 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. > > > > > > There are other drawbacks to the mask argument in my opinion. The API will > > > have to be updated again as soon as 32 bits aren't enough to represent all > > > possible masks. We can't predict it will be large enough forever but on > > > the > > > other hand, using uint64_t seems overkill at this point. > > > > Inside rte_mbuf packet_type itself is a 32 bit value. > > These 32 bits are divided into several fields to mark packet types, > > i.e: bits [0-3] are for all possible L2 types, bits [4-7] for L3 types, etc. > > As long as packet_type itself is 32bits, 32bit mask is sufficient. > > If we'll ever run out of 32 bits in packet_type itself, it will be ABI > > change anyway. > > Sure, however why not do it now this issue has been raised so this function > doesn't need updating the day it breaks? I know there's a million other > places with a similar problem but I'm all for making new code future proof. If rte_mbuf packet_type will have to be increased to 64bit long, then this function will have to change anyway (with or without mask parameter). It will have to become: rte_eth_dev_get_ptype_info(uint8_t portid, uint64_t ptypes[], ...) So I think we don't have to worry about mask parameter itself. > > Perhaps in this particular case there is no way to hit the limit (although > there are only four unused bits left to extend RTE_PTYPE masks) but we've > seen this happen too many times with subsequent ABI breakage. When ptype was introduced we tried to reserve some free space for each layer (L2/L3/L4/...), so it wouldn't be overrun immediately. But of course if there would be a new HW that can recognise dozen new packet types - it is possible. Do you have any particular use-case in mind? > > > > I think this use for masks should be avoided when performance does not > > > matter much, as in this case, user application cannot know the number of > > > entries in advance and must rely on the returned value to iterate. > > > > User doesn't know numbers of entries in advance anyway (with and without > > the mask). > > That's why this function was introduced at first place. > > > > With mask it just a bit more handy, in case user cares only about > > particular subset of supported > > packet types (only L2 let say). > > OK, so we definitely need something to let applications know the layer a > given packet type belongs to, I'm sure it can be done in a convenient way > that won't be limited to the underlying type of the mask. > > > > A helper function can be added to convert a RTE_PTYPE_* value to the layer > > > it belongs to (using enum to define possible values). > > > > Not sure what for? > > This is assuming rte_eth_dev_get_ptype_info() doesn't filter anything (no > "mask" argument). In that case a separate function must be added to convert > RTE_PTYPE_* values to a layer, so applications can look for interesting > packet types while parsing plist[] on their own. Honestly, I don't see why do you need that. You already do know that let say RTE_PTYPE_L3_IPV4 belongs to L3. Why do you need some extra enum
[dpdk-dev] [PATCH 01/12] ethdev: add API to query what/if packet type is set
On Wed, Jan 06, 2016 at 02:29:07PM +, Ananyev, Konstantin wrote:
>
>
> > -Original Message-
> > From: Adrien Mazarguil [mailto:adrien.mazarguil at 6wind.com]
> > Sent: Wednesday, January 06, 2016 10:01 AM
> > To: Ananyev, Konstantin
> > Cc: N?lio Laranjeiro; Tan, Jianfeng; dev at dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH 01/12] ethdev: add API to query what/if
> > packet type is set
> >
> > On Tue, Jan 05, 2016 at 04:50:31PM +, Ananyev, Konstantin wrote:
> > [...]
> > > > -Original Message-
> > > > From: N?lio Laranjeiro [mailto:nelio.laranjeiro at 6wind.com]
> > [...]
> > > > 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.
> >
> > There are other drawbacks to the mask argument in my opinion. The API will
> > have to be updated again as soon as 32 bits aren't enough to represent all
> > possible masks. We can't predict it will be large enough forever but on the
> > other hand, using uint64_t seems overkill at this point.
>
> Inside rte_mbuf packet_type itself is a 32 bit value.
> These 32 bits are divided into several fields to mark packet types,
> i.e: bits [0-3] are for all possible L2 types, bits [4-7] for L3 types, etc.
> As long as packet_type itself is 32bits, 32bit mask is sufficient.
> If we'll ever run out of 32 bits in packet_type itself, it will be ABI change
> anyway.
Sure, however why not do it now this issue has been raised so this function
doesn't need updating the day it breaks? I know there's a million other
places with a similar problem but I'm all for making new code future proof.
Perhaps in this particular case there is no way to hit the limit (although
there are only four unused bits left to extend RTE_PTYPE masks) but we've
seen this happen too many times with subsequent ABI breakage.
> > I think this use for masks should be avoided when performance does not
> > matter much, as in this case, user application cannot know the number of
> > entries in advance and must rely on the returned value to iterate.
>
> User doesn't know numbers of entries in advance anyway (with and without the
> mask).
> That's why this function was introduced at first place.
>
> With mask it just a bit more handy, in case user cares only about particular
> subset of supported
> packet types (only L2 let say).
OK, so we definitely need something to let applications know the layer a
given packet type belongs to, I'm sure it can be done in a convenient way
that won't be limited to the underlying type of the mask.
> > A helper function can be added to convert a RTE_PTYPE_* value to the layer
> > it belongs to (using enum to define possible values).
>
> Not sure what for?
This is assuming rte_eth_dev_get_ptype_info() doesn't filter anything (no
"mask" argument). In that case a separate function must be added to convert
RTE_PTYPE_* values to a layer, so applications can look for interesting
packet types while parsing plist[] on their own.
This layer information could be defined as an enum, i.e.:
enum rte_ptype_info {
RTE_PTYPE_UNKNOWN,
RTE_PTYPE_L2,
RTE_PTYPE_L3,
...
};
Or even an int value (2 standing for for "layer 2" etc. Tunnel encapsulation
wouldn't be described easily that way though). It's just an idea.
> > If we absolutely want a mean to filter returned values, I suggest we use
> > this enum instead of the mask argument.
> > Since it won't be a mask, it won't
> > have to be updated every time a new protocol requires extending one.
>
> Number of bits PTYPE_L2/L3/L4,... layers are already defined.
> So let say RTE_PTYPE_L2_MASK shouldn't change if you'll add new L2 ptype -
> there are few reserved values right now.
> if one day we'll run out bits in let say RTE_PTYPE_L2_MASK and will have to
> increase its size -
> it would mean change of the packet_type layout and possible ABI breakage
> anyway.
I'm aware of this, only pointing out we tend to rely on masks and type
boundaries a bit too much when there are other methods that are as (if not
more) convenient.
Perhaps some sort of tunneled packet types beyond inner L4 consuming the
four remaining bits will be added? That could happen
[dpdk-dev] [PATCH 01/12] ethdev: add API to query what/if packet type is set
> -Original Message- > From: Adrien Mazarguil [mailto:adrien.mazarguil at 6wind.com] > Sent: Wednesday, January 06, 2016 10:01 AM > To: Ananyev, Konstantin > Cc: N?lio Laranjeiro; Tan, Jianfeng; dev at dpdk.org > Subject: Re: [dpdk-dev] [PATCH 01/12] ethdev: add API to query what/if packet > type is set > > On Tue, Jan 05, 2016 at 04:50:31PM +, Ananyev, Konstantin wrote: > [...] > > > -Original Message- > > > From: N?lio Laranjeiro [mailto:nelio.laranjeiro at 6wind.com] > [...] > > > 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. > > There are other drawbacks to the mask argument in my opinion. The API will > have to be updated again as soon as 32 bits aren't enough to represent all > possible masks. We can't predict it will be large enough forever but on the > other hand, using uint64_t seems overkill at this point. Inside rte_mbuf packet_type itself is a 32 bit value. These 32 bits are divided into several fields to mark packet types, i.e: bits [0-3] are for all possible L2 types, bits [4-7] for L3 types, etc. As long as packet_type itself is 32bits, 32bit mask is sufficient. If we'll ever run out of 32 bits in packet_type itself, it will be ABI change anyway. > > I think this use for masks should be avoided when performance does not > matter much, as in this case, user application cannot know the number of > entries in advance and must rely on the returned value to iterate. User doesn't know numbers of entries in advance anyway (with and without the mask). That's why this function was introduced at first place. With mask it just a bit more handy, in case user cares only about particular subset of supported packet types (only L2 let say). > > A helper function can be added to convert a RTE_PTYPE_* value to the layer > it belongs to (using enum to define possible values). Not sure what for? > > If we absolutely want a mean to filter returned values, I suggest we use > this enum instead of the mask argument. > Since it won't be a mask, it won't > have to be updated every time a new protocol requires extending one. Number of bits PTYPE_L2/L3/L4,... layers are already defined. So let say RTE_PTYPE_L2_MASK shouldn't change if you'll add new L2 ptype - there are few reserved values right now. if one day we'll run out bits in let say RTE_PTYPE_L2_MASK and will have to increase its size - it would mean change of the packet_type layout and possible ABI breakage anyway. Konstantin > > > > 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 > > -- > Adrien Mazarguil > 6WIND
[dpdk-dev] [PATCH 01/12] ethdev: add API to query what/if packet type is set
On Tue, Jan 05, 2016 at 04:50:31PM +, Ananyev, Konstantin wrote: [...] > > -Original Message- > > From: N?lio Laranjeiro [mailto:nelio.laranjeiro at 6wind.com] [...] > > 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. There are other drawbacks to the mask argument in my opinion. The API will have to be updated again as soon as 32 bits aren't enough to represent all possible masks. We can't predict it will be large enough forever but on the other hand, using uint64_t seems overkill at this point. I think this use for masks should be avoided when performance does not matter much, as in this case, user application cannot know the number of entries in advance and must rely on the returned value to iterate. A helper function can be added to convert a RTE_PTYPE_* value to the layer it belongs to (using enum to define possible values). If we absolutely want a mean to filter returned values, I suggest we use this enum instead of the mask argument. Since it won't be a mask, it won't have to be updated every time a new protocol requires extending one. > > 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 -- Adrien Mazarguil 6WIND
[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 +, 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
> > > ---
> > > 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 = _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_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,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[]);
> > > +
> > > +/**
> > >
[dpdk-dev] [PATCH 01/12] ethdev: add API to query what/if packet type is set
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 +, 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
> > > > ---
> > > > 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 = _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_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,23 @@ extern void rte_eth_dev_info_get(uint8_t port_id,
> > > > struct rte_eth_dev_info *dev_info);
> > > >
> > > > /**
> > > > + * Retrieve the contextual
[dpdk-dev] [PATCH 01/12] ethdev: add API to query what/if packet type is set
> -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
>
> 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
> > ---
> > 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 = _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_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,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" {
> > *
[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().
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
> ---
> 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 = _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_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,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 0x000f
> +
> +#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 0x00f0
> +
> +#define RTE_PTYPE_L3_MAX_NUM 6
> /**
> * TCP
[dpdk-dev] [PATCH 01/12] ethdev: add API to query what/if packet type is set
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
---
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 = _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_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,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 0x000f
+
+#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 0x00f0
+
+#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 0x0f00
+
+#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 0xf000
+
+#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 0x000f
+
+#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
