[PATCH] netpoll: Remove 4s sleep during carrier detection

[PATCH] netpoll: Remove 4s sleep during carrier detection

[Breno Leitao]

This patch proposes to remove the msleep(4s) during netpoll_setup() if
the carrier appears instantly.

Modern NICs do not seem to have this bouncing problem anymore, and this
sleep slows down the machine boot unnecessarily

Reported-by: Michael van der Westhuizen <rmikey@meta.com>
Signed-off-by: Breno Leitao <leitao@debian.org>
---
 net/core/netpoll.c | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/net/core/netpoll.c b/net/core/netpoll.c
index 9be762e1d042..6f45aeaf3da0 100644
--- a/net/core/netpoll.c
+++ b/net/core/netpoll.c
@@ -704,15 +704,6 @@ int netpoll_setup(struct netpoll *np)
 			msleep(1);
 		}
 
-		/* If carrier appears to come up instantly, we don't
-		 * trust it and pause so that we don't pump all our
-		 * queued console messages into the bitbucket.
-		 */
-
-		if (time_before(jiffies, atleast)) {
-			np_notice(np, "carrier detect appears untrustworthy, waiting 4 seconds\n");
-			msleep(4000);
-		}
 		rtnl_lock();
 	}
 
-- 
2.30.2

[RFC PATCH v2] netpoll: Remove 4s sleep during carrier detection

[Breno Leitao]

This patch proposes to remove the msleep(4s) during netpoll_setup() if
the carrier appears instantly.

Modern NICs do not seem to have this bouncing problem anymore, and this
sleep slows down the machine boot unnecessarily

Reported-by: Michael van der Westhuizen <rmikey@meta.com>
Signed-off-by: Breno Leitao <leitao@debian.org>
---
 net/core/netpoll.c | 12 +-----------
 1 file changed, 1 insertion(+), 11 deletions(-)

diff --git a/net/core/netpoll.c b/net/core/netpoll.c
index 9be762e1d042..a089b704b986 100644
--- a/net/core/netpoll.c
+++ b/net/core/netpoll.c
@@ -682,7 +682,7 @@ int netpoll_setup(struct netpoll *np)
 	}
 
 	if (!netif_running(ndev)) {
-		unsigned long atmost, atleast;
+		unsigned long atmost;
 
 		np_info(np, "device %s not up yet, forcing it\n", np->dev_name);
 
@@ -694,7 +694,6 @@ int netpoll_setup(struct netpoll *np)
 		}
 
 		rtnl_unlock();
-		atleast = jiffies + HZ/10;
 		atmost = jiffies + carrier_timeout * HZ;
 		while (!netif_carrier_ok(ndev)) {
 			if (time_after(jiffies, atmost)) {
@@ -704,15 +703,6 @@ int netpoll_setup(struct netpoll *np)
 			msleep(1);
 		}
 
-		/* If carrier appears to come up instantly, we don't
-		 * trust it and pause so that we don't pump all our
-		 * queued console messages into the bitbucket.
-		 */
-
-		if (time_before(jiffies, atleast)) {
-			np_notice(np, "carrier detect appears untrustworthy, waiting 4 seconds\n");
-			msleep(4000);
-		}
 		rtnl_lock();
 	}
 
-- 
2.30.2

Re: [RFC PATCH v2] netpoll: Remove 4s sleep during carrier detection

[Jakub Kicinski]

On Thu, 19 Jan 2023 10:00:08 -0800 Breno Leitao wrote:
> This patch proposes to remove the msleep(4s) during netpoll_setup() if
> the carrier appears instantly.
> 
> Modern NICs do not seem to have this bouncing problem anymore, and this
> sleep slows down the machine boot unnecessarily

We should mention in the message that the wait is counter-productive on
servers which have BMC communicating over NC-SI via the same NIC as gets
used for netconsole. BMC will keep the PHY up, hence the carrier
appearing instantly.

We could add a smaller delay, but really having instant carrier and
then loosing it seems like a driver bug, so let's try to rip the band
aid off and ask for forgiveness instead.


Few extra process rules:
 - don't repost another version within 24h,
 - keep a changelog under --- 
 - add tree name to the tag - [PATCH net-next]

Also, I'd just go for PATCH, no need to RFC this.
If someone wants to object they can object to a PATCH.

Re: [RFC PATCH v2] netpoll: Remove 4s sleep during carrier detection

[Andrew Lunn]

On Thu, Jan 19, 2023 at 11:04:21AM -0800, Jakub Kicinski wrote:
> On Thu, 19 Jan 2023 10:00:08 -0800 Breno Leitao wrote:
> > This patch proposes to remove the msleep(4s) during netpoll_setup() if
> > the carrier appears instantly.
> > 
> > Modern NICs do not seem to have this bouncing problem anymore, and this
> > sleep slows down the machine boot unnecessarily

I'm not sure 'bouncing' is the correct word here. That would imply up,
down, up, down and then stable up. What i guess the real issue here
was the MAC driver said the link was up while autoneg was still
happening, which takes around 1.5 seconds.

> We should mention in the message that the wait is counter-productive on
> servers which have BMC communicating over NC-SI via the same NIC as gets
> used for netconsole. BMC will keep the PHY up, hence the carrier
> appearing instantly.
> 
> We could add a smaller delay, but really having instant carrier and
> then loosing it seems like a driver bug, so let's try to rip the band
> aid off and ask for forgiveness instead.

It would be good to put some of this into the commit message. Explain
the case you see it go wrong.

The other scenarios i can think of are:

The bootloader configured the interface up, and used the interface,
e.g. to tftp boot. The PHY was left up when transitioning into
Linux. Hence there is no need to wait around 1.5 seconds for autoneg
to complete.

The link is fibre, SERDES getting sync could happen within 0.1Hz, and
so it appears to be instantaneously.

This work around does seem very old, pre-git times, so i also doubt
there are many systems which are truly broken like this.

      Andrew