Message ID | 20210816135433.1922261-1-elibr@nvidia.com |
---|---|
State | Deferred |
Headers | show |
Series | [ovs-dev,V3,1/2] netdev-offload-dpdk: Use has_vlan match attribute | expand |
Context | Check | Description |
---|---|---|
ovsrobot/apply-robot | success | apply and check: success |
ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
-----Original Message----- From: Eli Britstein <elibr@nvidia.com> Sent: Monday 16 August 2021 14:55 To: dev@openvswitch.org; Ilya Maximets <i.maximets@ovn.org> Cc: Finn, Emma <emma.finn@intel.com>; Stokes, Ian <ian.stokes@intel.com>; Sriharsha Basavapatna <sriharsha.basavapatna@broadcom.com>; Gaetan Rivet <gaetanr@nvidia.com>; Majd Dibbiny <majd@nvidia.com>; Eli Britstein <elibr@nvidia.com>; Salem Sol <salems@nvidia.com> Subject: [PATCH V3 1/2] netdev-offload-dpdk: Use has_vlan match attribute DPDK 20.11 introduced an ability to specify existance/non-existance of VLAN tag by [1]. Use this attribute. [1]: 09315fc83861 ("ethdev: add VLAN attributes to ethernet and VLAN items") Signed-off-by: Eli Britstein <elibr@nvidia.com> Reviewed-by: Salem Sol <salems@nvidia.com> Hi Eli, I tested this but currently we don't have support in the i40e pmd for the has_vlan match attribute and with these patches it is breaking offload for VLAN packets on Intel devices. Thanks, Emma --- lib/netdev-offload-dpdk.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c index f6706ee0c..28c4ba276 100644 --- a/lib/netdev-offload-dpdk.c +++ b/lib/netdev-offload-dpdk.c @@ -210,6 +210,8 @@ dump_flow_pattern(struct ds *s, ds_put_cstr(s, "eth "); if (eth_spec) { + uint32_t has_vlan_mask; + if (!eth_mask) { eth_mask = &rte_flow_item_eth_mask; } @@ -222,6 +224,9 @@ dump_flow_pattern(struct ds *s, DUMP_PATTERN_ITEM(eth_mask->type, "type", "0x%04"PRIx16, ntohs(eth_spec->type), ntohs(eth_mask->type)); + has_vlan_mask = eth_mask->has_vlan ? UINT32_MAX : 0; + DUMP_PATTERN_ITEM(has_vlan_mask, "has_vlan", "%d", + eth_spec->has_vlan, eth_mask->has_vlan); } ds_put_cstr(s, "/ "); } else if (item->type == RTE_FLOW_ITEM_TYPE_VLAN) { @@ -1047,6 +1052,7 @@ parse_flow_match(struct netdev *netdev, struct flow_patterns *patterns, struct match *match) { + struct rte_flow_item_eth *eth_spec = NULL, *eth_mask = NULL; struct flow *consumed_masks; uint8_t proto = 0; @@ -1092,6 +1098,11 @@ parse_flow_match(struct netdev *netdev, memset(&consumed_masks->dl_src, 0, sizeof consumed_masks->dl_src); consumed_masks->dl_type = 0; + spec->has_vlan = 0; + mask->has_vlan = 1; + eth_spec = spec; + eth_mask = mask; + add_flow_pattern(patterns, RTE_FLOW_ITEM_TYPE_ETH, spec, mask); } @@ -1108,6 +1119,11 @@ parse_flow_match(struct netdev *netdev, /* Match any protocols. */ mask->inner_type = 0; + if (eth_spec && eth_mask) { + eth_spec->has_vlan = 1; + eth_mask->has_vlan = 1; + } + add_flow_pattern(patterns, RTE_FLOW_ITEM_TYPE_VLAN, spec, mask); } /* For untagged matching match->wc.masks.vlans[0].tci is 0xFFFF and -- 2.28.0.2311.g225365fb51
On 8/24/2021 6:08 PM, Finn, Emma wrote: > External email: Use caution opening links or attachments > > > -----Original Message----- > From: Eli Britstein <elibr@nvidia.com> > Sent: Monday 16 August 2021 14:55 > To: dev@openvswitch.org; Ilya Maximets <i.maximets@ovn.org> > Cc: Finn, Emma <emma.finn@intel.com>; Stokes, Ian <ian.stokes@intel.com>; Sriharsha Basavapatna <sriharsha.basavapatna@broadcom.com>; Gaetan Rivet <gaetanr@nvidia.com>; Majd Dibbiny <majd@nvidia.com>; Eli Britstein <elibr@nvidia.com>; Salem Sol <salems@nvidia.com> > Subject: [PATCH V3 1/2] netdev-offload-dpdk: Use has_vlan match attribute > > DPDK 20.11 introduced an ability to specify existance/non-existance of VLAN tag by [1]. > Use this attribute. > > [1]: 09315fc83861 ("ethdev: add VLAN attributes to ethernet and VLAN items") > > Signed-off-by: Eli Britstein <elibr@nvidia.com> > Reviewed-by: Salem Sol <salems@nvidia.com> > > Hi Eli, > > I tested this but currently we don't have support in the i40e pmd for the has_vlan match attribute and with these patches it is breaking offload for VLAN packets on Intel devices. Hi Emma, Thanks for testing. Is adding such support in your plans? How do you suggest to proceed? It is needed in order to fix OVS bug. Thanks, Eli > > Thanks, > Emma > --- > lib/netdev-offload-dpdk.c | 16 ++++++++++++++++ > 1 file changed, 16 insertions(+) > > diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c index f6706ee0c..28c4ba276 100644 > --- a/lib/netdev-offload-dpdk.c > +++ b/lib/netdev-offload-dpdk.c > @@ -210,6 +210,8 @@ dump_flow_pattern(struct ds *s, > > ds_put_cstr(s, "eth "); > if (eth_spec) { > + uint32_t has_vlan_mask; > + > if (!eth_mask) { > eth_mask = &rte_flow_item_eth_mask; > } > @@ -222,6 +224,9 @@ dump_flow_pattern(struct ds *s, > DUMP_PATTERN_ITEM(eth_mask->type, "type", "0x%04"PRIx16, > ntohs(eth_spec->type), > ntohs(eth_mask->type)); > + has_vlan_mask = eth_mask->has_vlan ? UINT32_MAX : 0; > + DUMP_PATTERN_ITEM(has_vlan_mask, "has_vlan", "%d", > + eth_spec->has_vlan, eth_mask->has_vlan); > } > ds_put_cstr(s, "/ "); > } else if (item->type == RTE_FLOW_ITEM_TYPE_VLAN) { @@ -1047,6 +1052,7 @@ parse_flow_match(struct netdev *netdev, > struct flow_patterns *patterns, > struct match *match) > { > + struct rte_flow_item_eth *eth_spec = NULL, *eth_mask = NULL; > struct flow *consumed_masks; > uint8_t proto = 0; > > @@ -1092,6 +1098,11 @@ parse_flow_match(struct netdev *netdev, > memset(&consumed_masks->dl_src, 0, sizeof consumed_masks->dl_src); > consumed_masks->dl_type = 0; > > + spec->has_vlan = 0; > + mask->has_vlan = 1; > + eth_spec = spec; > + eth_mask = mask; > + > add_flow_pattern(patterns, RTE_FLOW_ITEM_TYPE_ETH, spec, mask); > } > > @@ -1108,6 +1119,11 @@ parse_flow_match(struct netdev *netdev, > /* Match any protocols. */ > mask->inner_type = 0; > > + if (eth_spec && eth_mask) { > + eth_spec->has_vlan = 1; > + eth_mask->has_vlan = 1; > + } > + > add_flow_pattern(patterns, RTE_FLOW_ITEM_TYPE_VLAN, spec, mask); > } > /* For untagged matching match->wc.masks.vlans[0].tci is 0xFFFF and > -- > 2.28.0.2311.g225365fb51 >
-----Original Message----- From: Eli Britstein <elibr@nvidia.com> Sent: Tuesday 24 August 2021 16:26 To: Finn, Emma <emma.finn@intel.com>; dev@openvswitch.org; Ilya Maximets <i.maximets@ovn.org> Cc: Stokes, Ian <ian.stokes@intel.com>; Sriharsha Basavapatna <sriharsha.basavapatna@broadcom.com>; Gaetan Rivet <gaetanr@nvidia.com>; Majd Dibbiny <majd@nvidia.com>; Salem Sol <salems@nvidia.com> Subject: Re: [PATCH V3 1/2] netdev-offload-dpdk: Use has_vlan match attribute On 8/24/2021 6:08 PM, Finn, Emma wrote: > External email: Use caution opening links or attachments > > > -----Original Message----- > From: Eli Britstein <elibr@nvidia.com> > Sent: Monday 16 August 2021 14:55 > To: dev@openvswitch.org; Ilya Maximets <i.maximets@ovn.org> > Cc: Finn, Emma <emma.finn@intel.com>; Stokes, Ian <ian.stokes@intel.com>; Sriharsha Basavapatna <sriharsha.basavapatna@broadcom.com>; Gaetan Rivet <gaetanr@nvidia.com>; Majd Dibbiny <majd@nvidia.com>; Eli Britstein <elibr@nvidia.com>; Salem Sol <salems@nvidia.com> > Subject: [PATCH V3 1/2] netdev-offload-dpdk: Use has_vlan match attribute > > DPDK 20.11 introduced an ability to specify existance/non-existance of VLAN tag by [1]. > Use this attribute. > > [1]: 09315fc83861 ("ethdev: add VLAN attributes to ethernet and VLAN items") > > Signed-off-by: Eli Britstein <elibr@nvidia.com> > Reviewed-by: Salem Sol <salems@nvidia.com> > > Hi Eli, > > I tested this but currently we don't have support in the i40e pmd for the has_vlan match attribute and with these patches it is breaking offload for VLAN packets on Intel devices. Hi Emma, Thanks for testing. Is adding such support in your plans? How do you suggest to proceed? It is needed in order to fix OVS bug. Thanks, Eli Hi, Let me look into putting a feature request in DPDK for the i40e pmd. Thanks, Emma > > Thanks, > Emma > --- > lib/netdev-offload-dpdk.c | 16 ++++++++++++++++ > 1 file changed, 16 insertions(+) > > diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c index f6706ee0c..28c4ba276 100644 > --- a/lib/netdev-offload-dpdk.c > +++ b/lib/netdev-offload-dpdk.c > @@ -210,6 +210,8 @@ dump_flow_pattern(struct ds *s, > > ds_put_cstr(s, "eth "); > if (eth_spec) { > + uint32_t has_vlan_mask; > + > if (!eth_mask) { > eth_mask = &rte_flow_item_eth_mask; > } > @@ -222,6 +224,9 @@ dump_flow_pattern(struct ds *s, > DUMP_PATTERN_ITEM(eth_mask->type, "type", "0x%04"PRIx16, > ntohs(eth_spec->type), > ntohs(eth_mask->type)); > + has_vlan_mask = eth_mask->has_vlan ? UINT32_MAX : 0; > + DUMP_PATTERN_ITEM(has_vlan_mask, "has_vlan", "%d", > + eth_spec->has_vlan, eth_mask->has_vlan); > } > ds_put_cstr(s, "/ "); > } else if (item->type == RTE_FLOW_ITEM_TYPE_VLAN) { @@ -1047,6 +1052,7 @@ parse_flow_match(struct netdev *netdev, > struct flow_patterns *patterns, > struct match *match) > { > + struct rte_flow_item_eth *eth_spec = NULL, *eth_mask = NULL; > struct flow *consumed_masks; > uint8_t proto = 0; > > @@ -1092,6 +1098,11 @@ parse_flow_match(struct netdev *netdev, > memset(&consumed_masks->dl_src, 0, sizeof consumed_masks->dl_src); > consumed_masks->dl_type = 0; > > + spec->has_vlan = 0; > + mask->has_vlan = 1; > + eth_spec = spec; > + eth_mask = mask; > + > add_flow_pattern(patterns, RTE_FLOW_ITEM_TYPE_ETH, spec, mask); > } > > @@ -1108,6 +1119,11 @@ parse_flow_match(struct netdev *netdev, > /* Match any protocols. */ > mask->inner_type = 0; > > + if (eth_spec && eth_mask) { > + eth_spec->has_vlan = 1; > + eth_mask->has_vlan = 1; > + } > + > add_flow_pattern(patterns, RTE_FLOW_ITEM_TYPE_VLAN, spec, mask); > } > /* For untagged matching match->wc.masks.vlans[0].tci is 0xFFFF and > -- > 2.28.0.2311.g225365fb51 >
On 8/24/21 5:25 PM, Eli Britstein wrote: > > On 8/24/2021 6:08 PM, Finn, Emma wrote: >> External email: Use caution opening links or attachments >> >> >> -----Original Message----- >> From: Eli Britstein <elibr@nvidia.com> >> Sent: Monday 16 August 2021 14:55 >> To: dev@openvswitch.org; Ilya Maximets <i.maximets@ovn.org> >> Cc: Finn, Emma <emma.finn@intel.com>; Stokes, Ian <ian.stokes@intel.com>; Sriharsha Basavapatna <sriharsha.basavapatna@broadcom.com>; Gaetan Rivet <gaetanr@nvidia.com>; Majd Dibbiny <majd@nvidia.com>; Eli Britstein <elibr@nvidia.com>; Salem Sol <salems@nvidia.com> >> Subject: [PATCH V3 1/2] netdev-offload-dpdk: Use has_vlan match attribute >> >> DPDK 20.11 introduced an ability to specify existance/non-existance of VLAN tag by [1]. >> Use this attribute. >> >> [1]: 09315fc83861 ("ethdev: add VLAN attributes to ethernet and VLAN items") >> >> Signed-off-by: Eli Britstein <elibr@nvidia.com> >> Reviewed-by: Salem Sol <salems@nvidia.com> >> >> Hi Eli, >> >> I tested this but currently we don't have support in the i40e pmd for the has_vlan match attribute and with these patches it is breaking offload for VLAN packets on Intel devices. > > Hi Emma, > > Thanks for testing. > > Is adding such support in your plans? > > How do you suggest to proceed? It is needed in order to fix OVS bug. > > Thanks, > > Eli The "Table 1.2 rte_flow items availability in networking drivers" here: https://doc.dpdk.org/guides/nics/overview.html says that both ixgbe and i40e has a full support for 'vlan' and 'eth' items. Is it a bug? Should it be 'partial' instead? In general, this sounds like a big limitation of rte_flow API. I mean the fact that there is no way to get what is implemented by a particular driver and what is not implemented in runtime. Someone should, probably, work on adding this kind of API to DPDK. Otherwise, we will stuck with inability to use certain actions/matches unless all the drivers supports them (which is also hard to check taking documentation issues into account). If I missed it and the API actually exists, we should definitely start using it. CC: dpdk-dev and rte_flow maintainers. Thoughts? Best regards, Ilya Maximets.
On 8/24/2021 6:47 PM, Ilya Maximets wrote: > External email: Use caution opening links or attachments > > > On 8/24/21 5:25 PM, Eli Britstein wrote: >> On 8/24/2021 6:08 PM, Finn, Emma wrote: >>> External email: Use caution opening links or attachments >>> >>> >>> -----Original Message----- >>> From: Eli Britstein <elibr@nvidia.com> >>> Sent: Monday 16 August 2021 14:55 >>> To: dev@openvswitch.org; Ilya Maximets <i.maximets@ovn.org> >>> Cc: Finn, Emma <emma.finn@intel.com>; Stokes, Ian <ian.stokes@intel.com>; Sriharsha Basavapatna <sriharsha.basavapatna@broadcom.com>; Gaetan Rivet <gaetanr@nvidia.com>; Majd Dibbiny <majd@nvidia.com>; Eli Britstein <elibr@nvidia.com>; Salem Sol <salems@nvidia.com> >>> Subject: [PATCH V3 1/2] netdev-offload-dpdk: Use has_vlan match attribute >>> >>> DPDK 20.11 introduced an ability to specify existance/non-existance of VLAN tag by [1]. >>> Use this attribute. >>> >>> [1]: 09315fc83861 ("ethdev: add VLAN attributes to ethernet and VLAN items") >>> >>> Signed-off-by: Eli Britstein <elibr@nvidia.com> >>> Reviewed-by: Salem Sol <salems@nvidia.com> >>> >>> Hi Eli, >>> >>> I tested this but currently we don't have support in the i40e pmd for the has_vlan match attribute and with these patches it is breaking offload for VLAN packets on Intel devices. >> Hi Emma, >> >> Thanks for testing. >> >> Is adding such support in your plans? >> >> How do you suggest to proceed? It is needed in order to fix OVS bug. >> >> Thanks, >> >> Eli > The "Table 1.2 rte_flow items availability in networking drivers" > here: https://doc.dpdk.org/guides/nics/overview.html > says that both ixgbe and i40e has a full support for 'vlan' and > 'eth' items. Is it a bug? Should it be 'partial' instead? > > In general, this sounds like a big limitation of rte_flow API. > I mean the fact that there is no way to get what is implemented by > a particular driver and what is not implemented in runtime. > Someone should, probably, work on adding this kind of API to DPDK. > Otherwise, we will stuck with inability to use certain actions/matches > unless all the drivers supports them (which is also hard to check > taking documentation issues into account). If I missed it and the > API actually exists, we should definitely start using it. > > CC: dpdk-dev and rte_flow maintainers. > > Thoughts? There is such an API - rte_flow_validate(). However, in OVS, as each flow is independent and can have different matches and actions, we just call rte_flow_create(). The PMD (at least mlx5) first internally validates it (as if rte_flow_validate() is called), and bail out with a failure in case validate fails. Can you suggest an effective way to utilize it in OVS? In theory, if the API exists in rte_flow, OVS should not care if all PMDs support it or not. In practice, the "has_vlan" field was introduced only in 20.11, and apparently Intel has not adapted i40e PMD, so it breaks their offloads. I suspected this so I've added Emma and Ian to review it. I don't know i40e HW capabilities, but at least from PMD point of view, it can be silently ignored until a proper support is added. > > Best regards, Ilya Maximets.
On 8/24/21 6:04 PM, Eli Britstein wrote: > > On 8/24/2021 6:47 PM, Ilya Maximets wrote: >> External email: Use caution opening links or attachments >> >> >> On 8/24/21 5:25 PM, Eli Britstein wrote: >>> On 8/24/2021 6:08 PM, Finn, Emma wrote: >>>> External email: Use caution opening links or attachments >>>> >>>> >>>> -----Original Message----- >>>> From: Eli Britstein <elibr@nvidia.com> >>>> Sent: Monday 16 August 2021 14:55 >>>> To: dev@openvswitch.org; Ilya Maximets <i.maximets@ovn.org> >>>> Cc: Finn, Emma <emma.finn@intel.com>; Stokes, Ian <ian.stokes@intel.com>; Sriharsha Basavapatna <sriharsha.basavapatna@broadcom.com>; Gaetan Rivet <gaetanr@nvidia.com>; Majd Dibbiny <majd@nvidia.com>; Eli Britstein <elibr@nvidia.com>; Salem Sol <salems@nvidia.com> >>>> Subject: [PATCH V3 1/2] netdev-offload-dpdk: Use has_vlan match attribute >>>> >>>> DPDK 20.11 introduced an ability to specify existance/non-existance of VLAN tag by [1]. >>>> Use this attribute. >>>> >>>> [1]: 09315fc83861 ("ethdev: add VLAN attributes to ethernet and VLAN items") >>>> >>>> Signed-off-by: Eli Britstein <elibr@nvidia.com> >>>> Reviewed-by: Salem Sol <salems@nvidia.com> >>>> >>>> Hi Eli, >>>> >>>> I tested this but currently we don't have support in the i40e pmd for the has_vlan match attribute and with these patches it is breaking offload for VLAN packets on Intel devices. >>> Hi Emma, >>> >>> Thanks for testing. >>> >>> Is adding such support in your plans? >>> >>> How do you suggest to proceed? It is needed in order to fix OVS bug. >>> >>> Thanks, >>> >>> Eli >> The "Table 1.2 rte_flow items availability in networking drivers" >> here: https://doc.dpdk.org/guides/nics/overview.html >> says that both ixgbe and i40e has a full support for 'vlan' and >> 'eth' items. Is it a bug? Should it be 'partial' instead? >> >> In general, this sounds like a big limitation of rte_flow API. >> I mean the fact that there is no way to get what is implemented by >> a particular driver and what is not implemented in runtime. >> Someone should, probably, work on adding this kind of API to DPDK. >> Otherwise, we will stuck with inability to use certain actions/matches >> unless all the drivers supports them (which is also hard to check >> taking documentation issues into account). If I missed it and the >> API actually exists, we should definitely start using it. >> >> CC: dpdk-dev and rte_flow maintainers. >> >> Thoughts? > > There is such an API - rte_flow_validate(). > > However, in OVS, as each flow is independent and can have different matches and actions, we just call rte_flow_create(). The PMD (at least mlx5) first internally validates it (as if rte_flow_validate() is called), and bail out with a failure in case validate fails. > > Can you suggest an effective way to utilize it in OVS? This one is hard to use. And I guess, it's hard to determine what exactly is not supported as some things can be only supported in certain combinations or otherwise not supported in certain combinations. So, rte_flow_validate() doesn't seem to be suitable for checking particular features and alternating the flow construction based on that. If we had clear yes/no flags for all features that could be easily probed/verified or otherwise retrived from the driver, that would be better. But it seems to be a problem for current rte_flow implementations in actual drivers. > > In theory, if the API exists in rte_flow, OVS should not care if all PMDs support it or not. > > In practice, the "has_vlan" field was introduced only in 20.11, and apparently Intel has not adapted i40e PMD, so it breaks their offloads. I suspected this so I've added Emma and Ian to review it. > > I don't know i40e HW capabilities, but at least from PMD point of view, it can be silently ignored until a proper support is added. > >> >> Best regards, Ilya Maximets.
diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c index f6706ee0c..28c4ba276 100644 --- a/lib/netdev-offload-dpdk.c +++ b/lib/netdev-offload-dpdk.c @@ -210,6 +210,8 @@ dump_flow_pattern(struct ds *s, ds_put_cstr(s, "eth "); if (eth_spec) { + uint32_t has_vlan_mask; + if (!eth_mask) { eth_mask = &rte_flow_item_eth_mask; } @@ -222,6 +224,9 @@ dump_flow_pattern(struct ds *s, DUMP_PATTERN_ITEM(eth_mask->type, "type", "0x%04"PRIx16, ntohs(eth_spec->type), ntohs(eth_mask->type)); + has_vlan_mask = eth_mask->has_vlan ? UINT32_MAX : 0; + DUMP_PATTERN_ITEM(has_vlan_mask, "has_vlan", "%d", + eth_spec->has_vlan, eth_mask->has_vlan); } ds_put_cstr(s, "/ "); } else if (item->type == RTE_FLOW_ITEM_TYPE_VLAN) { @@ -1047,6 +1052,7 @@ parse_flow_match(struct netdev *netdev, struct flow_patterns *patterns, struct match *match) { + struct rte_flow_item_eth *eth_spec = NULL, *eth_mask = NULL; struct flow *consumed_masks; uint8_t proto = 0; @@ -1092,6 +1098,11 @@ parse_flow_match(struct netdev *netdev, memset(&consumed_masks->dl_src, 0, sizeof consumed_masks->dl_src); consumed_masks->dl_type = 0; + spec->has_vlan = 0; + mask->has_vlan = 1; + eth_spec = spec; + eth_mask = mask; + add_flow_pattern(patterns, RTE_FLOW_ITEM_TYPE_ETH, spec, mask); } @@ -1108,6 +1119,11 @@ parse_flow_match(struct netdev *netdev, /* Match any protocols. */ mask->inner_type = 0; + if (eth_spec && eth_mask) { + eth_spec->has_vlan = 1; + eth_mask->has_vlan = 1; + } + add_flow_pattern(patterns, RTE_FLOW_ITEM_TYPE_VLAN, spec, mask); } /* For untagged matching match->wc.masks.vlans[0].tci is 0xFFFF and