* [PATCH net-next 0/2] net: break dependency of drivers on geneve and vxlan
@ 2016-01-06 15:41 Hannes Frederic Sowa
2016-01-06 15:41 ` [PATCH net-next 1/2] vxlan: break dependency to network drivers Hannes Frederic Sowa
2016-01-06 15:41 ` [PATCH net-next 2/2] geneve: " Hannes Frederic Sowa
0 siblings, 2 replies; 9+ messages in thread
From: Hannes Frederic Sowa @ 2016-01-06 15:41 UTC (permalink / raw)
To: netdev
Device drivers which support geneve or vxlan offloading have a dependency
on the correlating kernel modules. Thus those drivers automatically loaded
the geneve or vxlan modules. Break this dependency with this small series.
Hannes Frederic Sowa (2):
vxlan: break dependency to network drivers
geneve: break dependency to network drivers
drivers/net/geneve.c | 30 +++++++++++++++++++++++++++---
drivers/net/vxlan.c | 17 +++++++++++------
include/linux/netdevice.h | 2 ++
include/net/geneve.h | 7 +++----
include/net/vxlan.h | 5 +----
5 files changed, 44 insertions(+), 17 deletions(-)
--
2.5.0
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH net-next 1/2] vxlan: break dependency to network drivers
2016-01-06 15:41 [PATCH net-next 0/2] net: break dependency of drivers on geneve and vxlan Hannes Frederic Sowa
@ 2016-01-06 15:41 ` Hannes Frederic Sowa
2016-01-06 15:41 ` [PATCH net-next 2/2] geneve: " Hannes Frederic Sowa
1 sibling, 0 replies; 9+ messages in thread
From: Hannes Frederic Sowa @ 2016-01-06 15:41 UTC (permalink / raw)
To: netdev
Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
---
drivers/net/vxlan.c | 17 +++++++++++------
include/linux/netdevice.h | 1 +
include/net/vxlan.h | 5 +----
3 files changed, 13 insertions(+), 10 deletions(-)
diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
index fecf7b6c732e96..fbfac24238c06c 100644
--- a/drivers/net/vxlan.c
+++ b/drivers/net/vxlan.c
@@ -2468,7 +2468,7 @@ static struct device_type vxlan_type = {
* supply the listening VXLAN udp ports. Callers are expected
* to implement the ndo_add_vxlan_port.
*/
-void vxlan_get_rx_port(struct net_device *dev)
+static void vxlan_notify_refresh_netdev(struct net_device *dev)
{
struct vxlan_sock *vs;
struct net *net = dev_net(dev);
@@ -2488,7 +2488,6 @@ void vxlan_get_rx_port(struct net_device *dev)
}
spin_unlock(&vn->sock_lock);
}
-EXPORT_SYMBOL_GPL(vxlan_get_rx_port);
/* Initialize the device structure. */
static void vxlan_setup(struct net_device *dev)
@@ -3164,20 +3163,26 @@ static void vxlan_handle_lowerdev_unregister(struct vxlan_net *vn,
unregister_netdevice_many(&list_kill);
}
-static int vxlan_lowerdev_event(struct notifier_block *unused,
- unsigned long event, void *ptr)
+static int vxlan_notifier(struct notifier_block *unused,
+ unsigned long event, void *ptr)
{
struct net_device *dev = netdev_notifier_info_to_dev(ptr);
struct vxlan_net *vn = net_generic(dev_net(dev), vxlan_net_id);
- if (event == NETDEV_UNREGISTER)
+ switch (event) {
+ case NETDEV_REFRESH_OFFLOAD_VXLAN:
+ vxlan_notify_refresh_netdev(dev);
+ break;
+ case NETDEV_UNREGISTER:
vxlan_handle_lowerdev_unregister(vn, dev);
+ break;
+ }
return NOTIFY_DONE;
}
static struct notifier_block vxlan_notifier_block __read_mostly = {
- .notifier_call = vxlan_lowerdev_event,
+ .notifier_call = vxlan_notifier,
};
static __net_init int vxlan_init_net(struct net *net)
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index c20b814e46a072..d57d5074b33315 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -2183,6 +2183,7 @@ struct netdev_lag_lower_state_info {
#define NETDEV_BONDING_INFO 0x0019
#define NETDEV_PRECHANGEUPPER 0x001A
#define NETDEV_CHANGELOWERSTATE 0x001B
+#define NETDEV_REFRESH_OFFLOAD_VXLAN 0x001C
int register_netdevice_notifier(struct notifier_block *nb);
int unregister_netdevice_notifier(struct notifier_block *nb);
diff --git a/include/net/vxlan.h b/include/net/vxlan.h
index 0fb86442544b26..2b5617850e4097 100644
--- a/include/net/vxlan.h
+++ b/include/net/vxlan.h
@@ -242,13 +242,10 @@ static inline netdev_features_t vxlan_features_check(struct sk_buff *skb,
/* IPv6 header + UDP + VXLAN + Ethernet header */
#define VXLAN6_HEADROOM (40 + 8 + 8 + 14)
-#if IS_ENABLED(CONFIG_VXLAN)
-void vxlan_get_rx_port(struct net_device *netdev);
-#else
static inline void vxlan_get_rx_port(struct net_device *netdev)
{
+ call_netdevice_notifiers(NETDEV_REFRESH_OFFLOAD_VXLAN, netdev);
}
-#endif
static inline unsigned short vxlan_get_sk_family(struct vxlan_sock *vs)
{
--
2.5.0
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH net-next 2/2] geneve: break dependency to network drivers
2016-01-06 15:41 [PATCH net-next 0/2] net: break dependency of drivers on geneve and vxlan Hannes Frederic Sowa
2016-01-06 15:41 ` [PATCH net-next 1/2] vxlan: break dependency to network drivers Hannes Frederic Sowa
@ 2016-01-06 15:41 ` Hannes Frederic Sowa
2016-01-06 18:00 ` Jesse Gross
1 sibling, 1 reply; 9+ messages in thread
From: Hannes Frederic Sowa @ 2016-01-06 15:41 UTC (permalink / raw)
To: netdev
Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
---
drivers/net/geneve.c | 30 +++++++++++++++++++++++++++---
include/linux/netdevice.h | 1 +
include/net/geneve.h | 7 +++----
3 files changed, 31 insertions(+), 7 deletions(-)
diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c
index 24b077a32c1c9c..548925d1571cb1 100644
--- a/drivers/net/geneve.c
+++ b/drivers/net/geneve.c
@@ -1110,7 +1110,7 @@ static struct device_type geneve_type = {
* supply the listening GENEVE udp ports. Callers are expected
* to implement the ndo_add_geneve_port.
*/
-void geneve_get_rx_port(struct net_device *dev)
+static void geneve_notify_refresh_netdev(struct net_device *dev)
{
struct net *net = dev_net(dev);
struct geneve_net *gn = net_generic(net, geneve_net_id);
@@ -1128,7 +1128,6 @@ void geneve_get_rx_port(struct net_device *dev)
}
rcu_read_unlock();
}
-EXPORT_SYMBOL_GPL(geneve_get_rx_port);
/* Initialize the device structure. */
static void geneve_setup(struct net_device *dev)
@@ -1450,6 +1449,24 @@ struct net_device *geneve_dev_create_fb(struct net *net, const char *name,
}
EXPORT_SYMBOL_GPL(geneve_dev_create_fb);
+static int geneve_notifier(struct notifier_block *unused,
+ unsigned long event, void *ptr)
+{
+ struct net_device *dev = netdev_notifier_info_to_dev(ptr);
+
+ switch (event) {
+ case NETDEV_REFRESH_OFFLOAD_VXLAN:
+ geneve_notify_refresh_netdev(dev);
+ break;
+ }
+
+ return NOTIFY_DONE;
+}
+
+static struct notifier_block geneve_notifier_block __read_mostly = {
+ .notifier_call = geneve_notifier,
+};
+
static __net_init int geneve_init_net(struct net *net)
{
struct geneve_net *gn = net_generic(net, geneve_net_id);
@@ -1502,11 +1519,17 @@ static int __init geneve_init_module(void)
if (rc)
goto out1;
- rc = rtnl_link_register(&geneve_link_ops);
+ rc = register_netdevice_notifier(&geneve_notifier_block);
if (rc)
goto out2;
+ rc = rtnl_link_register(&geneve_link_ops);
+ if (rc)
+ goto out3;
+
return 0;
+out3:
+ unregister_netdevice_notifier(&geneve_notifier_block);
out2:
unregister_pernet_subsys(&geneve_net_ops);
out1:
@@ -1517,6 +1540,7 @@ late_initcall(geneve_init_module);
static void __exit geneve_cleanup_module(void)
{
rtnl_link_unregister(&geneve_link_ops);
+ unregister_netdevice_notifier(&geneve_notifier_block);
unregister_pernet_subsys(&geneve_net_ops);
}
module_exit(geneve_cleanup_module);
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index d57d5074b33315..ee3a7cffadbdc7 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -2184,6 +2184,7 @@ struct netdev_lag_lower_state_info {
#define NETDEV_PRECHANGEUPPER 0x001A
#define NETDEV_CHANGELOWERSTATE 0x001B
#define NETDEV_REFRESH_OFFLOAD_VXLAN 0x001C
+#define NETDEV_REFRESH_OFFLOAD_GENEVE 0x001D
int register_netdevice_notifier(struct notifier_block *nb);
int unregister_netdevice_notifier(struct notifier_block *nb);
diff --git a/include/net/geneve.h b/include/net/geneve.h
index e6c23dc765f7ec..36245115143652 100644
--- a/include/net/geneve.h
+++ b/include/net/geneve.h
@@ -1,6 +1,8 @@
#ifndef __NET_GENEVE_H
#define __NET_GENEVE_H 1
+#include <linux/netdevice.h>
+
#ifdef CONFIG_INET
#include <net/udp_tunnel.h>
#endif
@@ -62,13 +64,10 @@ struct genevehdr {
struct geneve_opt options[];
};
-#if IS_ENABLED(CONFIG_GENEVE)
-void geneve_get_rx_port(struct net_device *netdev);
-#else
static inline void geneve_get_rx_port(struct net_device *netdev)
{
+ call_netdevice_notifiers(NETDEV_REFRESH_OFFLOAD_GENEVE, netdev);
}
-#endif
#ifdef CONFIG_INET
struct net_device *geneve_dev_create_fb(struct net *net, const char *name,
--
2.5.0
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH net-next 2/2] geneve: break dependency to network drivers
2016-01-06 15:41 ` [PATCH net-next 2/2] geneve: " Hannes Frederic Sowa
@ 2016-01-06 18:00 ` Jesse Gross
2016-01-06 18:48 ` Hannes Frederic Sowa
0 siblings, 1 reply; 9+ messages in thread
From: Jesse Gross @ 2016-01-06 18:00 UTC (permalink / raw)
To: Hannes Frederic Sowa; +Cc: Linux Kernel Network Developers
On Wed, Jan 6, 2016 at 7:41 AM, Hannes Frederic Sowa
<hannes@stressinduktion.org> wrote:
> diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c
> index 24b077a32c1c9c..548925d1571cb1 100644
> --- a/drivers/net/geneve.c
> +++ b/drivers/net/geneve.c
> +static int geneve_notifier(struct notifier_block *unused,
> + unsigned long event, void *ptr)
> +{
> + struct net_device *dev = netdev_notifier_info_to_dev(ptr);
> +
> + switch (event) {
> + case NETDEV_REFRESH_OFFLOAD_VXLAN:
> + geneve_notify_refresh_netdev(dev);
Presumably this should be NETDEV_REFRESH_OFFLOAD_GENEVE, not VXLAN.
However, rather than having a notifier for each protocol, it seems
like it might be cleaner to just have a single one that triggers for
all protocols and drivers that don't have the corresponding NDO
wouldn't get called, similar to what happens when the port gets added
in the first place.
> diff --git a/include/net/geneve.h b/include/net/geneve.h
> index e6c23dc765f7ec..36245115143652 100644
> --- a/include/net/geneve.h
> +++ b/include/net/geneve.h
[...]
> static inline void geneve_get_rx_port(struct net_device *netdev)
> {
> + call_netdevice_notifiers(NETDEV_REFRESH_OFFLOAD_GENEVE, netdev);
> }
Unfortunately, I don't think that we can assume that RTNL is held
here. It actually is for the drivers that implement Geneve at this
point but not in all cases for VXLAN. For example, ixgbe refreshes the
offloads in a service task in addition to when it is opened. There's
only a couple instances of things like this, so I guess it's probably
not too hard to through and make sure that we hold RTNL in those
cases.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH net-next 2/2] geneve: break dependency to network drivers
2016-01-06 18:00 ` Jesse Gross
@ 2016-01-06 18:48 ` Hannes Frederic Sowa
2016-01-06 19:52 ` Jesse Gross
0 siblings, 1 reply; 9+ messages in thread
From: Hannes Frederic Sowa @ 2016-01-06 18:48 UTC (permalink / raw)
To: Jesse Gross; +Cc: Linux Kernel Network Developers
On 06.01.2016 19:00, Jesse Gross wrote:
> On Wed, Jan 6, 2016 at 7:41 AM, Hannes Frederic Sowa
> <hannes@stressinduktion.org> wrote:
>> diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c
>> index 24b077a32c1c9c..548925d1571cb1 100644
>> --- a/drivers/net/geneve.c
>> +++ b/drivers/net/geneve.c
>> +static int geneve_notifier(struct notifier_block *unused,
>> + unsigned long event, void *ptr)
>> +{
>> + struct net_device *dev = netdev_notifier_info_to_dev(ptr);
>> +
>> + switch (event) {
>> + case NETDEV_REFRESH_OFFLOAD_VXLAN:
>> + geneve_notify_refresh_netdev(dev);
>
> Presumably this should be NETDEV_REFRESH_OFFLOAD_GENEVE, not VXLAN.
> However, rather than having a notifier for each protocol, it seems
> like it might be cleaner to just have a single one that triggers for
> all protocols and drivers that don't have the corresponding NDO
> wouldn't get called, similar to what happens when the port gets added
> in the first place.
Ah, thanks for noticing the typo.
The reason why I went with several types is that I didn't want to change
the behavior and wasn't sure if driver tested with reoccurring offload
refreshes to the driver. What you described was my first patch but
because I couldn't see if that works for all drivers I went this way.
>> diff --git a/include/net/geneve.h b/include/net/geneve.h
>> index e6c23dc765f7ec..36245115143652 100644
>> --- a/include/net/geneve.h
>> +++ b/include/net/geneve.h
> [...]
>> static inline void geneve_get_rx_port(struct net_device *netdev)
>> {
>> + call_netdevice_notifiers(NETDEV_REFRESH_OFFLOAD_GENEVE, netdev);
>> }
>
> Unfortunately, I don't think that we can assume that RTNL is held
> here. It actually is for the drivers that implement Geneve at this
> point but not in all cases for VXLAN. For example, ixgbe refreshes the
> offloads in a service task in addition to when it is opened. There's
> only a couple instances of things like this, so I guess it's probably
> not too hard to through and make sure that we hold RTNL in those
> cases.
Hmm, I am tempted to switch over to the netevent atomic notifier chain
and install those events there. It does not need rtnl lock at all, so we
can preserve the current semantics. What do you think?
Thanks,
Hannes
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH net-next 2/2] geneve: break dependency to network drivers
2016-01-06 18:48 ` Hannes Frederic Sowa
@ 2016-01-06 19:52 ` Jesse Gross
2016-01-06 20:25 ` Hannes Frederic Sowa
0 siblings, 1 reply; 9+ messages in thread
From: Jesse Gross @ 2016-01-06 19:52 UTC (permalink / raw)
To: Hannes Frederic Sowa; +Cc: Linux Kernel Network Developers
On Wed, Jan 6, 2016 at 10:48 AM, Hannes Frederic Sowa
<hannes@stressinduktion.org> wrote:
> On 06.01.2016 19:00, Jesse Gross wrote:
>>
>> On Wed, Jan 6, 2016 at 7:41 AM, Hannes Frederic Sowa
>> <hannes@stressinduktion.org> wrote:
>>>
>>> diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c
>>> index 24b077a32c1c9c..548925d1571cb1 100644
>>> --- a/drivers/net/geneve.c
>>> +++ b/drivers/net/geneve.c
>>> +static int geneve_notifier(struct notifier_block *unused,
>>> + unsigned long event, void *ptr)
>>> +{
>>> + struct net_device *dev = netdev_notifier_info_to_dev(ptr);
>>> +
>>> + switch (event) {
>>> + case NETDEV_REFRESH_OFFLOAD_VXLAN:
>>> + geneve_notify_refresh_netdev(dev);
>>
>>
>> Presumably this should be NETDEV_REFRESH_OFFLOAD_GENEVE, not VXLAN.
>> However, rather than having a notifier for each protocol, it seems
>> like it might be cleaner to just have a single one that triggers for
>> all protocols and drivers that don't have the corresponding NDO
>> wouldn't get called, similar to what happens when the port gets added
>> in the first place.
>
>
> Ah, thanks for noticing the typo.
>
> The reason why I went with several types is that I didn't want to change the
> behavior and wasn't sure if driver tested with reoccurring offload refreshes
> to the driver. What you described was my first patch but because I couldn't
> see if that works for all drivers I went this way.
Hmm, I see what you mean but I think it should be safe. All drivers
that have both Geneve and VXLAN offloads make calls to refresh them
back to back, which is a pattern that I would expect to continue. In
that case, having a single notifier that triggers multiple protocols
would have the same effect and is simpler.
>>> diff --git a/include/net/geneve.h b/include/net/geneve.h
>>> index e6c23dc765f7ec..36245115143652 100644
>>> --- a/include/net/geneve.h
>>> +++ b/include/net/geneve.h
>>
>> [...]
>>>
>>> static inline void geneve_get_rx_port(struct net_device *netdev)
>>> {
>>> + call_netdevice_notifiers(NETDEV_REFRESH_OFFLOAD_GENEVE, netdev);
>>> }
>>
>>
>> Unfortunately, I don't think that we can assume that RTNL is held
>> here. It actually is for the drivers that implement Geneve at this
>> point but not in all cases for VXLAN. For example, ixgbe refreshes the
>> offloads in a service task in addition to when it is opened. There's
>> only a couple instances of things like this, so I guess it's probably
>> not too hard to through and make sure that we hold RTNL in those
>> cases.
>
>
> Hmm, I am tempted to switch over to the netevent atomic notifier chain and
> install those events there. It does not need rtnl lock at all, so we can
> preserve the current semantics. What do you think?
I think that holding RTNL while we do these updates is actually the
right thing to do. The current situation of having calls from
different protocols protected by different locks is not really a great
model given that at the driver level these are usually shared data
structures. RTNL is already held in the majority of cases already, so
I think it is better to just convert the rest.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH net-next 2/2] geneve: break dependency to network drivers
2016-01-06 19:52 ` Jesse Gross
@ 2016-01-06 20:25 ` Hannes Frederic Sowa
2016-01-06 21:01 ` Jesse Gross
0 siblings, 1 reply; 9+ messages in thread
From: Hannes Frederic Sowa @ 2016-01-06 20:25 UTC (permalink / raw)
To: Jesse Gross; +Cc: Linux Kernel Network Developers
Hi Jesse,
hmpf, I saw your mail too late and send out another series just now.
On 06.01.2016 20:52, Jesse Gross wrote:
> On Wed, Jan 6, 2016 at 10:48 AM, Hannes Frederic Sowa
> <hannes@stressinduktion.org> wrote:
>> On 06.01.2016 19:00, Jesse Gross wrote:
>>>
>>> On Wed, Jan 6, 2016 at 7:41 AM, Hannes Frederic Sowa
>>> <hannes@stressinduktion.org> wrote:
>>>>
>>>> diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c
>>>> index 24b077a32c1c9c..548925d1571cb1 100644
>>>> --- a/drivers/net/geneve.c
>>>> +++ b/drivers/net/geneve.c
>>>> +static int geneve_notifier(struct notifier_block *unused,
>>>> + unsigned long event, void *ptr)
>>>> +{
>>>> + struct net_device *dev = netdev_notifier_info_to_dev(ptr);
>>>> +
>>>> + switch (event) {
>>>> + case NETDEV_REFRESH_OFFLOAD_VXLAN:
>>>> + geneve_notify_refresh_netdev(dev);
>>>
>>>
>>> Presumably this should be NETDEV_REFRESH_OFFLOAD_GENEVE, not VXLAN.
>>> However, rather than having a notifier for each protocol, it seems
>>> like it might be cleaner to just have a single one that triggers for
>>> all protocols and drivers that don't have the corresponding NDO
>>> wouldn't get called, similar to what happens when the port gets added
>>> in the first place.
>>
>>
>> Ah, thanks for noticing the typo.
>>
>> The reason why I went with several types is that I didn't want to change the
>> behavior and wasn't sure if driver tested with reoccurring offload refreshes
>> to the driver. What you described was my first patch but because I couldn't
>> see if that works for all drivers I went this way.
>
> Hmm, I see what you mean but I think it should be safe. All drivers
> that have both Geneve and VXLAN offloads make calls to refresh them
> back to back, which is a pattern that I would expect to continue. In
> that case, having a single notifier that triggers multiple protocols
> would have the same effect and is simpler.
My new series adds a new netdev_notifier which is atomic and can deal
with non locked rtnl events. I thought maybe it would be useful for
future callbacks, too. I am currently looking into ptp and reduce the
dependency there, if possible.
>>>> diff --git a/include/net/geneve.h b/include/net/geneve.h
>>>> index e6c23dc765f7ec..36245115143652 100644
>>>> --- a/include/net/geneve.h
>>>> +++ b/include/net/geneve.h
>>>
>>> [...]
>>>>
>>>> static inline void geneve_get_rx_port(struct net_device *netdev)
>>>> {
>>>> + call_netdevice_notifiers(NETDEV_REFRESH_OFFLOAD_GENEVE, netdev);
>>>> }
>>>
>>>
>>> Unfortunately, I don't think that we can assume that RTNL is held
>>> here. It actually is for the drivers that implement Geneve at this
>>> point but not in all cases for VXLAN. For example, ixgbe refreshes the
>>> offloads in a service task in addition to when it is opened. There's
>>> only a couple instances of things like this, so I guess it's probably
>>> not too hard to through and make sure that we hold RTNL in those
>>> cases.
>>
>>
>> Hmm, I am tempted to switch over to the netevent atomic notifier chain and
>> install those events there. It does not need rtnl lock at all, so we can
>> preserve the current semantics. What do you think?
>
> I think that holding RTNL while we do these updates is actually the
> right thing to do. The current situation of having calls from
> different protocols protected by different locks is not really a great
> model given that at the driver level these are usually shared data
> structures. RTNL is already held in the majority of cases already, so
> I think it is better to just convert the rest.
The refreshes from each module are completely synchronous and don't get
interleaved, so as long as the driver is correctly handling the locking
internally rtnl lock shouldn't be needed. But as I don't know too much
about driver developing I can revisit this.
As a advantage I see that the driver developers don't need to worry
about the rtnl lock at all when adding new events. Is this realistic?
Thanks,
Hannes
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH net-next 2/2] geneve: break dependency to network drivers
2016-01-06 20:25 ` Hannes Frederic Sowa
@ 2016-01-06 21:01 ` Jesse Gross
2016-01-06 21:18 ` Hannes Frederic Sowa
0 siblings, 1 reply; 9+ messages in thread
From: Jesse Gross @ 2016-01-06 21:01 UTC (permalink / raw)
To: Hannes Frederic Sowa; +Cc: Linux Kernel Network Developers
On Wed, Jan 6, 2016 at 12:25 PM, Hannes Frederic Sowa
<hannes@stressinduktion.org> wrote:
> On 06.01.2016 20:52, Jesse Gross wrote:
>> On Wed, Jan 6, 2016 at 10:48 AM, Hannes Frederic Sowa
>> <hannes@stressinduktion.org> wrote:
>>> On 06.01.2016 19:00, Jesse Gross wrote:
>>>> Unfortunately, I don't think that we can assume that RTNL is held
>>>> here. It actually is for the drivers that implement Geneve at this
>>>> point but not in all cases for VXLAN. For example, ixgbe refreshes the
>>>> offloads in a service task in addition to when it is opened. There's
>>>> only a couple instances of things like this, so I guess it's probably
>>>> not too hard to through and make sure that we hold RTNL in those
>>>> cases.
>>>
>>>
>>>
>>> Hmm, I am tempted to switch over to the netevent atomic notifier chain
>>> and
>>> install those events there. It does not need rtnl lock at all, so we can
>>> preserve the current semantics. What do you think?
>>
>>
>> I think that holding RTNL while we do these updates is actually the
>> right thing to do. The current situation of having calls from
>> different protocols protected by different locks is not really a great
>> model given that at the driver level these are usually shared data
>> structures. RTNL is already held in the majority of cases already, so
>> I think it is better to just convert the rest.
>
>
> The refreshes from each module are completely synchronous and don't get
> interleaved, so as long as the driver is correctly handling the locking
> internally rtnl lock shouldn't be needed. But as I don't know too much about
> driver developing I can revisit this.
>
> As a advantage I see that the driver developers don't need to worry about
> the rtnl lock at all when adding new events. Is this realistic?
I don't think that there is much savings to be had by avoiding RTNL
since the majority of interactions that the driver has with the stack
involve holding it anyways.
In order to do this safely without RTNL we need to have a lock in each
driver. I don't think that this is safely handled in all cases today
and is likely to get worse in the future. I also noticed that Geneve
actually doesn't hold any special lock while calling into drivers from
geneve_get_rx_port() so it is de-facto relying on RTNL. All other
operations in the Geneve driver are protected by RTNL currently, so we
would need to introduce a new lock to handle this as well. In effect,
it seems like people are implicitly assuming that these operations are
covered by RTNL since most similar things are.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH net-next 2/2] geneve: break dependency to network drivers
2016-01-06 21:01 ` Jesse Gross
@ 2016-01-06 21:18 ` Hannes Frederic Sowa
0 siblings, 0 replies; 9+ messages in thread
From: Hannes Frederic Sowa @ 2016-01-06 21:18 UTC (permalink / raw)
To: Jesse Gross; +Cc: Linux Kernel Network Developers
On 06.01.2016 22:01, Jesse Gross wrote:
> On Wed, Jan 6, 2016 at 12:25 PM, Hannes Frederic Sowa
> <hannes@stressinduktion.org> wrote:
>> The refreshes from each module are completely synchronous and don't get
>> interleaved, so as long as the driver is correctly handling the locking
>> internally rtnl lock shouldn't be needed. But as I don't know too much about
>> driver developing I can revisit this.
>>
>> As a advantage I see that the driver developers don't need to worry about
>> the rtnl lock at all when adding new events. Is this realistic?
>
> I don't think that there is much savings to be had by avoiding RTNL
> since the majority of interactions that the driver has with the stack
> involve holding it anyways.
>
> In order to do this safely without RTNL we need to have a lock in each
> driver. I don't think that this is safely handled in all cases today
> and is likely to get worse in the future. I also noticed that Geneve
> actually doesn't hold any special lock while calling into drivers from
> geneve_get_rx_port() so it is de-facto relying on RTNL. All other
> operations in the Geneve driver are protected by RTNL currently, so we
> would need to introduce a new lock to handle this as well. In effect,
> it seems like people are implicitly assuming that these operations are
> covered by RTNL since most similar things are.
Okay, on top of the v1 version I will check all drivers and add
necessary rtnl_locks. Hopefully it works out and I don't have to defer
calls into working queues in the drivers first.
Thanks,
Hannes
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2016-01-06 21:18 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-01-06 15:41 [PATCH net-next 0/2] net: break dependency of drivers on geneve and vxlan Hannes Frederic Sowa
2016-01-06 15:41 ` [PATCH net-next 1/2] vxlan: break dependency to network drivers Hannes Frederic Sowa
2016-01-06 15:41 ` [PATCH net-next 2/2] geneve: " Hannes Frederic Sowa
2016-01-06 18:00 ` Jesse Gross
2016-01-06 18:48 ` Hannes Frederic Sowa
2016-01-06 19:52 ` Jesse Gross
2016-01-06 20:25 ` Hannes Frederic Sowa
2016-01-06 21:01 ` Jesse Gross
2016-01-06 21:18 ` Hannes Frederic Sowa
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).