On Fri, Feb 03, 2017 at 02:11:34PM -0800, Joe Stringer wrote: > On 3 February 2017 at 12:56, Eric Garver <[email protected]> wrote: > > On Thu, Feb 02, 2017 at 02:50:01PM -0800, Joe Stringer wrote: > >> On 18 January 2017 at 11:45, Eric Garver <[email protected]> wrote: > >> > For out-of-tree datapath, only try genetlink/compat. > >> > For in-tree kernel datapath, try rtnetlink then genetlink. > >> > > >> > Signed-off-by: Eric Garver <[email protected]> > >> > --- > >> > lib/dpif-netlink.c | 35 ++++++++++++++++++++++++++++++++--- > >> > 1 file changed, 32 insertions(+), 3 deletions(-) > >> > > >> > diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c > >> > index e6459f358989..769806eadbf1 100644 > >> > --- a/lib/dpif-netlink.c > >> > +++ b/lib/dpif-netlink.c > >> > @@ -210,6 +210,11 @@ static int ovs_packet_family; > >> > * Initialized by dpif_netlink_init(). */ > >> > static unsigned int ovs_vport_mcgroup; > >> > > >> > +/* rtnetlink information for OVS. > >> > + * > >> > + * Initialized by dpif_netlink_init(). */ > >> > +static bool ovs_datapath_out_of_tree = false; > >> > >> Perhaps in the comment briefly mention that if this is true, devices > >> are created using OVS netlink and if it's false devices are created > >> using NETLINK_ROUTE / as LWT? > > > > I'll buff the comment to explain this. > > > >> > + > >> > static int dpif_netlink_init(void); > >> > static int open_dpif(const struct dpif_netlink_dp *, struct dpif **); > >> > static uint32_t dpif_netlink_port_get_pid(const struct dpif *, > >> > @@ -1014,11 +1019,13 @@ dpif_netlink_port_add(struct dpif *dpif_, struct > >> > netdev *netdev, > >> > odp_port_t *port_nop) > >> > { > >> > struct dpif_netlink *dpif = dpif_netlink_cast(dpif_); > >> > - int error; > >> > + int error = EOPNOTSUPP; > >> > > >> > fat_rwlock_wrlock(&dpif->upcall_lock); > >> > - error = dpif_netlink_port_create_and_add(dpif, netdev, port_nop); > >> > - if (error == EOPNOTSUPP) { > >> > + if (!ovs_datapath_out_of_tree) { > >> > + error = dpif_netlink_port_create_and_add(dpif, netdev, > >> > port_nop); > >> > + } > >> > + if (error) { > >> > error = dpif_netlink_port_add_compat(dpif, netdev, port_nop); > >> > } > >> > fat_rwlock_unlock(&dpif->upcall_lock); > >> > @@ -2503,6 +2510,7 @@ dpif_netlink_init(void) > >> > { > >> > static struct ovsthread_once once = OVSTHREAD_ONCE_INITIALIZER; > >> > static int error; > >> > + struct netdev *netdev = NULL; > >> > >> This can be in the #ifdef __linux__; otherwise you have an unused > >> variable on other platforms. > > > > Thanks for catching. I'll address that. > > > >> > > >> > if (ovsthread_once_start(&once)) { > >> > error = nl_lookup_genl_family(OVS_DATAPATH_FAMILY, > >> > @@ -2526,6 +2534,27 @@ dpif_netlink_init(void) > >> > error = nl_lookup_genl_mcgroup(OVS_VPORT_FAMILY, > >> > OVS_VPORT_MCGROUP, > >> > &ovs_vport_mcgroup); > >> > } > >> > +#ifdef __linux__ > >> > + if (!error) { > >> > + error = netdev_open("ovs-system-probe", "geneve", &netdev); > >> > + if (!error) { > >> > + error = netdev_geneve_create_kind(netdev, "ovs_geneve"); > >> > + if (error != EOPNOTSUPP) { > >> > + if (!error) { > >> > + char namebuf[NETDEV_VPORT_NAME_BUFSIZE]; > >> > + const char *dp_port; > >> > + > >> > + dp_port = netdev_vport_get_dpif_port(netdev, > >> > namebuf, > >> > + sizeof > >> > namebuf); > >> > + netdev_geneve_destroy(dp_port); > >> > + } > >> > + ovs_datapath_out_of_tree = true; > >> > + } > >> > + netdev_close(netdev); > >> > + error = 0; > >> > + } > >> > + } > >> > +#endif > >> > >> Geneve isn't added yet, so this patch introduces build breakage. > > > > Oops! I rearranged the patches before submitting. I'll move this patch > > to where I originally had it, after adding the newlink support. > > > >> This doesn't look like it matches the most recent discussion on this topic: > >> https://mail.openvswitch.org/pipermail/ovs-dev/2017-January/327476.html > > > > Correct. I opted not to do the LWT probe on init. We have to verify the > > device after creation anyways, which includes checking for LWT. It > > didn't seem like probing for LWT at init gained anything. > > OK, thanks for the explanation. I didn't have the full context to > review these patches so was partly going by just checking that the > feedback from the previous version was applied. > > If I follow correctly, the backported datapath will register to allow > device creation via NETLINK_ROUTE with the name ovs_foo for foo in > {geneve,gre,vxlan}; in this patch, if creation of such devices is > successful, or if it fails due to any reason other than EOPNOTSUPP, > then we will attempt to use these out of tree tunnel types? > > I guess the point is that the backported datapath can register to > allow such device creation, but it may fail since usually it's > configured over OVS genetlink; however, the failure wouldn't be > EOPNOTSUPP, it'd be something like EINVAL which still tells us that > the ovs_foo modules exist and therefore they should be used (but > configured over genetlink).
Yup. You nailed it. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
