| Message ID | 20230611155827.1957023-2-elibr@nvidia.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev,V3,1/2] netdev-offload-dpdk: Fix flushing of a physdev | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/intel-ovs-compilation | success | test: success |
On Sun, Jun 11, 2023 at 06:58:27PM +0300, Eli Britstein via dev wrote: > When using a userspace vport ("vxlan0"), dpif-netdev adds an additional > netdev ("vxlan_sys_4789"). The dpif netdev ("vxlan0") is added to the > netdev-offload ports map, thus flows are associated on this netdev. > > However, flushing is done on the dpif-netdev level ("vxlan_sys_4789"), > and relevant offload flows are not destroyed. > > To fix it, add the datapath netdev to the netdev-offload ports map. In > case there is no different internal netdev, use the dpif netdev, as before. > > Fixes: adbd4301a249 ("netdev-offload-dpdk: Use per-netdev offload metadata.") > Signed-off-by: Eli Britstein <elibr@nvidia.com> Acked-by: Simon Horman <simon.horman@corigine.com>
On Thu, Jun 29, 2023 at 04:08:47PM +0200, Simon Horman wrote: > On Sun, Jun 11, 2023 at 06:58:27PM +0300, Eli Britstein via dev wrote: > > When using a userspace vport ("vxlan0"), dpif-netdev adds an additional > > netdev ("vxlan_sys_4789"). The dpif netdev ("vxlan0") is added to the > > netdev-offload ports map, thus flows are associated on this netdev. > > > > However, flushing is done on the dpif-netdev level ("vxlan_sys_4789"), > > and relevant offload flows are not destroyed. > > > > To fix it, add the datapath netdev to the netdev-offload ports map. In > > case there is no different internal netdev, use the dpif netdev, as before. > > > > Fixes: adbd4301a249 ("netdev-offload-dpdk: Use per-netdev offload metadata.") > > Signed-off-by: Eli Britstein <elibr@nvidia.com> > > Acked-by: Simon Horman <simon.horman@corigine.com> Hi, this series seems to be going stale. I'd like to ask if anyone (else) could review it. In the meantime, I've applied the series on the current master branch and am running the GitHub actions over it: 1. https://github.com/horms/ovs/actions/runs/6455946731 2. https://github.com/horms/ovs/actions/runs/6455949977
On 6/11/23 17:58, Eli Britstein wrote: > When using a userspace vport ("vxlan0"), dpif-netdev adds an additional > netdev ("vxlan_sys_4789"). The dpif netdev ("vxlan0") is added to the > netdev-offload ports map, thus flows are associated on this netdev. > > However, flushing is done on the dpif-netdev level ("vxlan_sys_4789"), > and relevant offload flows are not destroyed. > > To fix it, add the datapath netdev to the netdev-offload ports map. In > case there is no different internal netdev, use the dpif netdev, as before. > > Fixes: adbd4301a249 ("netdev-offload-dpdk: Use per-netdev offload metadata.") > Signed-off-by: Eli Britstein <elibr@nvidia.com> > --- > lib/dpif-netdev.c | 15 ++++++++++----- > lib/dpif-netlink.c | 5 ++++- > lib/dpif-provider.h | 5 +++-- > lib/dpif.c | 8 +++++--- > 4 files changed, 22 insertions(+), 11 deletions(-) > > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c > index 70b953ae6..52d2998d7 100644 > --- a/lib/dpif-netdev.c > +++ b/lib/dpif-netdev.c > @@ -547,7 +547,8 @@ static int get_port_by_name(struct dp_netdev *dp, const char *devname, > static void dp_netdev_free(struct dp_netdev *) > OVS_REQUIRES(dp_netdev_mutex); > static int do_add_port(struct dp_netdev *dp, const char *devname, > - const char *type, odp_port_t port_no) > + const char *type, odp_port_t port_no, > + struct netdev **datapath_netdev) > OVS_REQ_WRLOCK(dp->port_rwlock); > static void do_del_port(struct dp_netdev *dp, struct dp_netdev_port *) > OVS_REQ_WRLOCK(dp->port_rwlock); > @@ -1884,7 +1885,7 @@ create_dp_netdev(const char *name, const struct dpif_class *class, > > error = do_add_port(dp, name, dpif_netdev_port_open_type(dp->class, > "internal"), > - ODPP_LOCAL); > + ODPP_LOCAL, NULL); > ovs_rwlock_unlock(&dp->port_rwlock); > if (error) { > dp_netdev_free(dp); > @@ -2151,7 +2152,7 @@ out: > > static int > do_add_port(struct dp_netdev *dp, const char *devname, const char *type, > - odp_port_t port_no) > + odp_port_t port_no, struct netdev **datapath_netdev) > OVS_REQ_WRLOCK(dp->port_rwlock) > { > struct netdev_saved_flags *sf; > @@ -2167,6 +2168,9 @@ do_add_port(struct dp_netdev *dp, const char *devname, const char *type, > if (error) { > return error; > } > + if (datapath_netdev) { > + *datapath_netdev = port->netdev; > + } > > hmap_insert(&dp->ports, &port->node, hash_port_no(port_no)); > seq_change(dp->port_seq); > @@ -2196,7 +2200,7 @@ do_add_port(struct dp_netdev *dp, const char *devname, const char *type, > > static int > dpif_netdev_port_add(struct dpif *dpif, struct netdev *netdev, > - odp_port_t *port_nop) > + odp_port_t *port_nop, struct netdev **datapath_netdev) > { > struct dp_netdev *dp = get_dp_netdev(dpif); > char namebuf[NETDEV_VPORT_NAME_BUFSIZE]; > @@ -2215,7 +2219,8 @@ dpif_netdev_port_add(struct dpif *dpif, struct netdev *netdev, > } > if (!error) { > *port_nop = port_no; > - error = do_add_port(dp, dpif_port, netdev_get_type(netdev), port_no); > + error = do_add_port(dp, dpif_port, netdev_get_type(netdev), port_no, > + datapath_netdev); > } > ovs_rwlock_unlock(&dp->port_rwlock); > > diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c > index 60bd39643..a02f0f2d9 100644 > --- a/lib/dpif-netlink.c > +++ b/lib/dpif-netlink.c > @@ -1144,7 +1144,7 @@ dpif_netlink_rtnl_port_create_and_add(struct dpif_netlink *dpif, > > static int > dpif_netlink_port_add(struct dpif *dpif_, struct netdev *netdev, > - odp_port_t *port_nop) > + odp_port_t *port_nop, struct netdev **datapath_netdev) > { > struct dpif_netlink *dpif = dpif_netlink_cast(dpif_); > int error = EOPNOTSUPP; > @@ -1157,6 +1157,9 @@ dpif_netlink_port_add(struct dpif *dpif_, struct netdev *netdev, > error = dpif_netlink_port_add_compat(dpif, netdev, port_nop); > } > fat_rwlock_unlock(&dpif->upcall_lock); > + if (datapath_netdev) { > + *datapath_netdev = netdev; > + } > > return error; > } > diff --git a/lib/dpif-provider.h b/lib/dpif-provider.h > index a33c6ec30..47c573d95 100644 > --- a/lib/dpif-provider.h > +++ b/lib/dpif-provider.h > @@ -203,10 +203,11 @@ struct dpif_class { > * ODPP_NONE, attempts to use that as the port's port number. > * > * If port is successfully added, sets '*port_no' to the new port's > - * port number. Returns EBUSY if caller attempted to choose a port > + * port number, and datapath_netdev to a potentially created netdev in the > + * dpif-class level. Returns EBUSY if caller attempted to choose a port > * number, and it was in use. */ > int (*port_add)(struct dpif *dpif, struct netdev *netdev, > - odp_port_t *port_no); > + odp_port_t *port_no, struct netdev **datapath_netdev); > > /* Removes port numbered 'port_no' from 'dpif'. */ > int (*port_del)(struct dpif *dpif, odp_port_t port_no); > diff --git a/lib/dpif.c b/lib/dpif.c > index 3305401fe..926b4ca6d 100644 > --- a/lib/dpif.c > +++ b/lib/dpif.c > @@ -586,6 +586,7 @@ dpif_port_add(struct dpif *dpif, struct netdev *netdev, odp_port_t *port_nop) > { > const char *netdev_name = netdev_get_name(netdev); > odp_port_t port_no = ODPP_NONE; > + struct netdev *datapath_netdev; > int error; > > COVERAGE_INC(dpif_port_add); > @@ -594,7 +595,8 @@ dpif_port_add(struct dpif *dpif, struct netdev *netdev, odp_port_t *port_nop) > port_no = *port_nop; > } > > - error = dpif->dpif_class->port_add(dpif, netdev, &port_no); > + error = dpif->dpif_class->port_add(dpif, netdev, &port_no, > + &datapath_netdev); > if (!error) { > VLOG_DBG_RL(&dpmsg_rl, "%s: added %s as port %"PRIu32, > dpif_name(dpif), netdev_name, port_no); > @@ -604,12 +606,12 @@ dpif_port_add(struct dpif *dpif, struct netdev *netdev, odp_port_t *port_nop) > const char *dpif_type_str = dpif_normalize_type(dpif_type(dpif)); > struct dpif_port dpif_port; > > - netdev_set_dpif_type(netdev, dpif_type_str); > + netdev_set_dpif_type(datapath_netdev, dpif_type_str); > > dpif_port.type = CONST_CAST(char *, netdev_get_type(netdev)); > dpif_port.name = CONST_CAST(char *, netdev_name); > dpif_port.port_no = port_no; > - netdev_ports_insert(netdev, &dpif_port); > + netdev_ports_insert(datapath_netdev, &dpif_port); > } > } else { > VLOG_WARN_RL(&error_rl, "%s: failed to add %s as port: %s", Hmm. Don't we need a corresponding change for removal as well? We seem to add these netdevs, but we never remove them. Or am I missing something? Removal might be tricky though. Insertion handles duplicates, but removal will just remove the backing datapath netdev even though other tunnel ports may still use it. Also, we could avoid API modifications here by using netdev_vport_get_dpif_port() and looking it up my name. It's not a clean solution either, but seems a little cleaner than changing dpif-provider API. Best regards, Ilya Maximets.
diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c index 70b953ae6..52d2998d7 100644 --- a/lib/dpif-netdev.c +++ b/lib/dpif-netdev.c @@ -547,7 +547,8 @@ static int get_port_by_name(struct dp_netdev *dp, const char *devname, static void dp_netdev_free(struct dp_netdev *) OVS_REQUIRES(dp_netdev_mutex); static int do_add_port(struct dp_netdev *dp, const char *devname, - const char *type, odp_port_t port_no) + const char *type, odp_port_t port_no, + struct netdev **datapath_netdev) OVS_REQ_WRLOCK(dp->port_rwlock); static void do_del_port(struct dp_netdev *dp, struct dp_netdev_port *) OVS_REQ_WRLOCK(dp->port_rwlock); @@ -1884,7 +1885,7 @@ create_dp_netdev(const char *name, const struct dpif_class *class, error = do_add_port(dp, name, dpif_netdev_port_open_type(dp->class, "internal"), - ODPP_LOCAL); + ODPP_LOCAL, NULL); ovs_rwlock_unlock(&dp->port_rwlock); if (error) { dp_netdev_free(dp); @@ -2151,7 +2152,7 @@ out: static int do_add_port(struct dp_netdev *dp, const char *devname, const char *type, - odp_port_t port_no) + odp_port_t port_no, struct netdev **datapath_netdev) OVS_REQ_WRLOCK(dp->port_rwlock) { struct netdev_saved_flags *sf; @@ -2167,6 +2168,9 @@ do_add_port(struct dp_netdev *dp, const char *devname, const char *type, if (error) { return error; } + if (datapath_netdev) { + *datapath_netdev = port->netdev; + } hmap_insert(&dp->ports, &port->node, hash_port_no(port_no)); seq_change(dp->port_seq); @@ -2196,7 +2200,7 @@ do_add_port(struct dp_netdev *dp, const char *devname, const char *type, static int dpif_netdev_port_add(struct dpif *dpif, struct netdev *netdev, - odp_port_t *port_nop) + odp_port_t *port_nop, struct netdev **datapath_netdev) { struct dp_netdev *dp = get_dp_netdev(dpif); char namebuf[NETDEV_VPORT_NAME_BUFSIZE]; @@ -2215,7 +2219,8 @@ dpif_netdev_port_add(struct dpif *dpif, struct netdev *netdev, } if (!error) { *port_nop = port_no; - error = do_add_port(dp, dpif_port, netdev_get_type(netdev), port_no); + error = do_add_port(dp, dpif_port, netdev_get_type(netdev), port_no, + datapath_netdev); } ovs_rwlock_unlock(&dp->port_rwlock); diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c index 60bd39643..a02f0f2d9 100644 --- a/lib/dpif-netlink.c +++ b/lib/dpif-netlink.c @@ -1144,7 +1144,7 @@ dpif_netlink_rtnl_port_create_and_add(struct dpif_netlink *dpif, static int dpif_netlink_port_add(struct dpif *dpif_, struct netdev *netdev, - odp_port_t *port_nop) + odp_port_t *port_nop, struct netdev **datapath_netdev) { struct dpif_netlink *dpif = dpif_netlink_cast(dpif_); int error = EOPNOTSUPP; @@ -1157,6 +1157,9 @@ dpif_netlink_port_add(struct dpif *dpif_, struct netdev *netdev, error = dpif_netlink_port_add_compat(dpif, netdev, port_nop); } fat_rwlock_unlock(&dpif->upcall_lock); + if (datapath_netdev) { + *datapath_netdev = netdev; + } return error; } diff --git a/lib/dpif-provider.h b/lib/dpif-provider.h index a33c6ec30..47c573d95 100644 --- a/lib/dpif-provider.h +++ b/lib/dpif-provider.h @@ -203,10 +203,11 @@ struct dpif_class { * ODPP_NONE, attempts to use that as the port's port number. * * If port is successfully added, sets '*port_no' to the new port's - * port number. Returns EBUSY if caller attempted to choose a port + * port number, and datapath_netdev to a potentially created netdev in the + * dpif-class level. Returns EBUSY if caller attempted to choose a port * number, and it was in use. */ int (*port_add)(struct dpif *dpif, struct netdev *netdev, - odp_port_t *port_no); + odp_port_t *port_no, struct netdev **datapath_netdev); /* Removes port numbered 'port_no' from 'dpif'. */ int (*port_del)(struct dpif *dpif, odp_port_t port_no); diff --git a/lib/dpif.c b/lib/dpif.c index 3305401fe..926b4ca6d 100644 --- a/lib/dpif.c +++ b/lib/dpif.c @@ -586,6 +586,7 @@ dpif_port_add(struct dpif *dpif, struct netdev *netdev, odp_port_t *port_nop) { const char *netdev_name = netdev_get_name(netdev); odp_port_t port_no = ODPP_NONE; + struct netdev *datapath_netdev; int error; COVERAGE_INC(dpif_port_add); @@ -594,7 +595,8 @@ dpif_port_add(struct dpif *dpif, struct netdev *netdev, odp_port_t *port_nop) port_no = *port_nop; } - error = dpif->dpif_class->port_add(dpif, netdev, &port_no); + error = dpif->dpif_class->port_add(dpif, netdev, &port_no, + &datapath_netdev); if (!error) { VLOG_DBG_RL(&dpmsg_rl, "%s: added %s as port %"PRIu32, dpif_name(dpif), netdev_name, port_no); @@ -604,12 +606,12 @@ dpif_port_add(struct dpif *dpif, struct netdev *netdev, odp_port_t *port_nop) const char *dpif_type_str = dpif_normalize_type(dpif_type(dpif)); struct dpif_port dpif_port; - netdev_set_dpif_type(netdev, dpif_type_str); + netdev_set_dpif_type(datapath_netdev, dpif_type_str); dpif_port.type = CONST_CAST(char *, netdev_get_type(netdev)); dpif_port.name = CONST_CAST(char *, netdev_name); dpif_port.port_no = port_no; - netdev_ports_insert(netdev, &dpif_port); + netdev_ports_insert(datapath_netdev, &dpif_port); } } else { VLOG_WARN_RL(&error_rl, "%s: failed to add %s as port: %s",
When using a userspace vport ("vxlan0"), dpif-netdev adds an additional netdev ("vxlan_sys_4789"). The dpif netdev ("vxlan0") is added to the netdev-offload ports map, thus flows are associated on this netdev. However, flushing is done on the dpif-netdev level ("vxlan_sys_4789"), and relevant offload flows are not destroyed. To fix it, add the datapath netdev to the netdev-offload ports map. In case there is no different internal netdev, use the dpif netdev, as before. Fixes: adbd4301a249 ("netdev-offload-dpdk: Use per-netdev offload metadata.") Signed-off-by: Eli Britstein <elibr@nvidia.com> --- lib/dpif-netdev.c | 15 ++++++++++----- lib/dpif-netlink.c | 5 ++++- lib/dpif-provider.h | 5 +++-- lib/dpif.c | 8 +++++--- 4 files changed, 22 insertions(+), 11 deletions(-)