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

[William Tu]

>>
>> That is always been behavior of the port lifecycle. If you remove the
>> driver of a device, port is removed from kernel datapath and the
>> device is deleted from kernel networking stack. For example you can
>> remove intel NIC driver, or veth module even if there is a
>> corresponding port in OVS bridge. I am not sure why tunnel device is
>> special case.
>>
>
>  It must have changed sometime then (or my observation is incorrect).
> Because in OVS 2.7, we can't remove "vport-geneve" when a geneve tunnel has
> been created.

Right, that's because in OVS2.7 without dpif-netlink-rtnl, geneve
tunnel is created through
vport_geneve (the geneve_dev_create_fb function) using openvswitch.ko, so
openvswitch.ko hold the reference.

On OVS2.7 without dpif-netlink-rtnl, kernel 4.8
lsmod
  vport_geneve1744  1   (ref by openvswitch.ko)
  geneve 20254  1 vport_geneve
  openvswitch   128504  3 vport_geneve
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

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

[Guru Shetty]

On 13 November 2017 at 05:01, Pravin Shelar  wrote:

> On Sat, Nov 11, 2017 at 11:53 PM, Guru Shetty  wrote:
> > On 10 November 2017 at 22:54, Gregory Rose  wrote:
> >
> >> On 11/11/2017 9:57 AM, William Tu wrote:
> >>
> >>> 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.
> 
> >>> Sure, will take a look.
> >>>
> >>
> >> I'm extremely uncomfortable with this approach of manually bumping a
> >> driver refcnt.
> >> I worry we're going to get into a situation where the driver won't
> >> *unload*.  In a properly
> >> configured system this shouldn't be necessary.  Driver dependencies
> >> shouldn't have to
> >> be manually configured.
> >>
> > Hypothetically, let us assume that we use only upstream "geneve". Once we
> > create tunnel via it, it shouldn't be possible to remove it. But right
> now,
> > we can. And I think that is wrong.
> >
>
> That is always been behavior of the port lifecycle. If you remove the
> driver of a device, port is removed from kernel datapath and the
> device is deleted from kernel networking stack. For example you can
> remove intel NIC driver, or veth module even if there is a
> corresponding port in OVS bridge. I am not sure why tunnel device is
> special case.
>

 It must have changed sometime then (or my observation is incorrect).
Because in OVS 2.7, we can't remove "vport-geneve" when a geneve tunnel has
been created.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

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

[William Tu]

On Mon, Nov 13, 2017 at 5:01 AM, Pravin Shelar  wrote:
> On Sat, Nov 11, 2017 at 11:53 PM, Guru Shetty  wrote:
>> On 10 November 2017 at 22:54, Gregory Rose  wrote:
>>
>>> On 11/11/2017 9:57 AM, William Tu wrote:
>>>
 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.
>
 Sure, will take a look.

>>>
>>> I'm extremely uncomfortable with this approach of manually bumping a
>>> driver refcnt.
>>> I worry we're going to get into a situation where the driver won't
>>> *unload*.  In a properly
>>> configured system this shouldn't be necessary.  Driver dependencies
>>> shouldn't have to
>>> be manually configured.
>>>
>> Hypothetically, let us assume that we use only upstream "geneve". Once we
>> create tunnel via it, it shouldn't be possible to remove it. But right now,
>> we can. And I think that is wrong.
>>
>
> That is always been behavior of the port lifecycle. If you remove the
> driver of a device, port is removed from kernel datapath and the
> device is deleted from kernel networking stack. For example you can
> remove intel NIC driver, or veth module even if there is a
> corresponding port in OVS bridge. I am not sure why tunnel device is
> special case.

agree. in this case we allow the driver to be unloaded and since openvswitch.ko
registers the NETDEV_UNREGISTER notifier, the ovs-vswtichd gets notified
that the port is removed.

I also check Eric's suggestion about the upper dev approach
(netdev_master_upper_dev_link) and although the tunnel port is the lower dev,
having an upper device does not prevent us from uploading the lower
device's driver.

Since this is triggered by upgrading OVS using ovs-ctl force-reload-kmod,
Guru has submitted another approach:
https://mail.openvswitch.org/pipermail/ovs-dev/2017-November/340835.html

Regards,
William
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

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

[Pravin Shelar]

On Sat, Nov 11, 2017 at 11:53 PM, Guru Shetty  wrote:
> On 10 November 2017 at 22:54, Gregory Rose  wrote:
>
>> On 11/11/2017 9:57 AM, William Tu wrote:
>>
>>> 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.

>>> Sure, will take a look.
>>>
>>
>> I'm extremely uncomfortable with this approach of manually bumping a
>> driver refcnt.
>> I worry we're going to get into a situation where the driver won't
>> *unload*.  In a properly
>> configured system this shouldn't be necessary.  Driver dependencies
>> shouldn't have to
>> be manually configured.
>>
> Hypothetically, let us assume that we use only upstream "geneve". Once we
> create tunnel via it, it shouldn't be possible to remove it. But right now,
> we can. And I think that is wrong.
>

That is always been behavior of the port lifecycle. If you remove the
driver of a device, port is removed from kernel datapath and the
device is deleted from kernel networking stack. For example you can
remove intel NIC driver, or veth module even if there is a
corresponding port in OVS bridge. I am not sure why tunnel device is
special case.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

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

[Gregory Rose]

On 11/12/2017 3:23 AM, Guru Shetty wrote:



On 10 November 2017 at 22:54, Gregory Rose > wrote:


On 11/11/2017 9:57 AM, William Tu wrote:

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.

Sure, will take a look.


I'm extremely uncomfortable with this approach of manually bumping
a driver refcnt.
I worry we're going to get into a situation where the driver won't
*unload*.  In a properly
configured system this shouldn't be necessary.  Driver
dependencies shouldn't have to
be manually configured.

Hypothetically, let us assume that we use only upstream "geneve". Once 
we create tunnel via it, it shouldn't be possible to remove it. But 
right now, we can. And I think that is wrong.


I can't disagree with that. I have no technical objection to  this 
approach but also can't remember any issue like this in the past. I'd 
feel better if we had some evidence that this is a problem that's been 
encountered before.


As it stands I can't object though.



Why do we have a vport-geneve driver loaded when there is a geneve
driver already
built in to the kernel?

OVS startup scripts will load one, if previously there was one used. 
This happens when we upgrade from 2.7 to 2.8. There is a patch 
proposed to remove that option.


Now that sounds promising.

Thanks,

- Greg


  What is the OVS vport-geneve driver providing that the built-in
kernel geneve driver does not provide?

- Greg




___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

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

[Guru Shetty]

On 10 November 2017 at 22:54, Gregory Rose  wrote:

> On 11/11/2017 9:57 AM, William Tu wrote:
>
>> 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.
>>>
>> Sure, will take a look.
>>
>
> I'm extremely uncomfortable with this approach of manually bumping a
> driver refcnt.
> I worry we're going to get into a situation where the driver won't
> *unload*.  In a properly
> configured system this shouldn't be necessary.  Driver dependencies
> shouldn't have to
> be manually configured.
>
Hypothetically, let us assume that we use only upstream "geneve". Once we
create tunnel via it, it shouldn't be possible to remove it. But right now,
we can. And I think that is wrong.


>
> Why do we have a vport-geneve driver loaded when there is a geneve driver
> already
> built in to the kernel?

OVS startup scripts will load one, if previously there was one used. This
happens when we upgrade from 2.7 to 2.8. There is a patch proposed to
remove that option.


>   What is the OVS vport-geneve driver providing that the built-in
> kernel geneve driver does not provide?
>
> - Greg
>
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

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

[Gregory Rose]

On 11/11/2017 9:57 AM, William Tu wrote:

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.

Sure, will take a look.


I'm extremely uncomfortable with this approach of manually bumping a 
driver refcnt.
I worry we're going to get into a situation where the driver won't 
*unload*.  In a properly
configured system this shouldn't be necessary.  Driver dependencies 
shouldn't have to

be manually configured.

Why do we have a vport-geneve driver loaded when there is a geneve 
driver already
built in to the kernel?  What is the OVS vport-geneve driver providing 
that the built-in

kernel geneve driver does not provide?

- Greg

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

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

[William Tu]

>> 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.

Sure, will take a look.
>
>> >
>> > 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.

Agree, that's a better place to take care of all tunnels.

William
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [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  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_geneve1744  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.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

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

[Eric Garver]

On Fri, Nov 10, 2017 at 12:23:37PM -0800, William Tu wrote:
> >> 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
> 
> Actually vport_geneve is using geneve. (Please correct me if I'm wrong)
> If we're using upstream tunnel (USE_UPSTREAM_TUNNEL=yes)
> then vport_geneve calls into kernel's geneve_dev_create_fb(), so it
> has symbol dependency.

Sure, but by that logic the compat interface is also broken in the same
way. I think if you start OVS with an empty config (no geneve tunnel
created), then you should have the same ref counts as above. You can
manually unload vport_geneve.

When USE_UPSTREAM_TUNNEL=yes OVS _does not_ functionally use
vport_geneve. It gets loaded because that's what OVS startup scripts do.
You can delete vport_geneve.ko and everything with still function with
the rtnetlink interface. I suggest renaming them, running depmod, and
giving it a try.

> If USE_UPSTREAM_TUNNEL=no, then geneve_dev_create_fb is replaced with rpl_xxx,
> then no dependency.

Agree. No dependency on in-kernel geneve driver.

> Can we say that if (OVS version == 2.8 && USE_UPSTREAM_TUNNEL=yes),
> then we don't need any vport-xxx implementation and we can remove these code?

Yes! But we still support older kernels (I think < 4.5-ish) that don't
support all the necessary features for tunnels. As such the OVS shipped
drivers will be around for awhile.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

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

[William Tu]

On Fri, Nov 10, 2017 at 7:54 AM, Eric Garver  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_geneve1744  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()?

>
> This patch also only address GENEVE, what about VXLAN and GRE?
>

Yes, we will need to address vxlan and gre later.

Thanks for the comments.
William
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

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

[Guru Shetty]

On 10 November 2017 at 07:54, Eric Garver  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.
>

This was observed in a real world case.
1. Take a RHEL7.3 host
1. Upgrade OVS RHEL packages from 2.7 to 2.8 (including kernel module
package).
2. Upgrade RHEL7.3 to RHEL7.4

When iptables gets upgraded in step 3, it calls:
/usr/libexec/iptables/ip6tables.init stop

That script tries to remove all kernel modules that are dependent on
conntrack. So it tries to remove openvswitch and vport-geneve. openvswitch
does not get removed as it has a reference count. But a 'modprobe -r
vport-geneve' will remove "geneve", which in turn deletes the geneve device
leaving the tunnels down.



> >
> > 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_geneve1744  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?
>
> This patch also only address GENEVE, what about VXLAN and GRE?
>
> >
> > Fixes: b6d6830d29e4 ("dpif-netlink-rtnl: Support GENEVE creation")
> > Signed-off-by: William Tu 
> > Cc: Eric Garver 
> > ---
> [...]
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

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

[Eric Garver]

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_geneve1744  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?

This patch also only address GENEVE, what about VXLAN and GRE?

> 
> Fixes: b6d6830d29e4 ("dpif-netlink-rtnl: Support GENEVE creation")
> Signed-off-by: William Tu 
> Cc: Eric Garver 
> ---
[...]
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev