[PATCH net-next] net: link_watch: mark bonding link events urgent

[PATCH net-next] net: link_watch: mark bonding link events urgent

[Roopa Prabhu]

From: Roopa Prabhu <roopa@cumulusnetworks.com>

It takes 1sec for bond link down notification to hit user-space
when all slaves of the bond go down. 1sec is too long for
protocol daemons in user-space relying on bond link notification
to failover/recover (eg: multichassis lag implementations in user-space).
Since the link event code already marks team device port link events
 urgent, this patch does the same for bonding link events.

Signed-off-by: Roopa Prabhu <roopa@cumulusnetworks.com>
---
 net/core/link_watch.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/core/link_watch.c b/net/core/link_watch.c
index 9828616..63bb2ad 100644
--- a/net/core/link_watch.c
+++ b/net/core/link_watch.c
@@ -92,7 +92,7 @@ static bool linkwatch_urgent_event(struct net_device *dev)
 	if (dev->ifindex != dev_get_iflink(dev))
 		return true;
 
-	if (dev->priv_flags & IFF_TEAM_PORT)
+	if (dev->priv_flags & (IFF_TEAM_PORT | IFF_BONDING))
 		return true;
 
 	return netif_carrier_ok(dev) &&	qdisc_tx_changing(dev);
-- 
2.1.4

Re: [PATCH net-next] net: link_watch: mark bonding link events urgent

[Jiri Pirko]

Mon, Jan 22, 2018 at 07:07:53AM CET, roopa@cumulusnetworks.com wrote:
>From: Roopa Prabhu <roopa@cumulusnetworks.com>
>
>It takes 1sec for bond link down notification to hit user-space
>when all slaves of the bond go down. 1sec is too long for
>protocol daemons in user-space relying on bond link notification
>to failover/recover (eg: multichassis lag implementations in user-space).
>Since the link event code already marks team device port link events
> urgent, this patch does the same for bonding link events.
>
>Signed-off-by: Roopa Prabhu <roopa@cumulusnetworks.com>
>---
> net/core/link_watch.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/net/core/link_watch.c b/net/core/link_watch.c
>index 9828616..63bb2ad 100644
>--- a/net/core/link_watch.c
>+++ b/net/core/link_watch.c
>@@ -92,7 +92,7 @@ static bool linkwatch_urgent_event(struct net_device *dev)
> 	if (dev->ifindex != dev_get_iflink(dev))
> 		return true;
> 
>-	if (dev->priv_flags & IFF_TEAM_PORT)
>+	if (dev->priv_flags & (IFF_TEAM_PORT | IFF_BONDING))

Don't you want to do that for bonding slaves? IFF_BONDING is set for
both master and slave. netif_is_bond_slave() helper checks that. In fact
netif_is_team_port() should be used here instead of checking IFF_TEAM_PORT
directly. And then you can use netif_is_lag_port() to check them both.



> 		return true;
> 
> 	return netif_carrier_ok(dev) &&	qdisc_tx_changing(dev);
>-- 
>2.1.4
>

Re: [PATCH net-next] net: link_watch: mark bonding link events urgent

[Roopa Prabhu]

On Mon, Jan 22, 2018 at 12:13 AM, Jiri Pirko <jiri@resnulli.us> wrote:
> Mon, Jan 22, 2018 at 07:07:53AM CET, roopa@cumulusnetworks.com wrote:
>>From: Roopa Prabhu <roopa@cumulusnetworks.com>
>>
>>It takes 1sec for bond link down notification to hit user-space
>>when all slaves of the bond go down. 1sec is too long for
>>protocol daemons in user-space relying on bond link notification
>>to failover/recover (eg: multichassis lag implementations in user-space).
>>Since the link event code already marks team device port link events
>> urgent, this patch does the same for bonding link events.
>>
>>Signed-off-by: Roopa Prabhu <roopa@cumulusnetworks.com>
>>---
>> net/core/link_watch.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>>diff --git a/net/core/link_watch.c b/net/core/link_watch.c
>>index 9828616..63bb2ad 100644
>>--- a/net/core/link_watch.c
>>+++ b/net/core/link_watch.c
>>@@ -92,7 +92,7 @@ static bool linkwatch_urgent_event(struct net_device *dev)
>>       if (dev->ifindex != dev_get_iflink(dev))
>>               return true;
>>
>>-      if (dev->priv_flags & IFF_TEAM_PORT)
>>+      if (dev->priv_flags & (IFF_TEAM_PORT | IFF_BONDING))
>
> Don't you want to do that for bonding slaves? IFF_BONDING is set for
> both master and slave. netif_is_bond_slave() helper checks that. In fact
> netif_is_team_port() should be used here instead of checking IFF_TEAM_PORT
> directly. And then you can use netif_is_lag_port() to check them both.


IFF_BONDING Is set for both slaves and master (and thats what I need).
In my case I consistently see link_watch throttle bond link down event
...since it follows immediately after the the bond slave link down
events
(any optimizations in the bonding driver for link events are squashed
at this point by link watch).
so I do need to cover the bond master case. I think for the use-case I
am talking about, team master device might also have the same
problem....if somebody else in
user-space is relying on the team master link down (though i do
understand the sequence of events are a bit different there...where
you set the master link down from user-space).

thanks for the pointers to the helpers..., given above, are you ok if
I move the check to netif_is_lag_port || netif_is_lag_master ?

Re: [PATCH net-next] net: link_watch: mark bonding link events urgent

[Jiri Pirko]

Mon, Jan 22, 2018 at 04:14:17PM CET, roopa@cumulusnetworks.com wrote:
>On Mon, Jan 22, 2018 at 12:13 AM, Jiri Pirko <jiri@resnulli.us> wrote:
>> Mon, Jan 22, 2018 at 07:07:53AM CET, roopa@cumulusnetworks.com wrote:
>>>From: Roopa Prabhu <roopa@cumulusnetworks.com>
>>>
>>>It takes 1sec for bond link down notification to hit user-space
>>>when all slaves of the bond go down. 1sec is too long for
>>>protocol daemons in user-space relying on bond link notification
>>>to failover/recover (eg: multichassis lag implementations in user-space).
>>>Since the link event code already marks team device port link events
>>> urgent, this patch does the same for bonding link events.
>>>
>>>Signed-off-by: Roopa Prabhu <roopa@cumulusnetworks.com>
>>>---
>>> net/core/link_watch.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>>diff --git a/net/core/link_watch.c b/net/core/link_watch.c
>>>index 9828616..63bb2ad 100644
>>>--- a/net/core/link_watch.c
>>>+++ b/net/core/link_watch.c
>>>@@ -92,7 +92,7 @@ static bool linkwatch_urgent_event(struct net_device *dev)
>>>       if (dev->ifindex != dev_get_iflink(dev))
>>>               return true;
>>>
>>>-      if (dev->priv_flags & IFF_TEAM_PORT)
>>>+      if (dev->priv_flags & (IFF_TEAM_PORT | IFF_BONDING))
>>
>> Don't you want to do that for bonding slaves? IFF_BONDING is set for
>> both master and slave. netif_is_bond_slave() helper checks that. In fact
>> netif_is_team_port() should be used here instead of checking IFF_TEAM_PORT
>> directly. And then you can use netif_is_lag_port() to check them both.
>
>
>IFF_BONDING Is set for both slaves and master (and thats what I need).
>In my case I consistently see link_watch throttle bond link down event
>...since it follows immediately after the the bond slave link down
>events
>(any optimizations in the bonding driver for link events are squashed
>at this point by link watch).
>so I do need to cover the bond master case. I think for the use-case I
>am talking about, team master device might also have the same
>problem....if somebody else in
>user-space is relying on the team master link down (though i do
>understand the sequence of events are a bit different there...where
>you set the master link down from user-space).
>
>thanks for the pointers to the helpers..., given above, are you ok if
>I move the check to netif_is_lag_port || netif_is_lag_master ?

Looks fine. Thanks.