Message ID | 20191120152826.25074-8-elibr@mellanox.com |
---|---|
State | Changes Requested |
Delegated to: | Ilya Maximets |
Headers | show |
Series | netdev datapath actions offload | expand |
On 20.11.2019 16:28, Eli Britstein wrote: > Introduce flow flush callback for dpdk offloaded flows. > > Signed-off-by: Eli Britstein <elibr@mellanox.com> > Reviewed-by: Oz Shlomo <ozsh@mellanox.com> > --- > 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 6e1ca8a0d..64873759d 100644 > --- a/lib/netdev-offload-dpdk.c > +++ b/lib/netdev-offload-dpdk.c > @@ -307,6 +307,21 @@ netdev_offload_dpdk_flow_del(struct netdev *netdev, const ovs_u128 *ufid, > return netdev_offload_dpdk_destroy_flow(netdev, ufid, rte_flow); > } > > +static int > +netdev_offload_dpdk_flow_flush(struct netdev *netdev) > +{ > + struct rte_flow_error error; > + int ret; > + > + ret = netdev_dpdk_rte_flow_flush(netdev, &error); I don't think that it's enough to only flush flows from the device, you need to destroy all the structures allocated for already offloaded flows in OVS, i.e. at least call ufid_to_rte_flow_disassociate() for each of them. > + if (ret) { > + VLOG_ERR("%s: rte flow flush error: %u : message : %s\n", > + netdev_get_name(netdev), error.type, error.message); > + } > + > + return ret; > +} > + > static int > netdev_offload_dpdk_init_flow_api(struct netdev *netdev) > { > @@ -315,6 +330,7 @@ netdev_offload_dpdk_init_flow_api(struct netdev *netdev) > > const struct netdev_flow_api netdev_offload_dpdk = { > .type = "dpdk_flow_api", > + .flow_flush = netdev_offload_dpdk_flow_flush, > .flow_put = netdev_offload_dpdk_flow_put, > .flow_del = netdev_offload_dpdk_flow_del, > .init_flow_api = netdev_offload_dpdk_init_flow_api, >
On 11/22/2019 6:06 PM, Ilya Maximets wrote: > On 20.11.2019 16:28, Eli Britstein wrote: >> Introduce flow flush callback for dpdk offloaded flows. >> >> Signed-off-by: Eli Britstein <elibr@mellanox.com> >> Reviewed-by: Oz Shlomo <ozsh@mellanox.com> >> --- >> 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 6e1ca8a0d..64873759d 100644 >> --- a/lib/netdev-offload-dpdk.c >> +++ b/lib/netdev-offload-dpdk.c >> @@ -307,6 +307,21 @@ netdev_offload_dpdk_flow_del(struct netdev *netdev, const ovs_u128 *ufid, >> return netdev_offload_dpdk_destroy_flow(netdev, ufid, rte_flow); >> } >> >> +static int >> +netdev_offload_dpdk_flow_flush(struct netdev *netdev) >> +{ >> + struct rte_flow_error error; >> + int ret; >> + >> + ret = netdev_dpdk_rte_flow_flush(netdev, &error); > I don't think that it's enough to only flush flows from the device, > you need to destroy all the structures allocated for already offloaded > flows in OVS, i.e. at least call ufid_to_rte_flow_disassociate() > for each of them. Well, I admit I haven't followed all the code of it. I took netdev-offload-tc.c flush and adapted. It also didn't remove mappings. Anyway, if you think the TC code is also bad, I'll drop those 2 commits from this series for now > >> + if (ret) { >> + VLOG_ERR("%s: rte flow flush error: %u : message : %s\n", >> + netdev_get_name(netdev), error.type, error.message); >> + } >> + >> + return ret; >> +} >> + >> static int >> netdev_offload_dpdk_init_flow_api(struct netdev *netdev) >> { >> @@ -315,6 +330,7 @@ netdev_offload_dpdk_init_flow_api(struct netdev *netdev) >> >> const struct netdev_flow_api netdev_offload_dpdk = { >> .type = "dpdk_flow_api", >> + .flow_flush = netdev_offload_dpdk_flow_flush, >> .flow_put = netdev_offload_dpdk_flow_put, >> .flow_del = netdev_offload_dpdk_flow_del, >> .init_flow_api = netdev_offload_dpdk_init_flow_api, >>
On 24.11.2019 14:22, Eli Britstein wrote: > > On 11/22/2019 6:06 PM, Ilya Maximets wrote: >> On 20.11.2019 16:28, Eli Britstein wrote: >>> Introduce flow flush callback for dpdk offloaded flows. >>> >>> Signed-off-by: Eli Britstein <elibr@mellanox.com> >>> Reviewed-by: Oz Shlomo <ozsh@mellanox.com> >>> --- >>> 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 6e1ca8a0d..64873759d 100644 >>> --- a/lib/netdev-offload-dpdk.c >>> +++ b/lib/netdev-offload-dpdk.c >>> @@ -307,6 +307,21 @@ netdev_offload_dpdk_flow_del(struct netdev *netdev, const ovs_u128 *ufid, >>> return netdev_offload_dpdk_destroy_flow(netdev, ufid, rte_flow); >>> } >>> >>> +static int >>> +netdev_offload_dpdk_flow_flush(struct netdev *netdev) >>> +{ >>> + struct rte_flow_error error; >>> + int ret; >>> + >>> + ret = netdev_dpdk_rte_flow_flush(netdev, &error); >> I don't think that it's enough to only flush flows from the device, >> you need to destroy all the structures allocated for already offloaded >> flows in OVS, i.e. at least call ufid_to_rte_flow_disassociate() >> for each of them. > > Well, I admit I haven't followed all the code of it. I took > netdev-offload-tc.c flush and adapted. It also didn't remove mappings. > Anyway, if you think the TC code is also bad, I'll drop those 2 commits > from this series for now Yes, netdev-offload-tc code is not fully correct. You may see that Roi is trying to fix it: https://mail.openvswitch.org/pipermail/ovs-dev/2019-December/365443.html So, please, implement it correctly or drop. Incorrect implementation is not a good thing to have. Best regards, Ilya Maximets.
diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c index 6e1ca8a0d..64873759d 100644 --- a/lib/netdev-offload-dpdk.c +++ b/lib/netdev-offload-dpdk.c @@ -307,6 +307,21 @@ netdev_offload_dpdk_flow_del(struct netdev *netdev, const ovs_u128 *ufid, return netdev_offload_dpdk_destroy_flow(netdev, ufid, rte_flow); } +static int +netdev_offload_dpdk_flow_flush(struct netdev *netdev) +{ + struct rte_flow_error error; + int ret; + + ret = netdev_dpdk_rte_flow_flush(netdev, &error); + if (ret) { + VLOG_ERR("%s: rte flow flush error: %u : message : %s\n", + netdev_get_name(netdev), error.type, error.message); + } + + return ret; +} + static int netdev_offload_dpdk_init_flow_api(struct netdev *netdev) { @@ -315,6 +330,7 @@ netdev_offload_dpdk_init_flow_api(struct netdev *netdev) const struct netdev_flow_api netdev_offload_dpdk = { .type = "dpdk_flow_api", + .flow_flush = netdev_offload_dpdk_flow_flush, .flow_put = netdev_offload_dpdk_flow_put, .flow_del = netdev_offload_dpdk_flow_del, .init_flow_api = netdev_offload_dpdk_init_flow_api,