| Message ID | 52b126477a39fe8e0bdea9fd9ec70b0951aab88b.1716393035.git.jerome.forissier@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Introduce the lwIP network stack | expand |
Hi Jerome, On Wed, 22 May 2024 at 19:04, Jerome Forissier <jerome.forissier@linaro.org> wrote: > > Some sandbox tests make strong assumptions on how the network stack is > implemented. For example, the ping tests assume that ARP resolution > occurs upon sending out the ICMP packet. This is not always the case > with the lwIP stack which can cache ARP information. > Therefore, disable these tests when CONFIG_NET_LWIP is enabled. Is the ARP Caching the only issue? U-Boot isn't supposed to use the network stack that much, so it might be a better idea to disable arp-caching in LWIP (assuming it's doable). We might even get a few size of bytes back Cheers /Ilias > > Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org> > --- > test/dm/dsa.c | 2 ++ > test/dm/eth.c | 4 ++++ > 2 files changed, 6 insertions(+) > > diff --git a/test/dm/dsa.c b/test/dm/dsa.c > index c857106eaf..147e2a4afe 100644 > --- a/test/dm/dsa.c > +++ b/test/dm/dsa.c > @@ -59,6 +59,7 @@ static int dm_test_dsa_probe(struct unit_test_state *uts) > > DM_TEST(dm_test_dsa_probe, UT_TESTF_SCAN_FDT); > > +#if !defined(CONFIG_NET_LWIP) > /* This test sends ping requests with the local address through each DSA port > * via the sandbox DSA master Eth. > */ > @@ -80,3 +81,4 @@ static int dm_test_dsa(struct unit_test_state *uts) > } > > DM_TEST(dm_test_dsa, UT_TESTF_SCAN_FDT); > +#endif /* !defined(CONFIG_NET_LWIP) */ > diff --git a/test/dm/eth.c b/test/dm/eth.c > index bb3dcc6b95..cf97b1c1ab 100644 > --- a/test/dm/eth.c > +++ b/test/dm/eth.c > @@ -170,6 +170,7 @@ static int dm_test_ip6_make_lladdr(struct unit_test_state *uts) > DM_TEST(dm_test_ip6_make_lladdr, UT_TESTF_SCAN_FDT); > #endif > > +#if !defined(CONFIG_NET_LWIP) > static int dm_test_eth(struct unit_test_state *uts) > { > net_ping_ip = string_to_ip("1.1.2.2"); > @@ -298,6 +299,7 @@ static int dm_test_eth_act(struct unit_test_state *uts) > return 0; > } > DM_TEST(dm_test_eth_act, UT_TESTF_SCAN_FDT); > +#endif /* !CONFIG_NET_LWIP */ > > /* Ensure that all addresses are loaded properly */ > static int dm_test_ethaddr(struct unit_test_state *uts) > @@ -332,6 +334,7 @@ static int dm_test_ethaddr(struct unit_test_state *uts) > } > DM_TEST(dm_test_ethaddr, UT_TESTF_SCAN_FDT); > > +#if !defined(CONFIG_NET_LWIP) > /* The asserts include a return on fail; cleanup in the caller */ > static int _dm_test_eth_rotate1(struct unit_test_state *uts) > { > @@ -616,6 +619,7 @@ static int dm_test_eth_async_ping_reply(struct unit_test_state *uts) > } > > DM_TEST(dm_test_eth_async_ping_reply, UT_TESTF_SCAN_FDT); > +#endif /* !CONFIG_NET_LWIP */ > > #if IS_ENABLED(CONFIG_IPV6_ROUTER_DISCOVERY) > > -- > 2.40.1 >
On 5/22/24 20:07, Ilias Apalodimas wrote: > Hi Jerome, > > On Wed, 22 May 2024 at 19:04, Jerome Forissier > <jerome.forissier@linaro.org> wrote: >> >> Some sandbox tests make strong assumptions on how the network stack is >> implemented. For example, the ping tests assume that ARP resolution >> occurs upon sending out the ICMP packet. This is not always the case >> with the lwIP stack which can cache ARP information. >> Therefore, disable these tests when CONFIG_NET_LWIP is enabled. > > Is the ARP Caching the only issue? No, but I don't recall exactly what was wrong in addition to that. > U-Boot isn't supposed to use the network stack that much, so it might > be a better idea to disable arp-caching in LWIP (assuming it's > doable). We might even get a few size of bytes back If ARP_TABLE_SIZE is 0, there is no caching but in addition to that the lwIP stack does not work well. For example when requested to send an ICMP packet to an unknown IP (unknown as in "not yet resolved at the ethernet level" i.e., ARP) and assuming ARP_TABLE_SIZE is 1, it will automatically do the ARP request/response then proceed with ICMP send. However if ARP_TABLE_SIZE is 0, only the ARP resolution occurs and the ICMP packet won't go out. So I am not sure that 0 is a valid value. > > Cheers > /Ilias >> >> Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org> >> --- >> test/dm/dsa.c | 2 ++ >> test/dm/eth.c | 4 ++++ >> 2 files changed, 6 insertions(+) >> >> diff --git a/test/dm/dsa.c b/test/dm/dsa.c >> index c857106eaf..147e2a4afe 100644 >> --- a/test/dm/dsa.c >> +++ b/test/dm/dsa.c >> @@ -59,6 +59,7 @@ static int dm_test_dsa_probe(struct unit_test_state *uts) >> >> DM_TEST(dm_test_dsa_probe, UT_TESTF_SCAN_FDT); >> >> +#if !defined(CONFIG_NET_LWIP) >> /* This test sends ping requests with the local address through each DSA port >> * via the sandbox DSA master Eth. >> */ >> @@ -80,3 +81,4 @@ static int dm_test_dsa(struct unit_test_state *uts) >> } >> >> DM_TEST(dm_test_dsa, UT_TESTF_SCAN_FDT); >> +#endif /* !defined(CONFIG_NET_LWIP) */ >> diff --git a/test/dm/eth.c b/test/dm/eth.c >> index bb3dcc6b95..cf97b1c1ab 100644 >> --- a/test/dm/eth.c >> +++ b/test/dm/eth.c >> @@ -170,6 +170,7 @@ static int dm_test_ip6_make_lladdr(struct unit_test_state *uts) >> DM_TEST(dm_test_ip6_make_lladdr, UT_TESTF_SCAN_FDT); >> #endif >> >> +#if !defined(CONFIG_NET_LWIP) >> static int dm_test_eth(struct unit_test_state *uts) >> { >> net_ping_ip = string_to_ip("1.1.2.2"); >> @@ -298,6 +299,7 @@ static int dm_test_eth_act(struct unit_test_state *uts) >> return 0; >> } >> DM_TEST(dm_test_eth_act, UT_TESTF_SCAN_FDT); >> +#endif /* !CONFIG_NET_LWIP */ >> >> /* Ensure that all addresses are loaded properly */ >> static int dm_test_ethaddr(struct unit_test_state *uts) >> @@ -332,6 +334,7 @@ static int dm_test_ethaddr(struct unit_test_state *uts) >> } >> DM_TEST(dm_test_ethaddr, UT_TESTF_SCAN_FDT); >> >> +#if !defined(CONFIG_NET_LWIP) >> /* The asserts include a return on fail; cleanup in the caller */ >> static int _dm_test_eth_rotate1(struct unit_test_state *uts) >> { >> @@ -616,6 +619,7 @@ static int dm_test_eth_async_ping_reply(struct unit_test_state *uts) >> } >> >> DM_TEST(dm_test_eth_async_ping_reply, UT_TESTF_SCAN_FDT); >> +#endif /* !CONFIG_NET_LWIP */ >> >> #if IS_ENABLED(CONFIG_IPV6_ROUTER_DISCOVERY) >> >> -- >> 2.40.1 >>
On Wed, May 22, 2024 at 06:00:10PM +0200, Jerome Forissier wrote: > Some sandbox tests make strong assumptions on how the network stack is > implemented. For example, the ping tests assume that ARP resolution > occurs upon sending out the ICMP packet. This is not always the case > with the lwIP stack which can cache ARP information. > Therefore, disable these tests when CONFIG_NET_LWIP is enabled. > > Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org> > --- > test/dm/dsa.c | 2 ++ > test/dm/eth.c | 4 ++++ > 2 files changed, 6 insertions(+) > > diff --git a/test/dm/dsa.c b/test/dm/dsa.c > index c857106eaf..147e2a4afe 100644 > --- a/test/dm/dsa.c > +++ b/test/dm/dsa.c > @@ -59,6 +59,7 @@ static int dm_test_dsa_probe(struct unit_test_state *uts) > > DM_TEST(dm_test_dsa_probe, UT_TESTF_SCAN_FDT); > > +#if !defined(CONFIG_NET_LWIP) > /* This test sends ping requests with the local address through each DSA port > * via the sandbox DSA master Eth. > */ > @@ -80,3 +81,4 @@ static int dm_test_dsa(struct unit_test_state *uts) > } > > DM_TEST(dm_test_dsa, UT_TESTF_SCAN_FDT); > +#endif /* !defined(CONFIG_NET_LWIP) */ > diff --git a/test/dm/eth.c b/test/dm/eth.c > index bb3dcc6b95..cf97b1c1ab 100644 > --- a/test/dm/eth.c > +++ b/test/dm/eth.c > @@ -170,6 +170,7 @@ static int dm_test_ip6_make_lladdr(struct unit_test_state *uts) > DM_TEST(dm_test_ip6_make_lladdr, UT_TESTF_SCAN_FDT); > #endif > > +#if !defined(CONFIG_NET_LWIP) > static int dm_test_eth(struct unit_test_state *uts) > { > net_ping_ip = string_to_ip("1.1.2.2"); > @@ -298,6 +299,7 @@ static int dm_test_eth_act(struct unit_test_state *uts) > return 0; > } > DM_TEST(dm_test_eth_act, UT_TESTF_SCAN_FDT); > +#endif /* !CONFIG_NET_LWIP */ > > /* Ensure that all addresses are loaded properly */ > static int dm_test_ethaddr(struct unit_test_state *uts) > @@ -332,6 +334,7 @@ static int dm_test_ethaddr(struct unit_test_state *uts) > } > DM_TEST(dm_test_ethaddr, UT_TESTF_SCAN_FDT); > > +#if !defined(CONFIG_NET_LWIP) > /* The asserts include a return on fail; cleanup in the caller */ > static int _dm_test_eth_rotate1(struct unit_test_state *uts) > { > @@ -616,6 +619,7 @@ static int dm_test_eth_async_ping_reply(struct unit_test_state *uts) > } > > DM_TEST(dm_test_eth_async_ping_reply, UT_TESTF_SCAN_FDT); > +#endif /* !CONFIG_NET_LWIP */ > > #if IS_ENABLED(CONFIG_IPV6_ROUTER_DISCOVERY) This seems like a lot of test disabling to me. And since a medium term goal would be to then drop the old network stack, a more nuanced approach is needed here. If not if/else'ing within the test, rewriting tests to cover the concept being tested needs to be done I think.
On Wed, 22 May 2024 at 19:08, Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote: > > Hi Jerome, > > On Wed, 22 May 2024 at 19:04, Jerome Forissier > <jerome.forissier@linaro.org> wrote: > > > > Some sandbox tests make strong assumptions on how the network stack is > > implemented. For example, the ping tests assume that ARP resolution > > occurs upon sending out the ICMP packet. This is not always the case > > with the lwIP stack which can cache ARP information. > > Therefore, disable these tests when CONFIG_NET_LWIP is enabled. > > Is the ARP Caching the only issue? > U-Boot isn't supposed to use the network stack that much, so it might > be a better idea to disable arp-caching in LWIP (assuming it's > doable). We might even get a few size of bytes back You end up hitting an arp cache a lot because of references to say DNS, IP GW or server end point for downloading, so while at first it doesn't appear you'd need it you also don't want to do ARP for every packet sent. Peter > Cheers > /Ilias > > > > Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org> > > --- > > test/dm/dsa.c | 2 ++ > > test/dm/eth.c | 4 ++++ > > 2 files changed, 6 insertions(+) > > > > diff --git a/test/dm/dsa.c b/test/dm/dsa.c > > index c857106eaf..147e2a4afe 100644 > > --- a/test/dm/dsa.c > > +++ b/test/dm/dsa.c > > @@ -59,6 +59,7 @@ static int dm_test_dsa_probe(struct unit_test_state *uts) > > > > DM_TEST(dm_test_dsa_probe, UT_TESTF_SCAN_FDT); > > > > +#if !defined(CONFIG_NET_LWIP) > > /* This test sends ping requests with the local address through each DSA port > > * via the sandbox DSA master Eth. > > */ > > @@ -80,3 +81,4 @@ static int dm_test_dsa(struct unit_test_state *uts) > > } > > > > DM_TEST(dm_test_dsa, UT_TESTF_SCAN_FDT); > > +#endif /* !defined(CONFIG_NET_LWIP) */ > > diff --git a/test/dm/eth.c b/test/dm/eth.c > > index bb3dcc6b95..cf97b1c1ab 100644 > > --- a/test/dm/eth.c > > +++ b/test/dm/eth.c > > @@ -170,6 +170,7 @@ static int dm_test_ip6_make_lladdr(struct unit_test_state *uts) > > DM_TEST(dm_test_ip6_make_lladdr, UT_TESTF_SCAN_FDT); > > #endif > > > > +#if !defined(CONFIG_NET_LWIP) > > static int dm_test_eth(struct unit_test_state *uts) > > { > > net_ping_ip = string_to_ip("1.1.2.2"); > > @@ -298,6 +299,7 @@ static int dm_test_eth_act(struct unit_test_state *uts) > > return 0; > > } > > DM_TEST(dm_test_eth_act, UT_TESTF_SCAN_FDT); > > +#endif /* !CONFIG_NET_LWIP */ > > > > /* Ensure that all addresses are loaded properly */ > > static int dm_test_ethaddr(struct unit_test_state *uts) > > @@ -332,6 +334,7 @@ static int dm_test_ethaddr(struct unit_test_state *uts) > > } > > DM_TEST(dm_test_ethaddr, UT_TESTF_SCAN_FDT); > > > > +#if !defined(CONFIG_NET_LWIP) > > /* The asserts include a return on fail; cleanup in the caller */ > > static int _dm_test_eth_rotate1(struct unit_test_state *uts) > > { > > @@ -616,6 +619,7 @@ static int dm_test_eth_async_ping_reply(struct unit_test_state *uts) > > } > > > > DM_TEST(dm_test_eth_async_ping_reply, UT_TESTF_SCAN_FDT); > > +#endif /* !CONFIG_NET_LWIP */ > > > > #if IS_ENABLED(CONFIG_IPV6_ROUTER_DISCOVERY) > > > > -- > > 2.40.1 > >
чт, 23 мая 2024 г. в 16:40, Peter Robinson <pbrobinson@gmail.com>: > > On Wed, 22 May 2024 at 19:08, Ilias Apalodimas > <ilias.apalodimas@linaro.org> wrote: > > > > Hi Jerome, > > > > On Wed, 22 May 2024 at 19:04, Jerome Forissier > > <jerome.forissier@linaro.org> wrote: > > > > > > Some sandbox tests make strong assumptions on how the network stack is > > > implemented. For example, the ping tests assume that ARP resolution > > > occurs upon sending out the ICMP packet. This is not always the case > > > with the lwIP stack which can cache ARP information. > > > Therefore, disable these tests when CONFIG_NET_LWIP is enabled. > > > > Is the ARP Caching the only issue? > > U-Boot isn't supposed to use the network stack that much, so it might > > be a better idea to disable arp-caching in LWIP (assuming it's > > doable). We might even get a few size of bytes back > > You end up hitting an arp cache a lot because of references to say > DNS, IP GW or server end point for downloading, so while at first it > doesn't appear you'd need it you also don't want to do ARP for every > packet sent. > > Peter > Caching has to be in the network stack. At least for 5-10 entries. There are 2 ways: or change the test or add flush/delete arp entries. Flushing and printing ARPs are useful for debugging and can go to the special Kconfig option. BR, Maxim. > > Cheers > > /Ilias > > > > > > Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org> > > > --- > > > test/dm/dsa.c | 2 ++ > > > test/dm/eth.c | 4 ++++ > > > 2 files changed, 6 insertions(+) > > > > > > diff --git a/test/dm/dsa.c b/test/dm/dsa.c > > > index c857106eaf..147e2a4afe 100644 > > > --- a/test/dm/dsa.c > > > +++ b/test/dm/dsa.c > > > @@ -59,6 +59,7 @@ static int dm_test_dsa_probe(struct unit_test_state *uts) > > > > > > DM_TEST(dm_test_dsa_probe, UT_TESTF_SCAN_FDT); > > > > > > +#if !defined(CONFIG_NET_LWIP) > > > /* This test sends ping requests with the local address through each DSA port > > > * via the sandbox DSA master Eth. > > > */ > > > @@ -80,3 +81,4 @@ static int dm_test_dsa(struct unit_test_state *uts) > > > } > > > > > > DM_TEST(dm_test_dsa, UT_TESTF_SCAN_FDT); > > > +#endif /* !defined(CONFIG_NET_LWIP) */ > > > diff --git a/test/dm/eth.c b/test/dm/eth.c > > > index bb3dcc6b95..cf97b1c1ab 100644 > > > --- a/test/dm/eth.c > > > +++ b/test/dm/eth.c > > > @@ -170,6 +170,7 @@ static int dm_test_ip6_make_lladdr(struct unit_test_state *uts) > > > DM_TEST(dm_test_ip6_make_lladdr, UT_TESTF_SCAN_FDT); > > > #endif > > > > > > +#if !defined(CONFIG_NET_LWIP) > > > static int dm_test_eth(struct unit_test_state *uts) > > > { > > > net_ping_ip = string_to_ip("1.1.2.2"); > > > @@ -298,6 +299,7 @@ static int dm_test_eth_act(struct unit_test_state *uts) > > > return 0; > > > } > > > DM_TEST(dm_test_eth_act, UT_TESTF_SCAN_FDT); > > > +#endif /* !CONFIG_NET_LWIP */ > > > > > > /* Ensure that all addresses are loaded properly */ > > > static int dm_test_ethaddr(struct unit_test_state *uts) > > > @@ -332,6 +334,7 @@ static int dm_test_ethaddr(struct unit_test_state *uts) > > > } > > > DM_TEST(dm_test_ethaddr, UT_TESTF_SCAN_FDT); > > > > > > +#if !defined(CONFIG_NET_LWIP) > > > /* The asserts include a return on fail; cleanup in the caller */ > > > static int _dm_test_eth_rotate1(struct unit_test_state *uts) > > > { > > > @@ -616,6 +619,7 @@ static int dm_test_eth_async_ping_reply(struct unit_test_state *uts) > > > } > > > > > > DM_TEST(dm_test_eth_async_ping_reply, UT_TESTF_SCAN_FDT); > > > +#endif /* !CONFIG_NET_LWIP */ > > > > > > #if IS_ENABLED(CONFIG_IPV6_ROUTER_DISCOVERY) > > > > > > -- > > > 2.40.1 > > >
diff --git a/test/dm/dsa.c b/test/dm/dsa.c index c857106eaf..147e2a4afe 100644 --- a/test/dm/dsa.c +++ b/test/dm/dsa.c @@ -59,6 +59,7 @@ static int dm_test_dsa_probe(struct unit_test_state *uts) DM_TEST(dm_test_dsa_probe, UT_TESTF_SCAN_FDT); +#if !defined(CONFIG_NET_LWIP) /* This test sends ping requests with the local address through each DSA port * via the sandbox DSA master Eth. */ @@ -80,3 +81,4 @@ static int dm_test_dsa(struct unit_test_state *uts) } DM_TEST(dm_test_dsa, UT_TESTF_SCAN_FDT); +#endif /* !defined(CONFIG_NET_LWIP) */ diff --git a/test/dm/eth.c b/test/dm/eth.c index bb3dcc6b95..cf97b1c1ab 100644 --- a/test/dm/eth.c +++ b/test/dm/eth.c @@ -170,6 +170,7 @@ static int dm_test_ip6_make_lladdr(struct unit_test_state *uts) DM_TEST(dm_test_ip6_make_lladdr, UT_TESTF_SCAN_FDT); #endif +#if !defined(CONFIG_NET_LWIP) static int dm_test_eth(struct unit_test_state *uts) { net_ping_ip = string_to_ip("1.1.2.2"); @@ -298,6 +299,7 @@ static int dm_test_eth_act(struct unit_test_state *uts) return 0; } DM_TEST(dm_test_eth_act, UT_TESTF_SCAN_FDT); +#endif /* !CONFIG_NET_LWIP */ /* Ensure that all addresses are loaded properly */ static int dm_test_ethaddr(struct unit_test_state *uts) @@ -332,6 +334,7 @@ static int dm_test_ethaddr(struct unit_test_state *uts) } DM_TEST(dm_test_ethaddr, UT_TESTF_SCAN_FDT); +#if !defined(CONFIG_NET_LWIP) /* The asserts include a return on fail; cleanup in the caller */ static int _dm_test_eth_rotate1(struct unit_test_state *uts) { @@ -616,6 +619,7 @@ static int dm_test_eth_async_ping_reply(struct unit_test_state *uts) } DM_TEST(dm_test_eth_async_ping_reply, UT_TESTF_SCAN_FDT); +#endif /* !CONFIG_NET_LWIP */ #if IS_ENABLED(CONFIG_IPV6_ROUTER_DISCOVERY)
Some sandbox tests make strong assumptions on how the network stack is implemented. For example, the ping tests assume that ARP resolution occurs upon sending out the ICMP packet. This is not always the case with the lwIP stack which can cache ARP information. Therefore, disable these tests when CONFIG_NET_LWIP is enabled. Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org> --- test/dm/dsa.c | 2 ++ test/dm/eth.c | 4 ++++ 2 files changed, 6 insertions(+)