On 7 February 2017 at 14:48, Eric Garver <e...@erig.me> wrote: > On Fri, Feb 03, 2017 at 02:11:34PM -0800, Joe Stringer wrote: >> On 3 February 2017 at 12:56, Eric Garver <e...@erig.me> wrote: >> > On Thu, Feb 02, 2017 at 02:50:01PM -0800, Joe Stringer wrote: >> >> On 18 January 2017 at 11:45, Eric Garver <e...@erig.me> wrote: >> >> > For out-of-tree datapath, only try genetlink/compat. >> >> > For in-tree kernel datapath, try rtnetlink then genetlink. >> >> > >> >> > Signed-off-by: Eric Garver <e...@erig.me> >> >> > --- >> >> > 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.
OK great. If you see a good spot in the code to put a brief explanation like this, that might help later readers. If there's no good spot, it could live in the patch summary. _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev