[v2,01/10] ethdev: reuse header definition in flow pattern item ETH
Checks
Commit Message
One ought to reuse existing header structs in flow items.
This particular item contains non-header fields, so it's
important to keep the header fields in a separate struct.
Signed-off-by: Ivan Malov <ivan.malov@oktetlabs.ru>
Reviewed-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
Reviewed-by: Andy Moreton <amoreton@xilinx.com>
---
lib/librte_ethdev/rte_flow.h | 44 ++++++++++++++++++++++--------------
1 file changed, 27 insertions(+), 17 deletions(-)
Comments
On 3/12/2021 11:07 AM, Ivan Malov wrote:
> One ought to reuse existing header structs in flow items.
> This particular item contains non-header fields, so it's
> important to keep the header fields in a separate struct.
>
> Signed-off-by: Ivan Malov <ivan.malov@oktetlabs.ru>
> Reviewed-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> Reviewed-by: Andy Moreton <amoreton@xilinx.com>
> ---
> lib/librte_ethdev/rte_flow.h | 44 ++++++++++++++++++++++--------------
> 1 file changed, 27 insertions(+), 17 deletions(-)
>
> diff --git a/lib/librte_ethdev/rte_flow.h b/lib/librte_ethdev/rte_flow.h
> index 669e677e9..96fd93ee1 100644
> --- a/lib/librte_ethdev/rte_flow.h
> +++ b/lib/librte_ethdev/rte_flow.h
> @@ -728,22 +728,32 @@ static const struct rte_flow_item_raw rte_flow_item_raw_mask = {
> *
> * Matches an Ethernet header.
> *
> - * The @p type field either stands for "EtherType" or "TPID" when followed
> - * by so-called layer 2.5 pattern items such as RTE_FLOW_ITEM_TYPE_VLAN. In
> - * the latter case, @p type refers to that of the outer header, with the
> - * inner EtherType/TPID provided by the subsequent pattern item. This is the
> - * same order as on the wire.
> - * If the @p type field contains a TPID value, then only tagged packets with the
> - * specified TPID will match the pattern.
> - * The field @p has_vlan can be used to match any type of tagged packets,
> - * instead of using the @p type field.
> - * If the @p type and @p has_vlan fields are not specified, then both tagged
> - * and untagged packets will match the pattern.
> + * Inside @p hdr field, the sub-field @p ether_type stands either for EtherType
> + * or TPID, depending on whether the item is followed by a VLAN item or not. If
> + * two VLAN items follow, the sub-field refers to the outer one, which, in turn,
> + * contains the inner TPID in the similar header field. The innermost VLAN item
> + * contains a layer-3 EtherType. All of that follows the order seen on the wire.
> + *
> + * If the field in question contains a TPID value, only tagged packets with the
> + * specified TPID will match the pattern. Alternatively, it's possible to match
> + * any type of tagged packets by means of the field @p has_vlan rather than use
> + * the EtherType/TPID field. Also, it's possible to leave the two fields unused.
> + * If this is the case, both tagged and untagged packets will match the pattern.
> */
It seems Ivan can do his magic with preexisting text too :)
> +RTE_STD_C11
> struct rte_flow_item_eth {
> - struct rte_ether_addr dst; /**< Destination MAC. */
> - struct rte_ether_addr src; /**< Source MAC. */
> - rte_be16_t type; /**< EtherType or TPID. */
> + union {
> + struct {
> + /*
> + * These fields are retained for compatibility.
> + * Please switch to the new header field below.
> + */
> + struct rte_ether_addr dst; /**< Destination MAC. */
> + struct rte_ether_addr src; /**< Source MAC. */
> + rte_be16_t type; /**< EtherType or TPID. */
> + };
> + struct rte_ether_hdr hdr;
> + };
> uint32_t has_vlan:1; /**< Packet header contains at least one VLAN. */
> uint32_t reserved:31; /**< Reserved, must be zero. */
> };
> @@ -751,9 +761,9 @@ struct rte_flow_item_eth {
> /** Default mask for RTE_FLOW_ITEM_TYPE_ETH. */
> #ifndef __cplusplus
> static const struct rte_flow_item_eth rte_flow_item_eth_mask = {
> - .dst.addr_bytes = "\xff\xff\xff\xff\xff\xff",
> - .src.addr_bytes = "\xff\xff\xff\xff\xff\xff",
> - .type = RTE_BE16(0x0000),
> + .hdr.d_addr.addr_bytes = "\xff\xff\xff\xff\xff\xff",
> + .hdr.s_addr.addr_bytes = "\xff\xff\xff\xff\xff\xff",
> + .hdr.ether_type = RTE_BE16(0x0000),
> };
> #endif
>
>
Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>
On 3/12/2021 11:07 AM, Ivan Malov wrote:
> One ought to reuse existing header structs in flow items.
> This particular item contains non-header fields, so it's
> important to keep the header fields in a separate struct.
>
Hi Ivan, Andrew, Thanks for following this up and updates.
For record, existing deprecation note is:
https://git.dpdk.org/dpdk/tree/doc/guides/rel_notes/deprecation.rst?h=v21.02#n99
Ori, Andrew,
Is there any struct left not updated after this patch?
> Signed-off-by: Ivan Malov <ivan.malov@oktetlabs.ru>
> Reviewed-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> Reviewed-by: Andy Moreton <amoreton@xilinx.com>
<...>
On 3/16/21 8:38 PM, Ferruh Yigit wrote:
> On 3/12/2021 11:07 AM, Ivan Malov wrote:
>> One ought to reuse existing header structs in flow items.
>> This particular item contains non-header fields, so it's
>> important to keep the header fields in a separate struct.
>>
>
> Hi Ivan, Andrew, Thanks for following this up and updates.
>
> For record, existing deprecation note is:
> https://git.dpdk.org/dpdk/tree/doc/guides/rel_notes/deprecation.rst?h=v21.02#n99
>
>
> Ori, Andrew,
>
> Is there any struct left not updated after this patch?
As I understand the following flow items corresponding to
network protocols, but still define own fields. Sometimes
corresponding network protocols do not have definitions in
DPDK, but it is not an excuse and definitions should be
simply added:
rte_flow_item_e_tag
rte_flow_item_nvgre
rte_flow_item_mpls
rte_flow_item_gre
rte_flow_item_gtp
rte_flow_item_geneve
rte_flow_item_vxlan_gpe
rte_flow_item_arp_eth_ipv4
rte_flow_item_icmp6
rte_flow_item_icmp6_nd_ns
rte_flow_item_icmp6_nd_na
rte_flow_item_icmp6_nd_opt
rte_flow_item_icmp6_nd_opt_sla_eth
rte_flow_item_icmp6_nd_opt_tla_eth
rte_flow_item_gtp_psc
rte_flow_item_pppoe
rte_flow_item_pppoe_proto_id
rte_flow_item_l2tpv3oip
rte_flow_item_nsh
rte_flow_item_igmp
rte_flow_item_ah
rte_flow_item_pfcp
rte_flow_item_geneve_opt
The list is composed very quickly so may be I oversight
something, but it definitely answers the question - yes,
more flow items remain.
>
>> Signed-off-by: Ivan Malov <ivan.malov@oktetlabs.ru>
>> Reviewed-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>> Reviewed-by: Andy Moreton <amoreton@xilinx.com>
>
> <...>
On 3/17/2021 6:40 AM, Andrew Rybchenko wrote:
> On 3/16/21 8:38 PM, Ferruh Yigit wrote:
>> On 3/12/2021 11:07 AM, Ivan Malov wrote:
>>> One ought to reuse existing header structs in flow items.
>>> This particular item contains non-header fields, so it's
>>> important to keep the header fields in a separate struct.
>>>
>>
>> Hi Ivan, Andrew, Thanks for following this up and updates.
>>
>> For record, existing deprecation note is:
>> https://git.dpdk.org/dpdk/tree/doc/guides/rel_notes/deprecation.rst?h=v21.02#n99
>>
>>
>> Ori, Andrew,
>>
>> Is there any struct left not updated after this patch?
>
> As I understand the following flow items corresponding to
> network protocols, but still define own fields. Sometimes
> corresponding network protocols do not have definitions in
> DPDK, but it is not an excuse and definitions should be
> simply added:
> rte_flow_item_e_tag
> rte_flow_item_nvgre
> rte_flow_item_mpls
> rte_flow_item_gre
> rte_flow_item_gtp
> rte_flow_item_geneve
> rte_flow_item_vxlan_gpe
> rte_flow_item_arp_eth_ipv4
> rte_flow_item_icmp6
> rte_flow_item_icmp6_nd_ns
> rte_flow_item_icmp6_nd_na
> rte_flow_item_icmp6_nd_opt
> rte_flow_item_icmp6_nd_opt_sla_eth
> rte_flow_item_icmp6_nd_opt_tla_eth
> rte_flow_item_gtp_psc
> rte_flow_item_pppoe
> rte_flow_item_pppoe_proto_id
> rte_flow_item_l2tpv3oip
> rte_flow_item_nsh
> rte_flow_item_igmp
> rte_flow_item_ah
> rte_flow_item_pfcp
> rte_flow_item_geneve_opt
>
> The list is composed very quickly so may be I oversight
> something, but it definitely answers the question - yes,
> more flow items remain.
>
Ahh, I was thinking the problematic ones, the ones adding extra fields than the
protocol header, 'rte_flow_item_eth', 'rte_flow_item_vlan' and there was one
more I guess, but you are right, all needs be updated.
So there are more to be updated, are you planning to work on any more of them in
this release?
btw, thanks again for addressing this issue.
>>
>>> Signed-off-by: Ivan Malov <ivan.malov@oktetlabs.ru>
>>> Reviewed-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>>> Reviewed-by: Andy Moreton <amoreton@xilinx.com>
>>
>> <...>
>
On 3/22/21 12:01 PM, Ferruh Yigit wrote:
> On 3/17/2021 6:40 AM, Andrew Rybchenko wrote:
>> On 3/16/21 8:38 PM, Ferruh Yigit wrote:
>>> On 3/12/2021 11:07 AM, Ivan Malov wrote:
>>>> One ought to reuse existing header structs in flow items.
>>>> This particular item contains non-header fields, so it's
>>>> important to keep the header fields in a separate struct.
>>>>
>>>
>>> Hi Ivan, Andrew, Thanks for following this up and updates.
>>>
>>> For record, existing deprecation note is:
>>> https://git.dpdk.org/dpdk/tree/doc/guides/rel_notes/deprecation.rst?h=v21.02#n99
>>>
>>>
>>>
>>> Ori, Andrew,
>>>
>>> Is there any struct left not updated after this patch?
>>
>> As I understand the following flow items corresponding to
>> network protocols, but still define own fields. Sometimes
>> corresponding network protocols do not have definitions in
>> DPDK, but it is not an excuse and definitions should be
>> simply added:
>> rte_flow_item_e_tag
>> rte_flow_item_nvgre
>> rte_flow_item_mpls
>> rte_flow_item_gre
>> rte_flow_item_gtp
>> rte_flow_item_geneve
>> rte_flow_item_vxlan_gpe
>> rte_flow_item_arp_eth_ipv4
>> rte_flow_item_icmp6
>> rte_flow_item_icmp6_nd_ns
>> rte_flow_item_icmp6_nd_na
>> rte_flow_item_icmp6_nd_opt
>> rte_flow_item_icmp6_nd_opt_sla_eth
>> rte_flow_item_icmp6_nd_opt_tla_eth
>> rte_flow_item_gtp_psc
>> rte_flow_item_pppoe
>> rte_flow_item_pppoe_proto_id
>> rte_flow_item_l2tpv3oip
>> rte_flow_item_nsh
>> rte_flow_item_igmp
>> rte_flow_item_ah
>> rte_flow_item_pfcp
>> rte_flow_item_geneve_opt
>>
>> The list is composed very quickly so may be I oversight
>> something, but it definitely answers the question - yes,
>> more flow items remain.
>>
>
> Ahh, I was thinking the problematic ones, the ones adding extra fields
> than the protocol header, 'rte_flow_item_eth', 'rte_flow_item_vlan' and
> there was one more I guess, but you are right, all needs be updated.
>
> So there are more to be updated, are you planning to work on any more of
> them in this release?
No plans on it yet.
> btw, thanks again for addressing this issue.
>
>
>>>
>>>> Signed-off-by: Ivan Malov <ivan.malov@oktetlabs.ru>
>>>> Reviewed-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>>>> Reviewed-by: Andy Moreton <amoreton@xilinx.com>
>>>
>>> <...>
>>
On 3/12/2021 11:07 AM, Ivan Malov wrote:
> One ought to reuse existing header structs in flow items.
> This particular item contains non-header fields, so it's
> important to keep the header fields in a separate struct.
>
> Signed-off-by: Ivan Malov <ivan.malov@oktetlabs.ru>
> Reviewed-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> Reviewed-by: Andy Moreton <amoreton@xilinx.com>
Series applied to dpdk-next-net/main, thanks.
@@ -728,22 +728,32 @@ static const struct rte_flow_item_raw rte_flow_item_raw_mask = {
*
* Matches an Ethernet header.
*
- * The @p type field either stands for "EtherType" or "TPID" when followed
- * by so-called layer 2.5 pattern items such as RTE_FLOW_ITEM_TYPE_VLAN. In
- * the latter case, @p type refers to that of the outer header, with the
- * inner EtherType/TPID provided by the subsequent pattern item. This is the
- * same order as on the wire.
- * If the @p type field contains a TPID value, then only tagged packets with the
- * specified TPID will match the pattern.
- * The field @p has_vlan can be used to match any type of tagged packets,
- * instead of using the @p type field.
- * If the @p type and @p has_vlan fields are not specified, then both tagged
- * and untagged packets will match the pattern.
+ * Inside @p hdr field, the sub-field @p ether_type stands either for EtherType
+ * or TPID, depending on whether the item is followed by a VLAN item or not. If
+ * two VLAN items follow, the sub-field refers to the outer one, which, in turn,
+ * contains the inner TPID in the similar header field. The innermost VLAN item
+ * contains a layer-3 EtherType. All of that follows the order seen on the wire.
+ *
+ * If the field in question contains a TPID value, only tagged packets with the
+ * specified TPID will match the pattern. Alternatively, it's possible to match
+ * any type of tagged packets by means of the field @p has_vlan rather than use
+ * the EtherType/TPID field. Also, it's possible to leave the two fields unused.
+ * If this is the case, both tagged and untagged packets will match the pattern.
*/
+RTE_STD_C11
struct rte_flow_item_eth {
- struct rte_ether_addr dst; /**< Destination MAC. */
- struct rte_ether_addr src; /**< Source MAC. */
- rte_be16_t type; /**< EtherType or TPID. */
+ union {
+ struct {
+ /*
+ * These fields are retained for compatibility.
+ * Please switch to the new header field below.
+ */
+ struct rte_ether_addr dst; /**< Destination MAC. */
+ struct rte_ether_addr src; /**< Source MAC. */
+ rte_be16_t type; /**< EtherType or TPID. */
+ };
+ struct rte_ether_hdr hdr;
+ };
uint32_t has_vlan:1; /**< Packet header contains at least one VLAN. */
uint32_t reserved:31; /**< Reserved, must be zero. */
};
@@ -751,9 +761,9 @@ struct rte_flow_item_eth {
/** Default mask for RTE_FLOW_ITEM_TYPE_ETH. */
#ifndef __cplusplus
static const struct rte_flow_item_eth rte_flow_item_eth_mask = {
- .dst.addr_bytes = "\xff\xff\xff\xff\xff\xff",
- .src.addr_bytes = "\xff\xff\xff\xff\xff\xff",
- .type = RTE_BE16(0x0000),
+ .hdr.d_addr.addr_bytes = "\xff\xff\xff\xff\xff\xff",
+ .hdr.s_addr.addr_bytes = "\xff\xff\xff\xff\xff\xff",
+ .hdr.ether_type = RTE_BE16(0x0000),
};
#endif