[ovs-dev] [PATCH] datapath: compat: fix kernel module reference count.

[Eric Garver]

On Fri, Nov 10, 2017 at 10:01:29AM -0800, William Tu wrote:
> On Fri, Nov 10, 2017 at 7:54 AM, Eric Garver <e at erig.me> wrote:
> > On Fri, Nov 10, 2017 at 05:36:25AM -0800, William Tu wrote:
> >> Before we introduce dpif-netlink-rtnl, creating a tunnel device
> >> depends on openvswitch/vport kernel module calling kernel's tunnel API.
> >> Example call sequence:
> >>  1) ovs_vport_add (openvswitch.ko)
> >>  2) geneve_create (vport_geneve.ko)
> >>  3) geneve_tnl_create (vport_geneve.ko)
> >>  4) geneve_dev_create_fb (geneve.ko)
> >> As a result, module dependency/refcnt is created: module openvswitch.ko
> >> references vport_geneve.ko and vport_geneve.ko depends on geneve.ko.
> >>
> >> After the dpif-netlink-rtnl, a tunnel device can be created by using
> >> rtnetlink, so the creation of fb device comes from the ovs-vswitchd
> >> instead of going through OVS kernel modules.  This breaks the module
> >> dependency between 1) and 2).  As a result, when ovs-vswitchd is running,
> >> the 'lsmod' shows:
> >>   geneve                 27326  1 vport_geneve
> >>   vport_geneve           12560  0
> >>   openvswitch           172469  1 vport_geneve
> >
> > Which is correct. vport_geneve is not using geneve. If anything is using
> > geneve, it's vport_netdev. But it already provides the proper linkage
> > via netdev_master_upper_dev_link().
> >
> >> Problems can be cerated by doing  `rmmod vport_geneve` then
> >> `rmmod geneve`, when ovs-vswitchd expects geneve module should already
> >> be loaded.
> >
> > Seems like this is not something that can happen by accident, but
> > requires the user to do something they shouldn't.
> >
> >>
> >> The patch introduces 1) load the upstream geneve.ko at init time
> >> from openvswitch.ko and 2) grap the reference count of geneve.
> >> This adds the dependency: openvswitch.ko needs geneve.ko, so that
> >> geneve won't be unloaded accidentally.  As a result, `lsmod` shows
> >>   geneve                 20254  2 vport_geneve
> >>   vport_geneve            1744  0
> >>   openvswitch           159846  1 vport_geneve
> >>
> >> Note that when choosing to use upstream tunnel, we don't actually need
> >> vport_geneve, so the patch increment refcnt of geneve instead of
> >> vport_geneve.
> >
> > I don't think inserting an artificial dependency is the right answer.
> > Although I'm not sure what problem you're trying to solve. Can you help
> > me understand the issue here?
> 
> yes, this is an artificial dependency. Another way I'm thinking is for
> ovs-vswitchd
>  to hold the geneve.ko dependency instead of openvswitch.ko, when user creates
> a geneve device. Is there a way to do that through rtnetlink or at
> dpif_netlink_rtnl_create()?

It should be vport-netdev doing the magic. It creates the linkage with
netdev_master_upper_dev_link(), but apparently that is not enough to
bump the kernel module refcnt and thus prevent unloading. Maybe you can
dig into why that is. AFAICS, it should be linking openvswitch.ko and
and geneve.ko with that call.

> >
> > This patch also only address GENEVE, what about VXLAN and GRE?
> >
> 
> Yes, we will need to address vxlan and gre later.

vport-netdev.c should be the one that holds onto the underlying device.
As such, this should be common and take care of all three.