Hi Ilya,
A few months ago we discussed the missing functionality of vports offloading
under user space bridges.
Commit [1] was added to explicitly avoid installing userspace vport flows (to
avoid confusion with the vport kernel).
When reverting commit [1] - we are left with this missing functionality.
Applying your suggested patch netdev_vport_has_system_port() API) does not
seem to work.
It always fails when it tries to look for the interface name (say "vxlan10") in
the system list where vxlan interfaces are renamed (say "vxlan_sys_4789").
You wrote:
> On master, after applying dynamic flow API patch-set, we'll be able to use
> patch that I suggested in previous mail to properly handle this situation and
> enable vport offloading.
It would be appreciated if we can resume the efforts in fixing this missing
functionality.
Please advise on the next steps.
[1]
Commit 0da667e345 ("dpif-netdev: Forbid vport offloading attempts.")
>
> -----Original Message-----
> From: Ilya Maximets <[email protected]>
> Sent: Monday, May 13, 2019 3:33 PM
> To: Ophir Munk <[email protected]>; [email protected]
> Cc: Ian Stokes <[email protected]>; Flavio Leitner <[email protected]>;
> Kevin Traynor <[email protected]>; Roni Bar Yanai
> <[email protected]>; Finn Christensen <[email protected]>; Ben Pfaff
> <[email protected]>; Simon Horman <[email protected]>; Olga
> Shern <[email protected]>; Asaf Penso <[email protected]>; Oz
> Shlomo <[email protected]>; Majd Dibbiny <[email protected]>
> Subject: Re: [PATCH] netdev: Dynamic per-port Flow API.
>
> On 13.05.2019 14:41, Ophir Munk wrote:
> > Hi Ilya,
> >
> >> -----Original Message-----
> >> From: Ilya Maximets <[email protected]>
> >> Sent: Monday, May 13, 2019 12:22 PM
> >> To: Ophir Munk <[email protected]>; [email protected]
> >> Cc: Ian Stokes <[email protected]>; Flavio Leitner
> >> <[email protected]>; Kevin Traynor <[email protected]>; Roni Bar
> >> Yanai <[email protected]>; Finn Christensen <[email protected]>;
> Ben
> >> Pfaff <[email protected]>; Simon Horman <[email protected]>;
> Olga
> >> Shern <[email protected]>; Asaf Penso <[email protected]>; Oz
> >> Shlomo <[email protected]>
> >> Subject: Re: [PATCH] netdev: Dynamic per-port Flow API.
> >>
> >> On 09.05.2019 1:59, Ophir Munk wrote:
> >>> Hi Ilya,
> >>>
> > ...
> >>> I am recreating this scenario in my setup.
> >>
> >> I see. Yes, you're right. And I think that this case could be
> >> reproduced on current master without any patches. So, it's a bug that we
> need to fix.
> >> Otherwise userspace datapath will try to offload its flows to the
> >> unrelated system interfaces. For now we could just forbid offloading
> >> to vports in dpif- netdev. I'll prepare the patch. This fix also should be
> backported.
> >>
> >
> > We need a way to enable offloading of vports in dpif-netdev. So if you
> > can address It with your new patch - that would be appreciated.
>
> I'm suggesting disabling because it'll be easy to backport to older branches.
> On master, after applying dynamic flow API patch-set, we'll be able to use
> patch that I suggested in previous mail to properly handle this situation and
> enable vport offloading.
>
> >
> >>>
> >>> This patch series is important but one of its main goals is to
> >>> enable different
> >> offloads for different vports under Kernel/userspace.
> >>> Can you please advise how this goal can be achieved?
> >>
> >> It looks like there is no way to distinguish system and userspace
> >> vports by looking only on netdev. We should check with dpif type.
> >
> > Agreed. But then looking at your sample patch below you are basing
> > your decision on the existence of system port (and not on dpif type).
>
> Right now 'ifindex' is used for checking, so this is equally racy.
>
> > I think it is risky because
> > you are dependent on the order of operations: Creation of the system port
> versus checking for system port existence.
> > Which was first (under different scenarios)?
>
> Actually, port creation and checking will always happen in the same thread
> context, so creation and checking will be serialized. Kernel should guarantee
> the port existence. No races here.
>
> >
> > What do you say about the following patch:
> >
> > diff --git a/lib/netdev-offload.c b/lib/netdev-offload.c int
> > netdev_ports_insert(struct netdev *netdev, const struct dpif_class
> > *dpif_class,
> > struct dpif_port *dpif_port) @@ -547,8 +555,9 @@
> > netdev_ports_insert(struct netdev *netdev, const struct dpif_class
> > *dpif_class,
> > netdev_ports_hash(dpif_port->port_no, dpif_class));
> > hmap_insert(&ifindex_to_port, &data->ifindex_node, ifindex);
> > ovs_mutex_unlock(&netdev_hmap_mutex);
> >
> > - netdev_init_flow_api(netdev);
> > + netdev_init_flow_api(netdev, dpif_class->type); /* Pass class
> > + type "netdev" or "syste" */
> >
> > return 0;
> > }
> >
> > It's not a full solution. It is just a hint how to pass the dpif type
> > ("netdev" or
> "system") when initializing the flow api.
> > We can use the dpif type when initializing vport offload.
>
> This is, actually, the first thing I tried. However, this requires exposing
> the
> internals of 'dpif-provider', which is bad from the point of code
> architecture.
>
> For the below patch code, I missed actual port requesting from the datapath.
> Following incremental needed:
>
> ---
> diff --git a/lib/netdev-vport.c b/lib/netdev-vport.c index
> 82943c102..1c88a91ed 100644
> --- a/lib/netdev-vport.c
> +++ b/lib/netdev-vport.c
> @@ -124,13 +124,28 @@ netdev_vport_class_get_dpif_port(const struct
> netdev_class *class) bool netdev_vport_has_system_port(const struct
> netdev *netdev) {
> - bool found;
> + bool found = false;
> + const char *name;
> + const char *type = "system";
> struct sset names = SSET_INITIALIZER(&names);
>
> ovs_assert(is_vport_class(netdev_get_class(netdev)));
>
> - dp_enumerate_names("system", &names);
> - found = sset_contains(&names, netdev_get_name(netdev));
> + dp_enumerate_names(type, &names);
> + SSET_FOR_EACH (name, &names) {
> + struct dpif *dpifp = NULL;
> +
> + if (dpif_open(name, type, &dpifp)) {
> + continue;
> + }
> +
> + found = dpif_port_exists(dpifp, netdev_get_name(netdev));
> +
> + dpif_close(dpifp);
> + if (found) {
> + break;
> + }
> + }
> sset_destroy(&names);
>
> return found;
> ---
>
> >
> >> Something like this (not tested):
> >>
> >> diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c
> >> index 01e900461..b7b0616ec 100644
> >> --- a/lib/netdev-offload-dpdk.c
> >> +++ b/lib/netdev-offload-dpdk.c
> >> @@ -22,6 +22,7 @@
> >> #include "dpif-netdev.h"
> >> #include "netdev-offload-provider.h"
> >> #include "netdev-provider.h"
> >> +#include "netdev-vport.h"
> >> #include "openvswitch/match.h"
> >> #include "openvswitch/vlog.h"
> >> #include "packets.h"
> >> @@ -752,6 +753,13 @@ netdev_offload_dpdk_flow_del(struct netdev
> >> *netdev, const ovs_u128 *ufid, static int
> >> netdev_offload_dpdk_init_flow_api(struct netdev *netdev) {
> >> + if (netdev_vport_is_vport_class(netdev->netdev_class)
> >> + && netdev_vport_has_system_port(netdev)) {
> >> + VLOG_DBG("%s: vport has backing system interface. Skipping.",
> >> + netdev_get_name(netdev));
> >> + return EOPNOTSUPP;
> >> + }
> >> +
> >> return netdev_dpdk_flow_api_supported(netdev) ? 0 : EOPNOTSUPP;
> >> }
> >>
> >> diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c index
> >> 498aae369..e4d121ed7 100644
> >> --- a/lib/netdev-offload-tc.c
> >> +++ b/lib/netdev-offload-tc.c
> >> @@ -31,6 +31,7 @@
> >> #include "netdev-linux.h"
> >> #include "netdev-offload-provider.h"
> >> #include "netdev-provider.h"
> >> +#include "netdev-vport.h"
> >> #include "netlink.h"
> >> #include "netlink-socket.h"
> >> #include "odp-netlink.h"
> >> @@ -1560,6 +1561,13 @@ netdev_tc_init_flow_api(struct netdev
> *netdev)
> >> int ifindex;
> >> int error;
> >>
> >> + if (netdev_vport_is_vport_class(netdev->netdev_class)
> >> + && !netdev_vport_has_system_port(netdev)) {
> >> + VLOG_DBG("%s: vport has no backing system interface. Skipping.",
> >> + netdev_get_name(netdev));
> >> + return EOPNOTSUPP;
> >> + }
> >> +
> >> ifindex = netdev_get_ifindex(netdev);
> >> if (ifindex < 0) {
> >> VLOG_INFO("init: failed to get ifindex for %s: %s", diff
> >> --git a/lib/netdev- vport.c b/lib/netdev-vport.c index
> >> 92a256af1..82943c102 100644
> >> --- a/lib/netdev-vport.c
> >> +++ b/lib/netdev-vport.c
> >> @@ -44,6 +44,7 @@
> >> #include "simap.h"
> >> #include "smap.h"
> >> #include "socket-util.h"
> >> +#include "sset.h"
> >> #include "unaligned.h"
> >> #include "unixctl.h"
> >> #include "openvswitch/vlog.h"
> >> @@ -120,6 +121,21 @@ netdev_vport_class_get_dpif_port(const struct
> >> netdev_class *class)
> >> return is_vport_class(class) ?
> >> vport_class_cast(class)->dpif_port : NULL; }
> >>
> >> +bool
> >> +netdev_vport_has_system_port(const struct netdev *netdev) {
> >> + bool found;
> >> + struct sset names = SSET_INITIALIZER(&names);
> >> +
> >> + ovs_assert(is_vport_class(netdev_get_class(netdev)));
> >> +
> >> + dp_enumerate_names("system", &names);
> >> + found = sset_contains(&names, netdev_get_name(netdev));
> >> + sset_destroy(&names);
> >> +
> >> + return found;
> >> +}
> >> +
> >> const char *
> >> netdev_vport_get_dpif_port(const struct netdev *netdev,
> >> char namebuf[], size_t bufsize) diff
> >> --git a/lib/netdev-vport.h b/lib/netdev-vport.h index
> >> 9d756a265..4be1b92ec 100644
> >> --- a/lib/netdev-vport.h
> >> +++ b/lib/netdev-vport.h
> >> @@ -40,6 +40,7 @@ void netdev_vport_inc_tx(const struct netdev *,
> >> const struct dpif_flow_stats *);
> >>
> >> bool netdev_vport_is_vport_class(const struct netdev_class *);
> >> +bool netdev_vport_has_system_port(const struct netdev *);
> >> const char *netdev_vport_class_get_dpif_port(const struct
> >> netdev_class *);
> >>
> >> #ifndef _WIN32
> >> ---
> >>
> >> I'll format this as a proper patch and send along with patch for
> >> enabling offloading for ports without correct ifindex.
> >>
> >> Best regards, Ilya Maximets.
> >
> > Best regards,
> > Ophir
> >
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
