Re: [PATCH v8 00/23] Introduce the lwIP network stack

[Tom Rini <trini@konsulko.com>]

[-- Attachment #1: Type: text/plain, Size: 6173 bytes --]

On Mon, Aug 19, 2024 at 04:53:51PM +0200, Jerome Forissier wrote:
> 
> 
> On 8/16/24 20:40, Tom Rini wrote:
> > On Fri, Aug 16, 2024 at 06:21:24PM +0200, Jerome Forissier wrote:
> >>
> >>
> >> On 8/7/24 22:44, Tom Rini wrote:
> >>> On Wed, Aug 07, 2024 at 07:11:44PM +0200, Jerome Forissier wrote:
> >>>
> >>>> This is a rework of a patch series by Maxim Uvarov: "net/lwip: add lwip
> >>>> library for the network stack" [1]. The goal is to introduce the lwIP TCP/IP
> >>>> stack [2] [3] as an alternative to the current implementation in net/,
> >>>> selectable with Kconfig, and ultimately keep only lwIP if possible. Some
> >>>> reasons for doing so are:
> >>>> - Make the support of HTTPS in the wget command easier. Javier T. and
> >>>> Raymond M. (CC'd) have some additional lwIP and Mbed TLS patches to do
> >>>> so. With that it becomes possible to fetch and launch a distro installer
> >>>> such as Debian etc. using a secure, authenticated connection directly
> >>>> from the U-Boot shell. Several use cases:
> >>>>   * Authentication: prevent MITM attack (third party replacing the
> >>>> binary with a different one)
> >>>>   * Confidentiality: prevent third parties from grabbing a copy of the
> >>>> image as it is being downloaded
> >>>>   * Allow connection to servers that do not support plain HTTP anymore
> >>>> (this is becoming more and more common on the Internet these days)
> >>>> - Possibly benefit from additional features implemented in lwIP
> >>>> - Less code to maintain in U-Boot
> >>>>
> >>>> Prior to applying this series, the lwIP stack needs to be added as a
> >>>> Git subtree with the following command:
> >>>>
> >>>>  $  git subtree add --squash --prefix lib/lwip/lwip https://git.savannah.gnu.org/git/lwip.git STABLE-2_2_0_RELEASE
> >>>
> >>> For v9, I think it would be good to do a CI run with NET_LWIP default
> >>> and seeing what fails from that too. There's a few problems still
> >>> leading to a lot of failures, in that case. Thanks.
> >>>
> >>
> >> See here: https://github.com/u-boot/u-boot/pull/635
> >>
> >> I fixed a number of issues, see the commit descriptions. As for the
> >> remaining ones:
> >> - There is no http server in the CI so the wget test I added fails
> > 
> > Ah yes. So the test needs some enable-me type flag, like other tests
> > that require external configuration.
> 
> Can you please suggest how to do that?

Well test/py/tests/test_net_boot.py for example.

> >> - tftp is super slow in QEMU (~145 KiB/s) which causes timeouts. This is
> >> for two reasons: (1) the tftp windowsize option is not supported in lwIP
> >> (while the legacy NET does support it) so the sender waits for an ACK
> >> before sending a new packet; and (2) the latency is very high due to
> >> memcpy() being incredibly slow in QEMU (I am mainly referring to the
> >> memcpy() call in tftp_write() in net/lwip/tftp.c). I measured ~20-60 ms
> >> to copy a few hundred bytes (!) and if CONFIG_USE_ARCH_MEMCPY is enabled
> >> it is slightly better but not much (~15 ms). Also it seems the QEMU
> >> networking emulation is fragmenting the UDP packets because with
> >> CONFIG_TFTP_BLOCKSIZE=1468 the tftp_write() function never receives
> >> 1468 bytes but only a few hundreds at a time (max 544 bytes).
> > 
> > I thought you fixed that? Or was that only for on real hardware?
> 
> It works OK on real hardware...

It should be fast enough here too.

> > But also, some of those numbers sound unusually terrible to me. We can
> > get some bad luck with the free instances, but, still.  Are you sure
> > there's nothing else going on?
> 
> ...and yes that's absolutely terrible. In fact I found something curious.
> With the following patch applied to provide a memcpy() speed test:
> 
> diff --git a/cmd/test.c b/cmd/test.c
> index b4c3eabf9f6..35b8d62af61 100644
> --- a/cmd/test.c
> +++ b/cmd/test.c
> @@ -7,6 +7,8 @@
>  #include <command.h>
>  #include <fs.h>
>  #include <log.h>
> +#include <stdlib.h>
> +#include <time.h>
>  #include <vsprintf.h>
>  
>  #define OP_INVALID	0
> @@ -215,3 +217,49 @@ U_BOOT_CMD(
>  	"do nothing, successfully",
>  	NULL
>  );
> +
> +static int do_mtest(struct cmd_tbl *cmdtp, int flag, int argc,
> +		    char *const argv[])
> +{
> +	ulong t0, delta;
> +	void *src, *dst;
> +	long sz;
> +
> +	if (argc < 2) {
> +		printf("Usage: %s <size_in_bytes> [dst [src]]\n", argv[0]);
> +		return 1;
> +	}
> +	sz = simple_strtol(argv[1], NULL, 10);
> +	if (sz < 0) {
> +		printf("%s: %s: invalid argument\n", argv[0], argv[1]);
> +		return 1;
> +	}
> +	if (argc > 2) {
> +		dst = (void *)hextoul(argv[2], NULL);
> +		if (argc > 3) {
> +			src = (void *)hextoul(argv[3], NULL);
> +		} else {
> +			src = malloc(sz);
> +		}
> +	} else {
> +		dst = malloc(sz);
> +		src = malloc(sz);
> +	}
> +	if (!src || !dst) {
> +		printf("%s: out of memory or NULL address\n", argv[0]);
> +		return 1;
> +	}
> +	printf("%ld bytes from 0x%p to 0x%p: ", sz, src, dst);
> +	t0 = get_timer(0);
> +	memcpy(dst, src, sz);
> +	delta = get_timer(t0);
> +	printf("%ld ms\n", delta);
> +
> +	return 0;
> +}
> +
> +U_BOOT_CMD(
> +	mtest,	CONFIG_SYS_MAXARGS,	0,	do_mtest,
> +	"memcpy() speed test",
> +	NULL
> +);
> 
> I can see that there are ranges of memory that are very slow to *write*
> to (and the loadaddr used by the tftp test happens to fall in that
> range):
> 
> $ make qemu_arm64_defconfig
> $ make -j$(nproc) CROSS_COMPILE="ccache aarch64-linux-gnu-"
> $ qemu-system-aarch64 -M virt -nographic -cpu cortex-a57 -bios u-boot.bin
> 
> U-Boot 2024.10-rc1-00195-g35ce7016c9cb-dirty (Aug 19 2024 - 16:42:15 +0200)
> [...]
> => mtest
> Usage: mtest <size_in_bytes> [dst [src]]
> => mtest 1000
> 1000 bytes from 0x00000000466baed0 to 0x00000000466baae0: 0 ms
> => mtest 1000 0x2000000 0x00000000466baed0
> 1000 bytes from 0x00000000466baed0 to 0x0000000002000000: 22 ms
> => mtest 1000 0x00000000466baed0 0x2000000
> 1000 bytes from 0x0000000002000000 to 0x00000000466baed0: 0 ms
> => 

This is all very strange. Can you investigate a bit please?

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]