[ovs-dev] [RFC PATCH v4 3/6] dpif-netlink: Probe for out-of-tree datapath.

Joe Stringer joe at ovn.org
Wed Feb 8 00:00:52 UTC 2017


On 7 February 2017 at 14:48, Eric Garver <e at 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 at 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 at 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 at 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.


More information about the dev mailing list