Hello, Sean. I see. You are right. Reviewed-by: Viacheslav Mitrofanov <v.v.mitrofa...@yadro.com>
On Tue, 2024-05-07 at 22:16 -0700, Sean Edmond wrote: > > Hi Vyachesla, > > Let's start by saying neighbour discovery protocol is definitely > working > properly. I'm only suggesting that it isn't properly caching the MAC > address. During IPv6 TFTP I observe that neighbour discovery is > performed before every packet sent from client(u-boot)->server. > > Here's a snapshot of what the packets from client(u-boot)<->server > look > like during IPv6 TFTP: > -> neighbour solicitation > <- neighbour advertisement > -> read request > <- Block 0 > -> neighbour solicitation > <- neighbour advertisement > -> ACK block 0 > <- Block 1 > -> neighbour solicitation > <- neighbour advertisement > -> ACK block 1 > - ... (continues for entire file transfer) > > The neighbour discovery on every TX TFTP packet isn't required and > results in degraded performance. Note, when performing the test with > IPv4, ARP is not performed before every TX packet. > > Here's a description of the code flow (including my proposal): > - net.c defines "u8 net_server_ethaddr[6];" > - tftp_send()-> net_send_udp_packet6(net_server_ethaddr, ...) > - in net_send_udp_packet6(), "net_nd_packet_mac = ether;" (now, > net_nd_packet_mac is pointing to net_server_ethaddr) > - in ndisc_receive(), when NA is received the mac becomes available > in > neigh_eth_addr > - My proposal is, "if pointer net_nd_packet_mac isn't NULL, copy this > contents of neigh_eth_addr into net_nd_packet_mac" > - For TFTP, my fix allows the server's MAC to get copied into > net_server_ethaddr > - on the next tftp_send(), in net_send_udp_packet6() neighbour > discovery > is skipped because "ether" isn't all zeros > > The memcpy isn't dangerous, because it's copying the discovered mac > address into the already allocated net_server_ethaddr (and it's > checking > to make sure that net_nd_packet_mac isn't NULL before copying). > > Also, the current code serves no purpose. The current code is, "if > net_nd_packet_mac is NULL, set it to stack variable neigh_eth_addr, > then > set net_nd_packet_mac to NULL when there is no ND pending (the mac > address doesn't get saved in net_server_ethaddr). > > Sean > > > > > > > > > > > On 2024-05-05 2:40 a.m., Vyacheslav V. Mitrofanov wrote: > > On Mon, 2024-04-29 at 11:51 -0700, seanedm...@linux.microsoft.com > > wrote: > > > s...@yadro.com<mailto:s...@yadro.com> > > > > > > From: Sean Edmond <seanedm...@microsoft.com> > > > > > > When a successful neighbor advertisement is received, the > > > ethernet > > > address should be saved for later use to avoid having to redo the > > > neighbor discovery process. > > > > > > For example, with TFTP the address should get saved into > > > "net_server_ethaddr". This is being done correctly with ARP for > > > IPv4, > > > but not for neighbor discovery with IPv6. > > > > > > Signed-off-by: Sean Edmond <seanedm...@microsoft.com> > > > --- > > > net/ndisc.c | 4 ++-- > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > diff --git a/net/ndisc.c b/net/ndisc.c > > > index d1cec0601c8..505515f2d95 100644 > > > --- a/net/ndisc.c > > > +++ b/net/ndisc.c > > > @@ -461,8 +461,8 @@ int ndisc_receive(struct ethernet_hdr *et, > > > struct > > > ip6_hdr *ip6, int len) > > > ndisc_extract_enetaddr(ndisc, > > > neigh_eth_addr); > > > > > > /* save address for later use */ > > > - if (!net_nd_packet_mac) > > > - net_nd_packet_mac = > > > neigh_eth_addr; > > > + if (net_nd_packet_mac) > > > + memcpy(net_nd_packet_mac, > > > neigh_eth_addr, 6); > > > > > > /* modify header, and transmit it */ > > > memcpy(((struct ethernet_hdr > > > *)net_nd_tx_packet)->et_dest, > > > -- > > > 2.42.0 > > > > > Hello, Sean. Thanks for your notice. I see that net_nd_packet_mac > > is > > just a uchar pointer without memory allocation. It is dangerous to > > do > > memcpy and not necessary. All works as it has to be.
