[PATCH] bug fix for efi network

[PATCH] bug fix for efi network

[Bean]

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

Hi,

This patch fix a few bugs in efinet.

It also change the tftp block size from 1024 to 8192, which would
result in HUGE speed difference. In my previous testing, the larger
the block size, the faster the speed. It can reach up to 60-70MB/s in
1Gb ethernet when block size is about 60K, therefore it would be a
good idea to allow user to configure this parameter. The native tftp
service in UEFI use a default block size of 8192.

-- 
Best wishes
Bean

[-- Attachment #2: efinet.txt --]
[-- Type: text/plain, Size: 1189 bytes --]

=== modified file 'grub-core/net/drivers/efi/efinet.c'
--- grub-core/net/drivers/efi/efinet.c	2012-03-10 19:41:28 +0000
+++ grub-core/net/drivers/efi/efinet.c	2012-04-29 08:12:12 +0000
@@ -63,14 +63,13 @@
   grub_efi_uintn_t bufsize = 1536;
   struct grub_net_buff *nb;
 
-  nb = grub_netbuff_alloc (bufsize);
+  nb = grub_netbuff_alloc (bufsize + 2);
   if (!nb)
     return NULL;
 
   /* Reserve 2 bytes so that 2 + 14/18 bytes of ethernet header is divisible
      by 4. So that IP header is aligned on 4 bytes. */
-  grub_netbuff_reserve (nb, 2);
-  if (!nb)
+  if (grub_netbuff_reserve (nb, 2))
     {
       grub_netbuff_free (nb);
       return NULL;

=== modified file 'grub-core/net/tftp.c'
--- grub-core/net/tftp.c	2012-02-12 18:11:06 +0000
+++ grub-core/net/tftp.c	2012-04-29 08:10:29 +0000
@@ -320,9 +320,9 @@
   rrqlen += grub_strlen ("blksize") + 1;
   rrq += grub_strlen ("blksize") + 1;
 
-  grub_strcpy (rrq, "1024");
-  rrqlen += grub_strlen ("1024") + 1;
-  rrq += grub_strlen ("1024") + 1;
+  grub_strcpy (rrq, "8192");
+  rrqlen += grub_strlen ("8192") + 1;
+  rrq += grub_strlen ("8192") + 1;
 
   grub_strcpy (rrq, "tsize");
   rrqlen += grub_strlen ("tsize") + 1;

Re: [PATCH] bug fix for efi network

[Bean]

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

Hi,

Sorry, the previous patch missed a similar bug.

On Sun, Apr 29, 2012 at 4:22 PM, Bean <bean123ch@gmail.com> wrote:
> Hi,
>
> This patch fix a few bugs in efinet.
>
> It also change the tftp block size from 1024 to 8192, which would
> result in HUGE speed difference. In my previous testing, the larger
> the block size, the faster the speed. It can reach up to 60-70MB/s in
> 1Gb ethernet when block size is about 60K, therefore it would be a
> good idea to allow user to configure this parameter. The native tftp
> service in UEFI use a default block size of 8192.
>
> --
> Best wishes
> Bean



-- 
Best wishes
Bean

[-- Attachment #2: efinet.txt --]
[-- Type: text/plain, Size: 1653 bytes --]

=== modified file 'grub-core/net/drivers/efi/efinet.c'
--- grub-core/net/drivers/efi/efinet.c	2012-03-10 19:41:28 +0000
+++ grub-core/net/drivers/efi/efinet.c	2012-04-29 08:23:58 +0000
@@ -63,14 +63,13 @@
   grub_efi_uintn_t bufsize = 1536;
   struct grub_net_buff *nb;
 
-  nb = grub_netbuff_alloc (bufsize);
+  nb = grub_netbuff_alloc (bufsize + 2);
   if (!nb)
     return NULL;
 
   /* Reserve 2 bytes so that 2 + 14/18 bytes of ethernet header is divisible
      by 4. So that IP header is aligned on 4 bytes. */
-  grub_netbuff_reserve (nb, 2);
-  if (!nb)
+  if (grub_netbuff_reserve (nb, 2))
     {
       grub_netbuff_free (nb);
       return NULL;
@@ -84,14 +83,13 @@
 
       bufsize = ALIGN_UP (bufsize, 32);
 
-      nb = grub_netbuff_alloc (bufsize);
+      nb = grub_netbuff_alloc (bufsize + 2);
       if (!nb)
 	return NULL;
 
       /* Reserve 2 bytes so that 2 + 14/18 bytes of ethernet header is divisible
 	 by 4. So that IP header is aligned on 4 bytes. */
-      grub_netbuff_reserve (nb, 2);
-      if (!nb)
+      if (grub_netbuff_reserve (nb, 2))
 	{
 	  grub_netbuff_free (nb);
 	  return NULL;

=== modified file 'grub-core/net/tftp.c'
--- grub-core/net/tftp.c	2012-02-12 18:11:06 +0000
+++ grub-core/net/tftp.c	2012-04-29 08:10:29 +0000
@@ -320,9 +320,9 @@
   rrqlen += grub_strlen ("blksize") + 1;
   rrq += grub_strlen ("blksize") + 1;
 
-  grub_strcpy (rrq, "1024");
-  rrqlen += grub_strlen ("1024") + 1;
-  rrq += grub_strlen ("1024") + 1;
+  grub_strcpy (rrq, "8192");
+  rrqlen += grub_strlen ("8192") + 1;
+  rrq += grub_strlen ("8192") + 1;
 
   grub_strcpy (rrq, "tsize");
   rrqlen += grub_strlen ("tsize") + 1;

Re: [PATCH] bug fix for efi network

[Bean]

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

Hi,

Update: I also rewrite the send_card_buffer function. Also add support
for tftp_block_size environment variable which allows user to specify
customized block size (default is 8192).

for example:

set tftp_block_size=16384
testspeed (tftp)/imagefile

When testing in virtual machine, try not to set block size too high
since it could cause timeouts, this is not an issue for real machine
since it normally have more nic buffer.

-- 
Best wishes
Bean

[-- Attachment #2: tftp.txt --]
[-- Type: text/plain, Size: 3792 bytes --]

=== modified file 'grub-core/net/drivers/efi/efinet.c'
--- grub-core/net/drivers/efi/efinet.c	2012-03-10 19:41:28 +0000
+++ grub-core/net/drivers/efi/efinet.c	2012-04-29 14:44:08 +0000
@@ -36,22 +36,55 @@
 {
   grub_efi_status_t st;
   grub_efi_simple_network_t *net = dev->efi_net;
-  grub_uint64_t limit_time = grub_get_time_ms () + 4000;
-  st = efi_call_7 (net->transmit, net, 0, (pack->tail - pack->data),
-		   pack->data, NULL, NULL, NULL);
-  if (st != GRUB_EFI_SUCCESS)
-    return grub_error (GRUB_ERR_IO, N_("couldn't send network packet"));
-  while (1)
+  grub_uint32_t int_status;
+  int i;
+
+  for (i = 0; i < 3; i++)
     {
+      grub_uint64_t limit_time;      
+      
+      efi_call_3 (net->get_status, net, &int_status, 0);
+
+      limit_time = grub_get_time_ms () + 5;
+      for (;;)
+	{
+	  st = efi_call_7 (net->transmit, net, 0, (pack->tail - pack->data),
+			   pack->data, NULL, NULL, NULL);
+	  if (st != GRUB_EFI_NOT_READY)
+	    break;
+
+	  if (limit_time < grub_get_time_ms ())
+	    {
+	      st = GRUB_EFI_TIMEOUT;
+	      break;
+	    }
+	}
+
+      if (st)
+	goto quit;
+
       void *txbuf = NULL;
-      st = efi_call_3 (net->get_status, net, 0, &txbuf);
-      if (st != GRUB_EFI_SUCCESS)
-	return grub_error (GRUB_ERR_IO, N_("couldn't send network packet"));
-      if (txbuf)
-	return GRUB_ERR_NONE;
-      if (limit_time < grub_get_time_ms ())
-	return grub_error (GRUB_ERR_TIMEOUT, N_("couldn't send network packet"));
+      limit_time = grub_get_time_ms () + 5;
+      for (;;)
+	{	 	  
+	  st = efi_call_3 (net->get_status, net, &int_status, &txbuf);
+
+	  if (txbuf != NULL)
+	    break;
+
+	  if (limit_time < grub_get_time_ms ())
+	    {
+	      st = GRUB_EFI_TIMEOUT;
+	      break;
+	    }
+	}
+
+    quit:
+      if (st != GRUB_EFI_TIMEOUT)
+	break;
     }
+
+  return st;
 }
 
 static struct grub_net_buff *
@@ -63,14 +96,13 @@
   grub_efi_uintn_t bufsize = 1536;
   struct grub_net_buff *nb;
 
-  nb = grub_netbuff_alloc (bufsize);
+  nb = grub_netbuff_alloc (bufsize + 2);
   if (!nb)
     return NULL;
 
   /* Reserve 2 bytes so that 2 + 14/18 bytes of ethernet header is divisible
      by 4. So that IP header is aligned on 4 bytes. */
-  grub_netbuff_reserve (nb, 2);
-  if (!nb)
+  if (grub_netbuff_reserve (nb, 2))
     {
       grub_netbuff_free (nb);
       return NULL;
@@ -84,14 +116,13 @@
 
       bufsize = ALIGN_UP (bufsize, 32);
 
-      nb = grub_netbuff_alloc (bufsize);
+      nb = grub_netbuff_alloc (bufsize + 2);
       if (!nb)
 	return NULL;
 
       /* Reserve 2 bytes so that 2 + 14/18 bytes of ethernet header is divisible
 	 by 4. So that IP header is aligned on 4 bytes. */
-      grub_netbuff_reserve (nb, 2);
-      if (!nb)
+      if (grub_netbuff_reserve (nb, 2))
 	{
 	  grub_netbuff_free (nb);
 	  return NULL;

=== modified file 'grub-core/net/tftp.c'
--- grub-core/net/tftp.c	2012-02-12 18:11:06 +0000
+++ grub-core/net/tftp.c	2012-04-29 14:31:12 +0000
@@ -27,6 +27,7 @@
 #include <grub/file.h>
 #include <grub/priority_queue.h>
 #include <grub/i18n.h>
+#include <grub/env.h>
 
 GRUB_MOD_LICENSE ("GPLv3+");
 
@@ -288,6 +289,7 @@
   grub_err_t err;
   grub_uint8_t *nbd;
   grub_net_network_level_address_t addr;
+  const char *block_size;
 
   data = grub_zalloc (sizeof (*data));
   if (!data)
@@ -320,9 +322,12 @@
   rrqlen += grub_strlen ("blksize") + 1;
   rrq += grub_strlen ("blksize") + 1;
 
-  grub_strcpy (rrq, "1024");
-  rrqlen += grub_strlen ("1024") + 1;
-  rrq += grub_strlen ("1024") + 1;
+  block_size = grub_env_get ("tftp_block_size");
+  if (block_size == NULL)
+    block_size = "8192";
+  grub_strcpy (rrq, block_size);
+  rrqlen += grub_strlen (block_size) + 1;
+  rrq += grub_strlen (block_size) + 1;
 
   grub_strcpy (rrq, "tsize");
   rrqlen += grub_strlen ("tsize") + 1;

Re: [PATCH] bug fix for efi network

[Vladimir 'φ-coder/phcoder' Serbinenko]

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

On 29.04.2012 17:05, Bean wrote:
> +  for (i = 0; i < 3; i++)
>      {
> +      grub_uint64_t limit_time;      
> +      
> +      efi_call_3 (net->get_status, net, &int_status, 0);
> +
> +      limit_time = grub_get_time_ms () + 5;
> +      for (;;)
> +	{
> +	  st = efi_call_7 (net->transmit, net, 0, (pack->tail - pack->data),
> +			   pack->data, NULL, NULL, NULL);
> +	  if (st != GRUB_EFI_NOT_READY)
> +	    break;
> +
> +	  if (limit_time < grub_get_time_ms ())
> +	    {
> +	      st = GRUB_EFI_TIMEOUT;
> +	      break;
> +	    }
> +	}
> +
> +      if (st)
> +	goto quit;
> +
>        void *txbuf = NULL;
> -      st = efi_call_3 (net->get_status, net, 0, &txbuf);
> -      if (st != GRUB_EFI_SUCCESS)
> -	return grub_error (GRUB_ERR_IO, N_("couldn't send network packet"));
> -      if (txbuf)
> -	return GRUB_ERR_NONE;
> -      if (limit_time < grub_get_time_ms ())
> -	return grub_error (GRUB_ERR_TIMEOUT, N_("couldn't send network packet"));
> +      limit_time = grub_get_time_ms () + 5;
> +      for (;;)
> +	{	 	  
> +	  st = efi_call_3 (net->get_status, net, &int_status, &txbuf);
> +
> +	  if (txbuf != NULL)
> +	    break;
> +
> +	  if (limit_time < grub_get_time_ms ())
> +	    {
> +	      st = GRUB_EFI_TIMEOUT;
> +	      break;
> +	    }
> +	}
> +
> +    quit:
> +      if (st != GRUB_EFI_TIMEOUT)
> +	break;
>      }
> +
> +  return st;
>  }
I see no justification as to why this part of patch so I can't know if
it's something which is we absolutely need to commit or something that
can wait.

-- 
Regards
Vladimir 'φ-coder/phcoder' Serbinenko



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 294 bytes --]

Re: [PATCH] bug fix for efi network

[Bean]

On Thu, May 3, 2012 at 11:38 PM, Vladimir 'φ-coder/phcoder' Serbinenko
<phcoder@gmail.com> wrote:
> On 29.04.2012 17:05, Bean wrote:
>> +  for (i = 0; i < 3; i++)
>>      {
>> +      grub_uint64_t limit_time;
>> +
>> +      efi_call_3 (net->get_status, net, &int_status, 0);
>> +
>> +      limit_time = grub_get_time_ms () + 5;
>> +      for (;;)
>> +     {
>> +       st = efi_call_7 (net->transmit, net, 0, (pack->tail - pack->data),
>> +                        pack->data, NULL, NULL, NULL);
>> +       if (st != GRUB_EFI_NOT_READY)
>> +         break;
>> +
>> +       if (limit_time < grub_get_time_ms ())
>> +         {
>> +           st = GRUB_EFI_TIMEOUT;
>> +           break;
>> +         }
>> +     }
>> +
>> +      if (st)
>> +     goto quit;
>> +
>>        void *txbuf = NULL;
>> -      st = efi_call_3 (net->get_status, net, 0, &txbuf);
>> -      if (st != GRUB_EFI_SUCCESS)
>> -     return grub_error (GRUB_ERR_IO, N_("couldn't send network packet"));
>> -      if (txbuf)
>> -     return GRUB_ERR_NONE;
>> -      if (limit_time < grub_get_time_ms ())
>> -     return grub_error (GRUB_ERR_TIMEOUT, N_("couldn't send network packet"));
>> +      limit_time = grub_get_time_ms () + 5;
>> +      for (;;)
>> +     {
>> +       st = efi_call_3 (net->get_status, net, &int_status, &txbuf);
>> +
>> +       if (txbuf != NULL)
>> +         break;
>> +
>> +       if (limit_time < grub_get_time_ms ())
>> +         {
>> +           st = GRUB_EFI_TIMEOUT;
>> +           break;
>> +         }
>> +     }
>> +
>> +    quit:
>> +      if (st != GRUB_EFI_TIMEOUT)
>> +     break;
>>      }
>> +
>> +  return st;
>>  }
> I see no justification as to why this part of patch so I can't know if
> it's something which is we absolutely need to commit or something that
> can wait.

Hi,

Yeah, this is not necessary, it's my custom code for the tftp service,
I just want to check if it makes a difference. In fact, the problem is
in the network code itself, not the low level driver.

-- 
Best wishes
Bean

Re: [PATCH] bug fix for efi network

[Vladimir 'φ-coder/phcoder' Serbinenko]

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

On 29.04.2012 10:22, Bean wrote:
> Hi,
>
> This patch fix a few bugs in efinet.
>
> It also change the tftp block size from 1024 to 8192, which would
> result in HUGE speed difference. In my previous testing, the larger
> the block size, the faster the speed. It can reach up to 60-70MB/s in
> 1Gb ethernet when block size is about 60K, therefore it would be a
> good idea to allow user to configure this parameter. The native tftp
> service in UEFI use a default block size of 8192.
8192 is bigger than the MTU on most cards so it results in fragmentation
or it emits jumbo packets. It's unclear how both network and server
would react to this.
Moreover the optimal packet size is in any case:
k * (mtu - ip_header_size - udp_header_size - tftp_header_size)
where k is some number. Only these number result in fully sized packets.
k=1 result in best available option for non-fragmented packets. k > 1
can create some preload by abusing fragmentation.
Note that some servers do preload by other means and k > 1 with such
servers would result in slowdown.
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel


-- 
Regards
Vladimir 'φ-coder/phcoder' Serbinenko



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 294 bytes --]

Re: [PATCH] bug fix for efi network

[Bean]

2012/4/30 Vladimir 'φ-coder/phcoder' Serbinenko <phcoder@gmail.com>:
> On 29.04.2012 10:22, Bean wrote:
>> Hi,
>>
>> This patch fix a few bugs in efinet.
>>
>> It also change the tftp block size from 1024 to 8192, which would
>> result in HUGE speed difference. In my previous testing, the larger
>> the block size, the faster the speed. It can reach up to 60-70MB/s in
>> 1Gb ethernet when block size is about 60K, therefore it would be a
>> good idea to allow user to configure this parameter. The native tftp
>> service in UEFI use a default block size of 8192.
> 8192 is bigger than the MTU on most cards so it results in fragmentation
> or it emits jumbo packets. It's unclear how both network and server
> would react to this.
> Moreover the optimal packet size is in any case:
> k * (mtu - ip_header_size - udp_header_size - tftp_header_size)
> where k is some number. Only these number result in fully sized packets.
> k=1 result in best available option for non-fragmented packets. k > 1
> can create some preload by abusing fragmentation.
> Note that some servers do preload by other means and k > 1 with such
> servers would result in slowdown.

Hi,

I believe server would split the packets into multiple fragment, each
within mtu to pass through network, and client assemble them to get
the original packet. Large block would reduce the number of
handshakes, and since nic has buffer to cache multiple packets, it
allows network driver and nic card to work in parallel and thus hide
some of the network traffic.

Perhaps users with real UEFI implementation can try out different
block size setting and reports the result.

-- 
Best wishes
Bean

Re: [PATCH] bug fix for efi network

[Vladimir 'φ-coder/phcoder' Serbinenko]

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

On 29.04.2012 20:19, Bean wrote:
> 2012/4/30 Vladimir 'φ-coder/phcoder' Serbinenko <phcoder@gmail.com>:
>> On 29.04.2012 10:22, Bean wrote:
>>> Hi,
>>>
>>> This patch fix a few bugs in efinet.
>>>
>>> It also change the tftp block size from 1024 to 8192, which would
>>> result in HUGE speed difference. In my previous testing, the larger
>>> the block size, the faster the speed. It can reach up to 60-70MB/s in
>>> 1Gb ethernet when block size is about 60K, therefore it would be a
>>> good idea to allow user to configure this parameter. The native tftp
>>> service in UEFI use a default block size of 8192.
>> 8192 is bigger than the MTU on most cards so it results in fragmentation
>> or it emits jumbo packets. It's unclear how both network and server
>> would react to this.
>> Moreover the optimal packet size is in any case:
>> k * (mtu - ip_header_size - udp_header_size - tftp_header_size)
>> where k is some number. Only these number result in fully sized packets.
>> k=1 result in best available option for non-fragmented packets. k > 1
>> can create some preload by abusing fragmentation.
>> Note that some servers do preload by other means and k > 1 with such
>> servers would result in slowdown.
> Hi,
>
> I believe server would split the packets into multiple fragment, each
> within mtu to pass through network, and client assemble them to get
> the original packet. Large block would reduce the number of
> handshakes, and since nic has buffer to cache multiple packets, it
> allows network driver and nic card to work in parallel and thus hide
> some of the network traffic.
But the fragmentation/defragmentation is also something that routers do.
While I'm pretty sure none of them crashes on such packets since the
ping of death, some of them may mishandle such packets or reject them as
an "attack attempt". While this is an interesting possibility to try
out, we need more data as to its functioning in real-world scenarios. We
would also need a way to reestablish connection with a small block if we
notice that the packets aren't comming through or are corrupted.
> Perhaps users with real UEFI implementation can try out different
> block size setting and reports the result.
>


-- 
Regards
Vladimir 'φ-coder/phcoder' Serbinenko



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 294 bytes --]