Syslinux - May 2017 - [PATCH 1/4] efi/udp: core_udp_connect should use SubnetMask not StationAddress for netmask

Signed-off-by: Julien Viard de Galbert <jviarddegalbert at online.net>
---
 efi/udp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/efi/udp.c b/efi/udp.c
index 1088f47..b0f13ad 100644
--- a/efi/udp.c
+++ b/efi/udp.c
@@ -163,7 +163,7 @@ void core_udp_connect(struct pxe_pvt_inode *socket, uint32_t
ip,
     } else {
 	udata.UseDefaultAddress = FALSE;
 	memcpy(&udata.StationAddress, &IPInfo.myip, sizeof(IPInfo.myip));
-	memcpy(&udata.StationAddress, &IPInfo.netmask,
sizeof(IPInfo.netmask));
+	memcpy(&udata.SubnetMask, &IPInfo.netmask, sizeof(IPInfo.netmask));
     }
     memcpy(&udata.RemoteAddress, &ip, sizeof(ip));
     udata.RemotePort = port;
-- 
2.9.3

Signed-off-by: Julien Viard de Galbert <jviarddegalbert at online.net>
---
 efi/udp.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/efi/udp.c b/efi/udp.c
index b0f13ad..8f4d7dc 100644
--- a/efi/udp.c
+++ b/efi/udp.c
@@ -52,7 +52,7 @@ EFI_STATUS core_udp_configure(EFI_UDP4 *udp,
EFI_UDP4_CONFIG_DATA *udata,
 	    }
 	} else {
 	    if (status != EFI_SUCCESS) {
-		Print(L"%s: udp->Configure() unsuccessful (%d)", f, status);
+		Print(L"%s: udp->Configure() unsuccessful (%d)\n", f, status);
 		if (!efi_net_def_addr && (status == EFI_INVALID_PARAMETER))
 		    efi_net_def_addr = 2;
 	    }
@@ -158,6 +158,7 @@ void core_udp_connect(struct pxe_pvt_inode *socket, uint32_t
ip,
     /* Re-use the existing local port number */
     udata.StationPort = socket->net.efi.localport;
 
+retry:
     if (efi_net_def_addr) {
 	udata.UseDefaultAddress = TRUE;
     } else {
@@ -170,6 +171,11 @@ void core_udp_connect(struct pxe_pvt_inode *socket,
uint32_t ip,
     udata.TimeToLive = 64;
 
     status = core_udp_configure(udp, &udata,
L"core_udp_connect");
+    if (efi_net_def_addr && (status == EFI_NO_MAPPING)) {
+	efi_net_def_addr = 0;
+	Print(L"disable UseDefaultAddress\n");
+	goto retry;
+    }
     if (status != EFI_SUCCESS) {
 	Print(L"Failed to configure UDP: %d\n", status);
 	return;
@@ -392,6 +398,7 @@ void core_udp_sendto(struct pxe_pvt_inode *socket, const
void *data,
     /* Re-use the existing local port number */
     udata.StationPort = socket->net.efi.localport;
 
+retry:
     if (efi_net_def_addr) {
 	udata.UseDefaultAddress = TRUE;
     } else {
@@ -404,6 +411,11 @@ void core_udp_sendto(struct pxe_pvt_inode *socket, const
void *data,
     udata.TimeToLive = 64;
 
     status = core_udp_configure(udp, &udata, L"core_udp_sendto");
+    if (efi_net_def_addr && (status == EFI_NO_MAPPING)) {
+	efi_net_def_addr = 0;
+	Print(L"disable UseDefaultAddress\n");
+	goto retry;
+    }
     if (status != EFI_SUCCESS)
 	goto bail;
 
-- 
2.9.3

This extra socket cause the EFI udp stack to duplicate every packet
to be able to provide them to both udp_reader and socket->net.efi.binding.
The ones in socket->net.efi.binding were never read so they accumulated in
the stack on EFI side making the stack slower and slower.
Slow enough to not be able to download a kernel+initramfs before the EFI
timeout (5 minutes).

Signed-off-by: Julien Viard de Galbert <jviarddegalbert at online.net>
---
 efi/udp.c | 24 ++----------------------
 1 file changed, 2 insertions(+), 22 deletions(-)

diff --git a/efi/udp.c b/efi/udp.c
index 8f4d7dc..4032d0b 100644
--- a/efi/udp.c
+++ b/efi/udp.c
@@ -10,14 +10,6 @@
 
 extern EFI_GUID Udp4ServiceBindingProtocol, Udp4Protocol;
 
-/*
- * This UDP binding is configured to operate in promiscuous mode. It is
- * only used for reading packets. It has no associated state unlike
- * socket->net.efi.binding, which has a remote IP address and port
- * number.
- */
-static struct efi_binding *udp_reader;
-
 static int volatile efi_udp_has_recv = 0;
 int volatile efi_net_def_addr = 1;
 
@@ -76,17 +68,11 @@ int core_udp_open(struct pxe_pvt_inode *socket)
     EFI_STATUS status;
     EFI_UDP4 *udp;
 
-    (void)socket;
-
-    udp_reader = efi_create_binding(&Udp4ServiceBindingProtocol,
&Udp4Protocol);
-    if (!udp_reader)
-	return -1;
-
     b = efi_create_binding(&Udp4ServiceBindingProtocol, &Udp4Protocol);
     if (!b)
 	goto bail;
 
-    udp = (EFI_UDP4 *)udp_reader->this;
+    udp = (EFI_UDP4 *)b->this;
 
     memset(&udata, 0, sizeof(udata));
 
@@ -114,9 +100,6 @@ bail:
     if (b)
 	efi_destroy_binding(b, &Udp4ServiceBindingProtocol);
 
-    efi_destroy_binding(udp_reader, &Udp4ServiceBindingProtocol);
-    udp_reader = NULL;
-
     return -1;
 }
 
@@ -127,9 +110,6 @@ bail:
  */
 void core_udp_close(struct pxe_pvt_inode *socket)
 {
-    efi_destroy_binding(udp_reader, &Udp4ServiceBindingProtocol);
-    udp_reader = NULL;
-
     if (!socket->net.efi.binding)
 	return;
 
@@ -239,7 +219,7 @@ int core_udp_recv(struct pxe_pvt_inode *socket, void *buf,
uint16_t *buf_len,
 
     (void)socket;
 
-    b = udp_reader;
+    b = socket->net.efi.binding;
     udp = (EFI_UDP4 *)b->this;
     memset(&token, 0, sizeof(token));
 
-- 
2.9.3

So now the total timeout for 'stalling on configure with no mapping' is
1 sec

Signed-off-by: Julien Viard de Galbert <jviarddegalbert at online.net>
---
 core/fs/pxe/tftp.c | 22 +++++++++++-----------
 efi/tcp.c          |  6 +++---
 efi/udp.c          | 14 +++++++-------
 3 files changed, 21 insertions(+), 21 deletions(-)

diff --git a/core/fs/pxe/tftp.c b/core/fs/pxe/tftp.c
index 594152b..b507761 100644
--- a/core/fs/pxe/tftp.c
+++ b/core/fs/pxe/tftp.c
@@ -82,7 +82,7 @@ static void tftp_get_packet(struct inode *inode)
     uint8_t timeout;
     uint16_t buffersize;
     uint16_t serial;
-    jiffies_t oldtime;
+    mstime_t oldtime;
     struct tftp_packet *pkt = NULL;
     uint16_t buf_len;
     struct pxe_pvt_inode *socket = PVT(inode);
@@ -96,7 +96,7 @@ static void tftp_get_packet(struct inode *inode)
      */
     timeout_ptr = TimeoutTable;
     timeout = *timeout_ptr++;
-    oldtime = jiffies();
+    oldtime = ms_timer();
 
  ack_again:
     ack_packet(inode, socket->tftp_lastpkt);
@@ -106,7 +106,7 @@ static void tftp_get_packet(struct inode *inode)
 	err = core_udp_recv(socket, socket->tftp_pktbuf, &buf_len,
 			    &src_ip, &src_port);
 	if (err) {
-	    jiffies_t now = jiffies();
+	    mstime_t now = ms_timer();
 
 	    if (now-oldtime >= timeout) {
 		oldtime = now;
@@ -198,8 +198,8 @@ void tftp_open(struct url_info *url, int flags, struct inode
*inode,
     int buffersize;
     int rrq_len;
     const uint8_t  *timeout_ptr;
-    jiffies_t timeout;
-    jiffies_t oldtime;
+    mstime_t timeout;
+    mstime_t oldtime;
     uint16_t opcode;
     uint16_t blk_num;
     uint64_t opdata;
@@ -241,7 +241,7 @@ sendreq:
     timeout = *timeout_ptr++;
     if (!timeout)
 	return;			/* No file available... */
-    oldtime = jiffies();
+    oldtime = ms_timer();
 
     core_udp_sendto(socket, rrq_packet_buf, rrq_len, url->ip, url->port);
 
@@ -253,7 +253,7 @@ wait_pkt:
 	err = core_udp_recv(socket, reply_packet_buf, &buf_len,
 			    &src_ip, &src_port);
 	if (err) {
-	    jiffies_t now = jiffies();
+	    mstime_t now = ms_timer();
 	    if (now - oldtime >= timeout)
 		 goto sendreq;
 	} else {
@@ -448,8 +448,8 @@ __export int tftp_put(struct url_info *url, int flags,
struct inode *inode,
     int err;
     int wrq_len;
     const uint8_t  *timeout_ptr;
-    jiffies_t timeout;
-    jiffies_t oldtime;
+    mstime_t timeout;
+    mstime_t oldtime;
     uint16_t opcode;
     uint16_t src_port = url->port;
     uint32_t src_ip;
@@ -493,7 +493,7 @@ sendreq:
     timeout = *timeout_ptr++;
     if (!timeout)
 	return return_code;			/* No file available... */
-    oldtime = jiffies();
+    oldtime = ms_timer();
 
     core_udp_sendto(socket, wrq_packet_buf, wrq_len, url->ip, src_port);
 
@@ -504,7 +504,7 @@ sendreq:
 	err = core_udp_recv(socket, reply_packet_buf, &buf_len,
 			    &src_ip, &src_port);
 	if (err) {
-	    jiffies_t now = jiffies();
+	    mstime_t now = ms_timer();
 	    if (now - oldtime >= timeout)
 		 goto sendreq;
 	} else {
diff --git a/efi/tcp.c b/efi/tcp.c
index 0e9140e..b252ad0 100644
--- a/efi/tcp.c
+++ b/efi/tcp.c
@@ -57,7 +57,7 @@ int core_tcp_connect(struct pxe_pvt_inode *socket, uint32_t
ip, uint16_t port)
     EFI_TCP4 *tcp = (EFI_TCP4 *)b->this;
     int rv = -1;
     int unmapped = 1;
-    jiffies_t start, last, cur;
+    mstime_t start, last, cur;
 
     memset(&tdata, 0, sizeof(tdata));
 
@@ -69,11 +69,11 @@ int core_tcp_connect(struct pxe_pvt_inode *socket, uint32_t
ip, uint16_t port)
 
     tdata.TimeToLive = 64;
 
-    last = start = jiffies();
+    last = start = ms_timer();
     while (unmapped){
 	status = uefi_call_wrapper(tcp->Configure, 2, tcp, &tdata);
 	if (status == EFI_NO_MAPPING) {
-	    cur = jiffies();
+	    cur = ms_timer();
 	    if ( (cur - last) >= EFI_NOMAP_PRINT_DELAY ) {
 		last = cur;
 		Print(L"core_tcp_connect: stalling on configure with no
mapping\n");
diff --git a/efi/udp.c b/efi/udp.c
index 4032d0b..d23c884 100644
--- a/efi/udp.c
+++ b/efi/udp.c
@@ -28,13 +28,13 @@ EFI_STATUS core_udp_configure(EFI_UDP4 *udp,
EFI_UDP4_CONFIG_DATA *udata,
 {
     EFI_STATUS status;
     int unmapped = 1;
-    jiffies_t start, last, cur;
+    mstime_t start, last, cur;
 
-    last = start = jiffies();
+    last = start = ms_timer();
     while (unmapped){
 	status = uefi_call_wrapper(udp->Configure, 2, udp, udata);
 	if (status == EFI_NO_MAPPING) {
-	    cur = jiffies();
+	    cur = ms_timer();
 	    if ( (cur - last) >= EFI_NOMAP_PRINT_DELAY ) {
 		last = cur;
 		Print(L"%s: stalling on configure with no mapping\n", f);
@@ -215,7 +215,7 @@ int core_udp_recv(struct pxe_pvt_inode *socket, void *buf,
uint16_t *buf_len,
     EFI_UDP4 *udp;
     size_t size;
     int rv = -1;
-    jiffies_t start;
+    mstime_t start;
 
     (void)socket;
 
@@ -232,11 +232,11 @@ int core_udp_recv(struct pxe_pvt_inode *socket, void *buf,
uint16_t *buf_len,
     if (status != EFI_SUCCESS)
 	goto bail;
 
-    start = jiffies();
+    start = ms_timer();
     while (cb_status == -1) {
 	/* 15ms receive timeout... */
-	if (jiffies() - start >= 15) {
-	    if (jiffies() - start >= 30)
+	if (ms_timer() - start >= 15) {
+	    if (ms_timer() - start >= 30)
 		dprintf("Failed to cancel UDP\n");
 
 	    uefi_call_wrapper(udp->Cancel, 2, udp, &token);
-- 
2.9.3

On Wed, May 31, 2017 at 3:56 AM, Julien Viard de Galbert via Syslinux
<syslinux at zytor.com> wrote:
> Signed-off-by: Julien Viard de Galbert <jviarddegalbert at
online.net>
> ---
>  efi/udp.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/efi/udp.c b/efi/udp.c
> index 1088f47..b0f13ad 100644
> --- a/efi/udp.c
> +++ b/efi/udp.c
> @@ -163,7 +163,7 @@ void core_udp_connect(struct pxe_pvt_inode *socket,
uint32_t ip,
>      } else {
>         udata.UseDefaultAddress = FALSE;
>         memcpy(&udata.StationAddress, &IPInfo.myip,
sizeof(IPInfo.myip));
> -       memcpy(&udata.StationAddress, &IPInfo.netmask,
sizeof(IPInfo.netmask));
> +       memcpy(&udata.SubnetMask, &IPInfo.netmask,
sizeof(IPInfo.netmask));
>      }
>      memcpy(&udata.RemoteAddress, &ip, sizeof(ip));
>      udata.RemotePort = port;
Thanks for catching this.  Done.

-- 
-Gene

On Wed, May 31, 2017 at 3:56 AM, Julien Viard de Galbert via Syslinux
<syslinux at zytor.com> wrote:
> Signed-off-by: Julien Viard de Galbert <jviarddegalbert at
online.net>
> ---
>  efi/udp.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
>
Excellent idea to trap on EFI_NO_MAPPING and fall back.  Done.

-- 
-Gene

On Wed, May 31, 2017 at 3:56 AM, Julien Viard de Galbert via Syslinux
<syslinux at zytor.com> wrote:
> This extra socket cause the EFI udp stack to duplicate every packet
> to be able to provide them to both udp_reader and
socket->net.efi.binding.
> The ones in socket->net.efi.binding were never read so they accumulated
in
> the stack on EFI side making the stack slower and slower.
> Slow enough to not be able to download a kernel+initramfs before the EFI
> timeout (5 minutes).
>
> Signed-off-by: Julien Viard de Galbert <jviarddegalbert at
online.net>
> ---
>  efi/udp.c | 24 ++----------------------
>  1 file changed, 2 insertions(+), 22 deletions(-)
That almost certainly explains the decaying download rates.
>  extern EFI_GUID Udp4ServiceBindingProtocol, Udp4Protocol;
>
> -/*
> - * This UDP binding is configured to operate in promiscuous mode. It is
> - * only used for reading packets. It has no associated state unlike
> - * socket->net.efi.binding, which has a remote IP address and port
> - * number.
> - */
This part has me puzzled why Matt was creating a promiscuous binding
instead of re-using the existing binding.

-- 
-Gene