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

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

[Breno Leitao]

This patch removes the msleep(4s) during netpoll_setup() if the carrier
appears instantly.

Here are some scenarios where this workaround is counter-productive in
modern ages:

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.

The link is fibre, SERDES getting sync could happen within 0.1Hz, and
the carrier also appears instantly.

Other than that, if a driver is reporting instant carrier and then
losing it, this is probably a driver bug.

Reported-by: Michael van der Westhuizen <rmikey@meta.com>
Signed-off-by: Breno Leitao <leitao@debian.org>
--
v1->v2: added "RFC" in the subject
v2->v3: improved the commit message
---
 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 9be762e1d..a089b704b 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: [PATCH v3] netpoll: Remove 4s sleep during carrier detection

[David Laight]

From: Breno Leitao
> Sent: 25 January 2023 18:53
> This patch removes the msleep(4s) during netpoll_setup() if the carrier
> appears instantly.
> 
> Here are some scenarios where this workaround is counter-productive in
> modern ages:
> 
> 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.
> 
> The link is fibre, SERDES getting sync could happen within 0.1Hz, and
> the carrier also appears instantly.
> 
> Other than that, if a driver is reporting instant carrier and then
> losing it, this is probably a driver bug.

I can't help feeling that this will break something.
The 4 second delay does look counter productive though.
Obvious alternatives are 'wait a bit before the first check'
and 'require carrier to be present for a few checks'.

It also has to be said that checking every ms seems over enthusiastic.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

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

[Breno Leitao]

On 26/01/2023 09:04, David Laight wrote:

>> This patch removes the msleep(4s) during netpoll_setup() if the carrier
>> appears instantly.
>>
>> Here are some scenarios where this workaround is counter-productive in
>> modern ages:
>>
>> 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.
>>
>> The link is fibre, SERDES getting sync could happen within 0.1Hz, and
>> the carrier also appears instantly.
>>
>> Other than that, if a driver is reporting instant carrier and then
>> losing it, this is probably a driver bug.
> 
> I can't help feeling that this will break something.

If we see breakages after this patch, then we can identify broken 
drivers, and fix the driver itself.

On the other side, if we keep this workaround, we are penalizing the 
boot of every modern machine in 4s, just because we might have some 
broken driver somewhere.

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

[Andrew Lunn]

On Thu, Jan 26, 2023 at 09:04:42AM +0000, David Laight wrote:
> From: Breno Leitao
> > Sent: 25 January 2023 18:53
> > This patch removes the msleep(4s) during netpoll_setup() if the carrier
> > appears instantly.
> > 
> > Here are some scenarios where this workaround is counter-productive in
> > modern ages:
> > 
> > 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.
> > 
> > The link is fibre, SERDES getting sync could happen within 0.1Hz, and
> > the carrier also appears instantly.
> > 
> > Other than that, if a driver is reporting instant carrier and then
> > losing it, this is probably a driver bug.
> 
> I can't help feeling that this will break something.
> The 4 second delay does look counter productive though.
> Obvious alternatives are 'wait a bit before the first check'
> and 'require carrier to be present for a few checks'.

I'm guessing, but i think the issue is that the MAC reports the
carrier is up, even though autoneg has not completed, and so packets
are getting dropped. Autoneg takes around 1.5 seconds, so you need to
wait this long before starting to send to prevent packets landing in
the bit bucket. And i guess polling as you suggests does not help,
since it never returns the true status.

But this is pure guesswork. Maybe some mailing list archaeology can
help explain this code.

I guess the likely breaking scenario is that simply the first 1.5
seconds of the kernel log goes to the bit bucket for broken
MACs. Which is not fatal, just annoying for somebody trying to debug a
crash in the first few seconds. I suppose dhcp might also take longer
for broken MACs, since its first requests also get lost, and it might
get into exponential back off.

I guess the risks are small here. But i use the word guess a lot...

  Andrew

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

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net-next.git (master)
by Jakub Kicinski <kuba@kernel.org>:

On Wed, 25 Jan 2023 10:52:30 -0800 you wrote:
> This patch removes the msleep(4s) during netpoll_setup() if the carrier
> appears instantly.
> 
> Here are some scenarios where this workaround is counter-productive in
> modern ages:
> 
> 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.
> 
> [...]

Here is the summary with links:
  - [v3] netpoll: Remove 4s sleep during carrier detection
    https://git.kernel.org/netdev/net-next/c/d8afe2f8a92d

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html