| Message ID | 20220207165645.811857-1-elibr@nvidia.com |
|---|---|
| State | Accepted |
| Commit | 9b7ed5f6f2808a44a4ba2ed489fb94ada904d0e2 |
| Headers | show |
| Series | [ovs-dev,V4,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 |
On 2/7/22 17:56, Eli Britstein via dev wrote: > 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") Hi, Eli. I'm afraid we still can't use the 'has_vlan' item until there are drivers that silently ignore it. And, unfortunately, there are may of them. I created a DPDK bug for that issue: https://bugs.dpdk.org/show_bug.cgi?id=958 And sent a patch to mark drivers with partial support, as Thomas suggested: https://patches.dpdk.org/project/dpdk/patch/20220316120157.390311-1-i.maximets@ovn.org/ Is there a way to fix the issue without using the 'has_vlan' field? Emma, you said that you will ask about support in i40e driver. Is there any progress on that front? Best regards, Ilya Maximets.
> -----Original Message----- > From: Ilya Maximets <i.maximets@ovn.org> > Sent: Wednesday, March 16, 2022 2:43 PM > To: Eli Britstein <elibr@nvidia.com>; dev@openvswitch.org; Emma Finn > <emma.finn@intel.com> > Cc: i.maximets@ovn.org; Ian Stokes <ian.stokes@intel.com> > Subject: Re: [ovs-dev] [PATCH V4 1/2] netdev-offload-dpdk: Use has_vlan > match attribute > > External email: Use caution opening links or attachments > > > On 2/7/22 17:56, Eli Britstein via dev wrote: > > 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") > > Hi, Eli. I'm afraid we still can't use the 'has_vlan' item until there are drivers > that silently ignore it. And, unfortunately, there are may of them. I created > a DPDK bug for that issue: AFAIU, the problem is not about drivers silently ignoring, but with drivers that fail validation when using this flag. If a driver silently ignores, the same behavior as if not using this flag at all. > > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugs > .dpdk.org%2Fshow_bug.cgi%3Fid%3D958&data=04%7C01%7Celibr%40 > nvidia.com%7C27fe1d1dcfdc4bf4a2f708da074a8809%7C43083d15727340c1 > b7db39efd9ccc17a%7C0%7C0%7C637830314377416526%7CUnknown%7CT > WFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLC > JXVCI6Mn0%3D%7C3000&sdata=y1C4%2BFZ5kv6tYsV%2B5muxyuP86X > S24LQPx5Nx4nGzPuc%3D&reserved=0 > And sent a patch to mark drivers with partial support, as Thomas > suggested: > > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatc > hes.dpdk.org%2Fproject%2Fdpdk%2Fpatch%2F20220316120157.390311-1- > i.maximets%40ovn.org%2F&data=04%7C01%7Celibr%40nvidia.com%7 > C27fe1d1dcfdc4bf4a2f708da074a8809%7C43083d15727340c1b7db39efd9cc > c17a%7C0%7C0%7C637830314377416526%7CUnknown%7CTWFpbGZsb3d8 > eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3 > D%7C3000&sdata=ZuUeoPofJpGQxWaIbYnpO7dehehFkVymJ%2Btx5Dr3 > fC8%3D&reserved=0 > > Is there a way to fix the issue without using the 'has_vlan' field? I don't think so. The problem is a missing match, so packets hit a wrong offloaded flow instead of miss and creating a correct flow. > > Emma, you said that you will ask about support in i40e driver. > Is there any progress on that front? > > Best regards, Ilya Maximets.
On 3/16/22 14:56, Eli Britstein wrote: > > >> -----Original Message----- >> From: Ilya Maximets <i.maximets@ovn.org> >> Sent: Wednesday, March 16, 2022 2:43 PM >> To: Eli Britstein <elibr@nvidia.com>; dev@openvswitch.org; Emma Finn >> <emma.finn@intel.com> >> Cc: i.maximets@ovn.org; Ian Stokes <ian.stokes@intel.com> >> Subject: Re: [ovs-dev] [PATCH V4 1/2] netdev-offload-dpdk: Use has_vlan >> match attribute >> >> External email: Use caution opening links or attachments >> >> >> On 2/7/22 17:56, Eli Britstein via dev wrote: >>> 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") >> >> Hi, Eli. I'm afraid we still can't use the 'has_vlan' item until there are drivers >> that silently ignore it. And, unfortunately, there are may of them. I created >> a DPDK bug for that issue: > AFAIU, the problem is not about drivers silently ignoring, but with drivers that fail validation when using this flag. > If a driver silently ignores, the same behavior as if not using this flag at all. I believe that i40e driver silently ignores the field, but it was reported that offloading is broken with that driver for some reason. Emma, Ian, could you, please, give more information on what exactly happens with i40e driver if this patch set applied? If the validation actually fails, I don't see a problem with applying the patch set, because correctness is more important. But if there are drivers that accept the flow, but ignores the flag, we are getting into "what is more broken?" territory, so it might be OK to get the patches anyway, I'm not sure. This part we need to discuss, but we need more information, as what exactly is happening with the i40e driver. >> >> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugs >> .dpdk.org%2Fshow_bug.cgi%3Fid%3D958&data=04%7C01%7Celibr%40 >> nvidia.com%7C27fe1d1dcfdc4bf4a2f708da074a8809%7C43083d15727340c1 >> b7db39efd9ccc17a%7C0%7C0%7C637830314377416526%7CUnknown%7CT >> WFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLC >> JXVCI6Mn0%3D%7C3000&sdata=y1C4%2BFZ5kv6tYsV%2B5muxyuP86X >> S24LQPx5Nx4nGzPuc%3D&reserved=0 >> And sent a patch to mark drivers with partial support, as Thomas >> suggested: >> >> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatc >> hes.dpdk.org%2Fproject%2Fdpdk%2Fpatch%2F20220316120157.390311-1- >> i.maximets%40ovn.org%2F&data=04%7C01%7Celibr%40nvidia.com%7 >> C27fe1d1dcfdc4bf4a2f708da074a8809%7C43083d15727340c1b7db39efd9cc >> c17a%7C0%7C0%7C637830314377416526%7CUnknown%7CTWFpbGZsb3d8 >> eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3 >> D%7C3000&sdata=ZuUeoPofJpGQxWaIbYnpO7dehehFkVymJ%2Btx5Dr3 >> fC8%3D&reserved=0 >> >> Is there a way to fix the issue without using the 'has_vlan' field? > I don't think so. The problem is a missing match, so packets hit a wrong offloaded flow instead of miss and creating a correct flow. OK. >> >> Emma, you said that you will ask about support in i40e driver. >> Is there any progress on that front? >> >> Best regards, Ilya Maximets.
> -----Original Message----- > From: Ilya Maximets <i.maximets@ovn.org> > Sent: Thursday 17 March 2022 13:10 > To: Eli Britstein <elibr@nvidia.com>; dev@openvswitch.org; Finn, Emma > <emma.finn@intel.com> > Cc: i.maximets@ovn.org; Stokes, Ian <ian.stokes@intel.com> > Subject: Re: [ovs-dev] [PATCH V4 1/2] netdev-offload-dpdk: Use has_vlan match > attribute > > On 3/16/22 14:56, Eli Britstein wrote: > > > > > >> -----Original Message----- > >> From: Ilya Maximets <i.maximets@ovn.org> > >> Sent: Wednesday, March 16, 2022 2:43 PM > >> To: Eli Britstein <elibr@nvidia.com>; dev@openvswitch.org; Emma Finn > >> <emma.finn@intel.com> > >> Cc: i.maximets@ovn.org; Ian Stokes <ian.stokes@intel.com> > >> Subject: Re: [ovs-dev] [PATCH V4 1/2] netdev-offload-dpdk: Use > >> has_vlan match attribute > >> > >> External email: Use caution opening links or attachments > >> > >> > >> On 2/7/22 17:56, Eli Britstein via dev wrote: > >>> 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") > >> > >> Hi, Eli. I'm afraid we still can't use the 'has_vlan' item until > >> there are drivers that silently ignore it. And, unfortunately, there > >> are may of them. I created a DPDK bug for that issue: > > AFAIU, the problem is not about drivers silently ignoring, but with drivers that > fail validation when using this flag. > > If a driver silently ignores, the same behavior as if not using this flag at all. > > I believe that i40e driver silently ignores the field, but it was reported that > offloading is broken with that driver for some reason. > > Emma, Ian, could you, please, give more information on what exactly happens > with i40e driver if this patch set applied? > > If the validation actually fails, I don't see a problem with applying the patch set, > because correctness is more important. But if there are drivers that accept the > flow, but ignores the flag, we are getting into "what is more broken?" territory, > so it might be OK to get the patches anyway, I'm not sure. This part we need to > discuss, but we need more information, as what exactly is happening with the > i40e driver. > With these patches applied, only partial offload of VLAN flows will fail to match on the inner_type. |WARN|dpdk0: rte_flow creation failed: 13 (Unsupported inner_type.). |WARN|dpdk0: Failed flow: flow create 3 ingress priority 0 group 0 pattern eth src is f6:e5:d4:c3:b2:a1 dst is 1a:2b:3c:4d:5e:6f has_vlan is 1 / vlan inner_type is 0x800 tci spec 0x7b tci mask 0xefff / ipv4 fragment_offset is 0x0 / end actions mark id 1 / rss / end I tested this with other flows (Eth, IPv4) i.e when has_vlan is 0, and those flows will offload correctly when this series is applied. +CC Beilei, maintainer of the i40e PMD. Link to these patches here - https://patchwork.ozlabs.org/project/openvswitch/list/?series=284866 Link to DPDK bug report being tracked here - https://bugs.dpdk.org/show_bug.cgi?id=958 Could you look into improving support for this within the driver itself in DPDK? Thanks, Emma <snip>
On 3/23/22 16:41, Finn, Emma wrote: >> -----Original Message----- >> From: Ilya Maximets <i.maximets@ovn.org> >> Sent: Thursday 17 March 2022 13:10 >> To: Eli Britstein <elibr@nvidia.com>; dev@openvswitch.org; Finn, Emma >> <emma.finn@intel.com> >> Cc: i.maximets@ovn.org; Stokes, Ian <ian.stokes@intel.com> >> Subject: Re: [ovs-dev] [PATCH V4 1/2] netdev-offload-dpdk: Use has_vlan match >> attribute >> >> On 3/16/22 14:56, Eli Britstein wrote: >>> >>> >>>> -----Original Message----- >>>> From: Ilya Maximets <i.maximets@ovn.org> >>>> Sent: Wednesday, March 16, 2022 2:43 PM >>>> To: Eli Britstein <elibr@nvidia.com>; dev@openvswitch.org; Emma Finn >>>> <emma.finn@intel.com> >>>> Cc: i.maximets@ovn.org; Ian Stokes <ian.stokes@intel.com> >>>> Subject: Re: [ovs-dev] [PATCH V4 1/2] netdev-offload-dpdk: Use >>>> has_vlan match attribute >>>> >>>> External email: Use caution opening links or attachments >>>> >>>> >>>> On 2/7/22 17:56, Eli Britstein via dev wrote: >>>>> 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") >>>> >>>> Hi, Eli. I'm afraid we still can't use the 'has_vlan' item until >>>> there are drivers that silently ignore it. And, unfortunately, there >>>> are may of them. I created a DPDK bug for that issue: >>> AFAIU, the problem is not about drivers silently ignoring, but with drivers that >> fail validation when using this flag. >>> If a driver silently ignores, the same behavior as if not using this flag at all. >> >> I believe that i40e driver silently ignores the field, but it was reported that >> offloading is broken with that driver for some reason. >> >> Emma, Ian, could you, please, give more information on what exactly happens >> with i40e driver if this patch set applied? >> >> If the validation actually fails, I don't see a problem with applying the patch set, >> because correctness is more important. But if there are drivers that accept the >> flow, but ignores the flag, we are getting into "what is more broken?" territory, >> so it might be OK to get the patches anyway, I'm not sure. This part we need to >> discuss, but we need more information, as what exactly is happening with the >> i40e driver. >> > > With these patches applied, only partial offload of VLAN flows will fail to match on the > inner_type. > > |WARN|dpdk0: rte_flow creation failed: 13 (Unsupported inner_type.). > |WARN|dpdk0: Failed flow: flow create 3 ingress priority 0 group 0 pattern eth > src is f6:e5:d4:c3:b2:a1 dst is 1a:2b:3c:4d:5e:6f has_vlan is 1 / vlan inner_type is > 0x800 tci spec 0x7b tci mask 0xefff / ipv4 fragment_offset is 0x0 / end actions > mark id 1 / rss / end > > I tested this with other flows (Eth, IPv4) i.e when has_vlan is 0, and those flows will > offload correctly when this series is applied. OK. Thanks for checking. Based on this conversation and conversations during public meetings, applied to master and branch-2.17. Best regards, Ilya Maximets. > > +CC Beilei, maintainer of the i40e PMD. > Link to these patches here - https://patchwork.ozlabs.org/project/openvswitch/list/?series=284866 > Link to DPDK bug report being tracked here - https://bugs.dpdk.org/show_bug.cgi?id=958 > Could you look into improving support for this within the driver itself in DPDK? > > Thanks, > Emma > > <snip>
diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c index 94dc6a9b7..e0d56abc1 100644 --- a/lib/netdev-offload-dpdk.c +++ b/lib/netdev-offload-dpdk.c @@ -363,6 +363,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; } @@ -377,6 +379,9 @@ dump_flow_pattern(struct ds *s, DUMP_PATTERN_ITEM(eth_mask->type, false, "type", "0x%04"PRIx16, ntohs(eth_spec->type), ntohs(eth_mask->type), 0); + has_vlan_mask = eth_mask->has_vlan ? UINT32_MAX : 0; + DUMP_PATTERN_ITEM(has_vlan_mask, false, "has_vlan", "%d", + eth_spec->has_vlan, eth_mask->has_vlan, 0); } ds_put_cstr(s, "/ "); } else if (item->type == RTE_FLOW_ITEM_TYPE_VLAN) { @@ -1369,6 +1374,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; @@ -1414,6 +1420,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, NULL); } @@ -1430,6 +1441,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, NULL); } /* For untagged matching match->wc.masks.vlans[0].tci is 0xFFFF and
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> --- lib/netdev-offload-dpdk.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)