[PATCH v2 0/3] net: ifup: greatly reduce ifup -a time for multiple network interface

[PATCH v2 0/3] net: ifup: greatly reduce ifup -a time for multiple network interface

[Ahmad Fatoum]

So far, ifup -a tries to up all non-disabled interfaces in sequence.
This can take a quite a while, because interfaces with the link down,
will be polled for 10s, before giving up on doing DHCP on them.

This series accelerates this in the common case by doing link up
check in parallel, getting down from (number_of_ports_wihout_link * 10s)
to 10s and then adds an optimization for the common case of automounts
with $global.net.server not set.

Times after eth_open_all has brought up CPU Ethernet and 4 DSA ports:

  barebox$ time ifup -a -s # old behavior
  time: 31081ms

  barebox$ time ifup -a
  time: 10002ms

  barebox$ time ifup -a1
  time: 1072ms

Ahmad Fatoum (3):
  net: ifup: have ifup -a poll for link up in parallel
  net: ifup: have ifup -a1 stop at first DHCP-set global.net.server
  defaultenv-2: automount: use ifup -a1 for NFS/TFTP automounts

 Documentation/user/automount.rst            |   2 +-
 defaultenv/defaultenv-2-base/boot/net       |   2 +-
 defaultenv/defaultenv-2-base/init/automount |   4 +-
 include/net.h                               |   5 +
 net/eth.c                                   |  28 +++--
 net/ifup.c                                  | 124 ++++++++++++++++----
 net/net.c                                   |   7 +-
 7 files changed, 138 insertions(+), 34 deletions(-)

-- 
2.30.2

[PATCH v2 1/3] net: ifup: have ifup -a poll for link up in parallel

[Ahmad Fatoum]

DHCP is usually fairly quick, but link up check timeout is 10 seconds,
which adds up, especially on systems with bigger DSA switches.

The workaround is to set ethX.mode=disabled for other ports, but let's
improve the default a bit and have barebox poll link ups in parallel, so
instead of (number_of_ports_wihout_link * 10s), we just wait 10s at
most.

For setups where this is a problem, users may revert to ifup in sequence
by doing ifup -a -s.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
v1 -> v2:
  - new patch
---
 include/net.h |   3 ++
 net/eth.c     |  28 ++++++++++----
 net/ifup.c    | 103 ++++++++++++++++++++++++++++++++++++++++----------
 3 files changed, 105 insertions(+), 29 deletions(-)

diff --git a/include/net.h b/include/net.h
index 0555b0bd6bed..a7da282412fa 100644
--- a/include/net.h
+++ b/include/net.h
@@ -109,6 +109,7 @@ static inline int eth_send_raw(struct eth_device *edev, void *packet,
 int eth_register(struct eth_device* dev);    /* Register network device		*/
 void eth_unregister(struct eth_device* dev); /* Unregister network device	*/
 int eth_set_ethaddr(struct eth_device *edev, const char *ethaddr);
+int eth_carrier_poll_once(struct eth_device *edev);
 int eth_open(struct eth_device *edev);
 void eth_close(struct eth_device *edev);
 int eth_send(struct eth_device *edev, void *packet, int length);	   /* Send a packet		*/
@@ -511,6 +512,8 @@ int net_icmp_send(struct net_connection *con, int len);
 void led_trigger_network(enum led_trigger trigger);
 
 #define IFUP_FLAG_FORCE		(1 << 0)
+#define IFUP_FLAG_PARALLEL	(1 << 1)
+#define IFUP_FLAG_SKIP_CONF	(1 << 2)
 
 int ifup_edev(struct eth_device *edev, unsigned flags);
 int ifup(const char *name, unsigned flags);
diff --git a/net/eth.c b/net/eth.c
index 6fb64afea024..ccac5e2a6488 100644
--- a/net/eth.c
+++ b/net/eth.c
@@ -176,13 +176,29 @@ int eth_complete(struct string_list *sl, char *instr)
 }
 #endif
 
+int eth_carrier_poll_once(struct eth_device *edev)
+{
+	int ret;
+
+	if (!IS_ENABLED(CONFIG_PHYLIB))
+		return 0;
+
+	if (!edev->phydev)
+		return 0;
+
+	ret = phy_update_status(edev->phydev);
+	if (ret)
+		return ret;
+
+	edev->last_link_check = get_time_ns();
+	return edev->phydev->link ? 0 : -ENETDOWN;
+}
+
 /*
  * Check for link if we haven't done so for longer.
  */
 static int eth_carrier_check(struct eth_device *edev, bool may_wait)
 {
-	int ret;
-
 	if (!IS_ENABLED(CONFIG_PHYLIB))
 		return 0;
 
@@ -190,12 +206,8 @@ static int eth_carrier_check(struct eth_device *edev, bool may_wait)
 		return 0;
 
 	if (!edev->last_link_check ||
-	    is_timeout(edev->last_link_check, 5 * SECOND)) {
-		ret = phy_update_status(edev->phydev);
-		if (ret)
-			return ret;
-		edev->last_link_check = get_time_ns();
-	}
+	    is_timeout(edev->last_link_check, 5 * SECOND))
+		eth_carrier_poll_once(edev);
 
 	if (may_wait && !edev->phydev->link) {
 		phy_wait_aneg_done(edev->phydev);
diff --git a/net/ifup.c b/net/ifup.c
index e4d5374db3ae..c491ea03c1c8 100644
--- a/net/ifup.c
+++ b/net/ifup.c
@@ -179,6 +179,28 @@ static void set_linux_bootarg(struct eth_device *edev)
 	}
 }
 
+static int ifup_edev_conf(struct eth_device *edev, unsigned flags)
+{
+	int ret;
+
+	if (edev->global_mode == ETH_MODE_DHCP) {
+		if (IS_ENABLED(CONFIG_NET_DHCP)) {
+			ret = dhcp(edev, NULL);
+		} else {
+			dev_err(&edev->dev, "DHCP support not available\n");
+			ret = -ENOSYS;
+		}
+		if (ret)
+			return ret;
+	}
+
+	set_linux_bootarg(edev);
+
+	edev->ifup = true;
+
+	return 0;
+}
+
 int ifup_edev(struct eth_device *edev, unsigned flags)
 {
 	int ret;
@@ -205,22 +227,10 @@ int ifup_edev(struct eth_device *edev, unsigned flags)
 	if (ret)
 		return ret;
 
-	if (edev->global_mode == ETH_MODE_DHCP) {
-		if (IS_ENABLED(CONFIG_NET_DHCP)) {
-			ret = dhcp(edev, NULL);
-		} else {
-			dev_err(&edev->dev, "DHCP support not available\n");
-			ret = -ENOSYS;
-		}
-		if (ret)
-			return ret;
-	}
-
-	set_linux_bootarg(edev);
-
-	edev->ifup = true;
+	if (flags & IFUP_FLAG_SKIP_CONF)
+		return 1;
 
-	return 0;
+	return ifup_edev_conf(edev, flags);
 }
 
 void ifdown_edev(struct eth_device *edev)
@@ -260,9 +270,54 @@ int ifdown(const char *ethname)
 
 static int net_ifup_force_detect;
 
-int ifup_all(unsigned flags)
+static bool ifup_edev_need_conf(struct eth_device *edev)
+{
+	return edev->active && !edev->ifup &&
+		edev->global_mode != ETH_MODE_DISABLED;
+}
+
+static void __ifup_all_parallel(unsigned flags)
 {
 	struct eth_device *edev;
+	unsigned netdev_count = 0;
+	u64 start;
+	int ret;
+
+	for_each_netdev(edev) {
+		ret = ifup_edev(edev, flags | IFUP_FLAG_SKIP_CONF);
+		if (ret == 1)
+			netdev_count++;
+	}
+
+	start = get_time_ns();
+	while (netdev_count && !is_timeout(start, PHY_AN_TIMEOUT * SECOND)) {
+		for_each_netdev(edev) {
+			if (!ifup_edev_need_conf(edev))
+				continue;
+
+			ret = eth_carrier_poll_once(edev);
+			if (ret)
+				continue;
+
+			ifup_edev_conf(edev, flags);
+			if (!edev->ifup)
+				continue;
+
+			netdev_count--;
+		}
+	}
+}
+
+static void __ifup_all_sequence(unsigned flags)
+{
+	struct eth_device *edev;
+
+	for_each_netdev(edev)
+		ifup_edev(edev, flags);
+}
+
+int ifup_all(unsigned flags)
+{
 	DIR *dir;
 	struct dirent *d;
 
@@ -285,8 +340,10 @@ int ifup_all(unsigned flags)
 	    list_empty(&netdev_list))
 		device_detect_all();
 
-	for_each_netdev(edev)
-		ifup_edev(edev, flags);
+	if (flags & IFUP_FLAG_PARALLEL)
+		__ifup_all_parallel(flags);
+	else
+		__ifup_all_sequence(flags);
 
 	return 0;
 }
@@ -315,14 +372,17 @@ BAREBOX_MAGICVAR(global.net.ifup_force_detect,
 static int do_ifup(int argc, char *argv[])
 {
 	int opt;
-	unsigned flags = 0;
+	unsigned flags = IFUP_FLAG_PARALLEL;
 	int all = 0;
 
-	while ((opt = getopt(argc, argv, "af")) > 0) {
+	while ((opt = getopt(argc, argv, "asf")) > 0) {
 		switch (opt) {
 		case 'f':
 			flags |= IFUP_FLAG_FORCE;
 			break;
+		case 's':
+			flags &= ~IFUP_FLAG_PARALLEL;
+			break;
 		case 'a':
 			all = 1;
 			break;
@@ -346,13 +406,14 @@ BAREBOX_CMD_HELP_TEXT("/env/network/<intf> file. See Documentation/user/networki
 BAREBOX_CMD_HELP_TEXT("")
 BAREBOX_CMD_HELP_TEXT("Options:")
 BAREBOX_CMD_HELP_OPT ("-a",  "bring up all interfaces")
+BAREBOX_CMD_HELP_OPT ("-s",  "bring up interfaces in sequence, not in parallel")
 BAREBOX_CMD_HELP_OPT ("-f",  "Force. Configure even if ip already set")
 BAREBOX_CMD_HELP_END
 
 BAREBOX_CMD_START(ifup)
 	.cmd		= do_ifup,
 	BAREBOX_CMD_DESC("bring a network interface up")
-	BAREBOX_CMD_OPTS("[-af] [INTF]")
+	BAREBOX_CMD_OPTS("[-asf] [INTF]")
 	BAREBOX_CMD_GROUP(CMD_GRP_NET)
 	BAREBOX_CMD_COMPLETE(eth_complete)
 	BAREBOX_CMD_HELP(cmd_ifup_help)
-- 
2.30.2

[PATCH v2 2/3] net: ifup: have ifup -a1 stop at first DHCP-set global.net.server

[Ahmad Fatoum]

The normal use case for ifup -a is to get *some* interface working and
not really wait for all interfaces to come up and then timeout waiting
for those without link up to never get a DHCP lease.

When using automounts with a previously empty $global.net.server, we
know this to be the case, because the first DHCP interface will set the
variable and all remaining ones won't affect this. Therefore, let's add
a ifup -a1 option, which would stop after $global.net.server was set via
DHCP. This is a special case of a possible future ifup -ar, which would
check after each ifup if $global.net.server was either newly set or
became resolvable, but that would be a bigger change, so we skip that
for now.

Times after eth_open_all has brought up CPU Ethernet and 4 DSA ports:

  barebox$ time ifup -a
  time: 10002ms

  barebox$ time ifup -a1
  time: 1072ms

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
  - v1 -> v2:
    - add new option for this behavior instead of just -a
    - stop after global.net.server is set instead of just
      stopping after first upped interface
---
 include/net.h |  2 ++
 net/ifup.c    | 27 ++++++++++++++++++++++++---
 net/net.c     |  7 ++++++-
 3 files changed, 32 insertions(+), 4 deletions(-)

diff --git a/include/net.h b/include/net.h
index a7da282412fa..b7cbab7d720a 100644
--- a/include/net.h
+++ b/include/net.h
@@ -257,6 +257,7 @@ extern unsigned char *NetRxPackets[PKTBUFSRX];/* Receive packets		*/
 
 void net_set_ip(struct eth_device *edev, IPaddr_t ip);
 void net_set_serverip(IPaddr_t ip);
+const char *net_get_server(void);
 void net_set_serverip_empty(IPaddr_t ip);
 void net_set_netmask(struct eth_device *edev, IPaddr_t ip);
 void net_set_gateway(IPaddr_t ip);
@@ -514,6 +515,7 @@ void led_trigger_network(enum led_trigger trigger);
 #define IFUP_FLAG_FORCE		(1 << 0)
 #define IFUP_FLAG_PARALLEL	(1 << 1)
 #define IFUP_FLAG_SKIP_CONF	(1 << 2)
+#define IFUP_FLAG_UNTIL_NET_SERVER	(1 << 3)
 
 int ifup_edev(struct eth_device *edev, unsigned flags);
 int ifup(const char *name, unsigned flags);
diff --git a/net/ifup.c b/net/ifup.c
index c491ea03c1c8..64298b44ebf3 100644
--- a/net/ifup.c
+++ b/net/ifup.c
@@ -304,6 +304,9 @@ static void __ifup_all_parallel(unsigned flags)
 				continue;
 
 			netdev_count--;
+
+			if ((flags & IFUP_FLAG_UNTIL_NET_SERVER) && net_get_server())
+				return;
 		}
 	}
 }
@@ -312,8 +315,12 @@ static void __ifup_all_sequence(unsigned flags)
 {
 	struct eth_device *edev;
 
-	for_each_netdev(edev)
+	for_each_netdev(edev) {
 		ifup_edev(edev, flags);
+
+		if ((flags & IFUP_FLAG_UNTIL_NET_SERVER) && net_get_server())
+			return;
+	}
 }
 
 int ifup_all(unsigned flags)
@@ -340,6 +347,16 @@ int ifup_all(unsigned flags)
 	    list_empty(&netdev_list))
 		device_detect_all();
 
+	/*
+	 * In the future, we may add an iproute -r option that tries to
+	 * resolve $global.net.server using each interface. For now,
+	 * we only support the special case of $global.net.server being
+	 * empty, i.e. the first DHCP lease setting $global.net.server
+	 * will be what we're going with.
+	 */
+	if (net_get_server())
+		flags &= ~IFUP_FLAG_UNTIL_NET_SERVER;
+
 	if (flags & IFUP_FLAG_PARALLEL)
 		__ifup_all_parallel(flags);
 	else
@@ -375,11 +392,14 @@ static int do_ifup(int argc, char *argv[])
 	unsigned flags = IFUP_FLAG_PARALLEL;
 	int all = 0;
 
-	while ((opt = getopt(argc, argv, "asf")) > 0) {
+	while ((opt = getopt(argc, argv, "asf1")) > 0) {
 		switch (opt) {
 		case 'f':
 			flags |= IFUP_FLAG_FORCE;
 			break;
+		case '1':
+			flags |= IFUP_FLAG_UNTIL_NET_SERVER;
+			break;
 		case 's':
 			flags &= ~IFUP_FLAG_PARALLEL;
 			break;
@@ -408,12 +428,13 @@ BAREBOX_CMD_HELP_TEXT("Options:")
 BAREBOX_CMD_HELP_OPT ("-a",  "bring up all interfaces")
 BAREBOX_CMD_HELP_OPT ("-s",  "bring up interfaces in sequence, not in parallel")
 BAREBOX_CMD_HELP_OPT ("-f",  "Force. Configure even if ip already set")
+BAREBOX_CMD_HELP_OPT ("-1",  "Early exit if DHCP sets $global.net.server")
 BAREBOX_CMD_HELP_END
 
 BAREBOX_CMD_START(ifup)
 	.cmd		= do_ifup,
 	BAREBOX_CMD_DESC("bring a network interface up")
-	BAREBOX_CMD_OPTS("[-asf] [INTF]")
+	BAREBOX_CMD_OPTS("[-asf1] [INTF]")
 	BAREBOX_CMD_GROUP(CMD_GRP_NET)
 	BAREBOX_CMD_COMPLETE(eth_complete)
 	BAREBOX_CMD_HELP(cmd_ifup_help)
diff --git a/net/net.c b/net/net.c
index 22931509d0d9..19161d2e828e 100644
--- a/net/net.c
+++ b/net/net.c
@@ -325,9 +325,14 @@ void net_set_serverip(IPaddr_t ip)
 	net_server = xasprintf("%pI4", &ip);
 }
 
+const char *net_get_server(void)
+{
+	return net_server && *net_server ? net_server : NULL;
+}
+
 void net_set_serverip_empty(IPaddr_t ip)
 {
-	if (net_server && *net_server)
+	if (net_get_server())
 		return;
 
 	net_set_serverip(ip);
-- 
2.30.2

[PATCH v2 3/3] defaultenv-2: automount: use ifup -a1 for NFS/TFTP automounts

[Ahmad Fatoum]

The ifup -a in the automounts has too big a scope: The remote server is
specified at mount time, so we could stop bringing up interfaces once
$global.net.server becomes resolvable. For the common case, where
$global.net.server is initially empty and then set via DHCP, ifup
provides the new -1 option to early-exit once $global.net.server was set.

For setups where one, but not all, Ethernet interfaces have a cable
attached, this normally reduces ifup -a time from 10s to 1s.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
v1 -> v2:
  - new patch
---
 Documentation/user/automount.rst            | 2 +-
 defaultenv/defaultenv-2-base/boot/net       | 2 +-
 defaultenv/defaultenv-2-base/init/automount | 4 ++--
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/Documentation/user/automount.rst b/Documentation/user/automount.rst
index 1fdeffc66336..b41b41ef021d 100644
--- a/Documentation/user/automount.rst
+++ b/Documentation/user/automount.rst
@@ -15,7 +15,7 @@ TFTP server, the following is required:
 .. code-block:: sh
 
   mkdir -p /mnt/tftp
-  automount /mnt/tftp 'ifup -a && mount -t tftp $global.net.server /mnt/tftp'
+  automount /mnt/tftp 'ifup -a1 && mount -t tftp $global.net.server /mnt/tftp'
 
 This creates an automountpoint on ``/mnt/tftp``. Whenever this directory is accessed,
 the command ``ifup eth0 && mount -t tftp $eth0.serverip /mnt/tftp`` is executed.
diff --git a/defaultenv/defaultenv-2-base/boot/net b/defaultenv/defaultenv-2-base/boot/net
index e79432eb277c..98286ff5e050 100644
--- a/defaultenv/defaultenv-2-base/boot/net
+++ b/defaultenv/defaultenv-2-base/boot/net
@@ -3,7 +3,7 @@
 path="/mnt/tftp"
 
 # global.net.server and global.hostname may be set by DHCP, so trigger it first
-ifup -a
+ifup -a1
 
 global.bootm.image="${path}/${global.user}-linux-${global.hostname}"
 
diff --git a/defaultenv/defaultenv-2-base/init/automount b/defaultenv/defaultenv-2-base/init/automount
index 0996cea04df7..39e823489793 100644
--- a/defaultenv/defaultenv-2-base/init/automount
+++ b/defaultenv/defaultenv-2-base/init/automount
@@ -3,12 +3,12 @@
 # automount tftp server
 
 mkdir -p /mnt/tftp
-automount /mnt/tftp 'ifup -a && mount -t tftp $global.net.server /mnt/tftp'
+automount /mnt/tftp 'ifup -a1 && mount -t tftp $global.net.server /mnt/tftp'
 
 # automount nfs server's nfsroot
 
 mkdir -p /mnt/nfs
-automount /mnt/nfs 'ifup -a && mount -t nfs ${global.net.server}:/home/${global.user}/nfsroot/${global.hostname} /mnt/nfs'
+automount /mnt/nfs 'ifup -a1 && mount -t nfs ${global.net.server}:/home/${global.user}/nfsroot/${global.hostname} /mnt/nfs'
 
 
 # FAT on usb disk example
-- 
2.30.2

Re: [PATCH v2 0/3] net: ifup: greatly reduce ifup -a time for multiple network interface

[Sascha Hauer]

On Mon, Jan 30, 2023 at 08:20:54AM +0100, Ahmad Fatoum wrote:
> So far, ifup -a tries to up all non-disabled interfaces in sequence.
> This can take a quite a while, because interfaces with the link down,
> will be polled for 10s, before giving up on doing DHCP on them.
> 
> This series accelerates this in the common case by doing link up
> check in parallel, getting down from (number_of_ports_wihout_link * 10s)
> to 10s and then adds an optimization for the common case of automounts
> with $global.net.server not set.
> 
> Times after eth_open_all has brought up CPU Ethernet and 4 DSA ports:
> 
>   barebox$ time ifup -a -s # old behavior
>   time: 31081ms
> 
>   barebox$ time ifup -a
>   time: 10002ms
> 
>   barebox$ time ifup -a1
>   time: 1072ms
> 
> Ahmad Fatoum (3):
>   net: ifup: have ifup -a poll for link up in parallel
>   net: ifup: have ifup -a1 stop at first DHCP-set global.net.server
>   defaultenv-2: automount: use ifup -a1 for NFS/TFTP automounts

Applied, thanks

Sascha

> 
>  Documentation/user/automount.rst            |   2 +-
>  defaultenv/defaultenv-2-base/boot/net       |   2 +-
>  defaultenv/defaultenv-2-base/init/automount |   4 +-
>  include/net.h                               |   5 +
>  net/eth.c                                   |  28 +++--
>  net/ifup.c                                  | 124 ++++++++++++++++----
>  net/net.c                                   |   7 +-
>  7 files changed, 138 insertions(+), 34 deletions(-)
> 
> -- 
> 2.30.2
> 
> 
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |