[PATCH 0/3] Net fix and improvements

[PATCH 0/3] Net fix and improvements

[Glenn Washburn]

The first patch looks like it was a copy/paste error. If the net module is
unloaded, grub_net_poll_cards_idle should be NULL so that a function in the
net module which now doesn't exist.

The second and third patches are for performance and were helpful when
debugging GRUB. When the net module is loaded, even if there are no net
cards found, grub_net_poll_cards_idle will call grub_net_tcp_retransmit()
unconditionally. But there's no need to go through tcp retransmit logic if
there aren't any cards that we could be retransmitting on.

As for the third patch, if the machine has network cards found, but there are
no tcp open sockets (because the user doesn't use the network to boot), then
grub_net_tcp_retransmit() should be a noop. Thus is doesn't need to call
grub_get_time_ms(), which does a call into firmware on powerpc-ieee1275. So
only call grub_get_time_ms() if there are tcp sockets.

Calls to grub_net_poll_cards_idle can happen a lot when GRUB is waiting for a
keypress (grub_getkey_noblock calls grub_net_poll_cards_idle). I found this
annoying when debugging an issue in GRUB with GDB and I found that nearly
every time I interrupted GRUB with the GDB I was landing in OpenBIOS's
milliseconds call and then I had to step out back into GRUB which could be
a hundred or more instructions. This reduces the probability that interrupting
GRUB lands in the firmware when GRUB is blocked waiting on a keypress.

Glenn

Glenn Washburn (3):
  net: Unset grub_net_poll_cards_idle when net module has been unloaded
  net: Avoid unnecessary calls to grub_net_tcp_retransmit
  net/tcp: Only call grub_get_time_ms when there are sockets to
    potentially retransmit for

 grub-core/net/net.c | 5 +++--
 grub-core/net/tcp.c | 9 +++++++--
 2 files changed, 10 insertions(+), 4 deletions(-)

-- 
2.27.0

[PATCH 1/3] net: Unset grub_net_poll_cards_idle when net module has been unloaded

[Glenn Washburn]

Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 grub-core/net/net.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/grub-core/net/net.c b/grub-core/net/net.c
index 4d3eb5c1a5..563dea9ec8 100644
--- a/grub-core/net/net.c
+++ b/grub-core/net/net.c
@@ -1948,5 +1948,5 @@ GRUB_MOD_FINI(net)
   grub_net_open = NULL;
   grub_net_fini_hw (0);
   grub_loader_unregister_preboot_hook (fini_hnd);
-  grub_net_poll_cards_idle = grub_net_poll_cards_idle_real;
+  grub_net_poll_cards_idle = NULL;
 }
-- 
2.27.0

[PATCH 2/3] net: Avoid unnecessary calls to grub_net_tcp_retransmit

[Glenn Washburn]

In grub_net_poll_cards_idle_real, only call grub_net_tcp_retransmit if there
are network cards found. If there are no network card found, there can be no
tcp sockets to transmit on.

Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 grub-core/net/net.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/grub-core/net/net.c b/grub-core/net/net.c
index 563dea9ec8..e208b84019 100644
--- a/grub-core/net/net.c
+++ b/grub-core/net/net.c
@@ -1580,7 +1580,8 @@ grub_net_poll_cards_idle_real (void)
 	|| ctime >= card->last_poll + card->idle_poll_delay_ms)
       receive_packets (card, 0);
   }
-  grub_net_tcp_retransmit ();
+  if (grub_net_cards)
+    grub_net_tcp_retransmit ();
 }
 
 /*  Read from the packets list*/
-- 
2.27.0

[PATCH 3/3] net/tcp: Only call grub_get_time_ms when there are sockets to potentially retransmit for

[Glenn Washburn]

If there are no TCP sockets, this call to grub_get_time_ms is unneeded. This
prevents a call into the firmware on some platforms.

Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 grub-core/net/tcp.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/grub-core/net/tcp.c b/grub-core/net/tcp.c
index e8ad34b84d..75461eb54e 100644
--- a/grub-core/net/tcp.c
+++ b/grub-core/net/tcp.c
@@ -362,8 +362,13 @@ void
 grub_net_tcp_retransmit (void)
 {
   grub_net_tcp_socket_t sock;
-  grub_uint64_t ctime = grub_get_time_ms ();
-  grub_uint64_t limit_time = ctime - TCP_RETRANSMISSION_TIMEOUT;
+  grub_uint64_t ctime = 0, limit_time = 0;
+
+  if (tcp_sockets)
+    {
+      ctime = grub_get_time_ms ();
+      limit_time = ctime - TCP_RETRANSMISSION_TIMEOUT;
+    }
 
   FOR_TCP_SOCKETS (sock)
   {
-- 
2.27.0

Re: [PATCH 0/3] Net fix and improvements

[Daniel Kiper]

Hey,

On Tue, Mar 08, 2022 at 03:20:17PM -0600, Glenn Washburn wrote:
> The first patch looks like it was a copy/paste error. If the net module is
> unloaded, grub_net_poll_cards_idle should be NULL so that a function in the
> net module which now doesn't exist.
>
> The second and third patches are for performance and were helpful when
> debugging GRUB. When the net module is loaded, even if there are no net
> cards found, grub_net_poll_cards_idle will call grub_net_tcp_retransmit()
> unconditionally. But there's no need to go through tcp retransmit logic if
> there aren't any cards that we could be retransmitting on.
>
> As for the third patch, if the machine has network cards found, but there are
> no tcp open sockets (because the user doesn't use the network to boot), then
> grub_net_tcp_retransmit() should be a noop. Thus is doesn't need to call
> grub_get_time_ms(), which does a call into firmware on powerpc-ieee1275. So
> only call grub_get_time_ms() if there are tcp sockets.
>
> Calls to grub_net_poll_cards_idle can happen a lot when GRUB is waiting for a
> keypress (grub_getkey_noblock calls grub_net_poll_cards_idle). I found this
> annoying when debugging an issue in GRUB with GDB and I found that nearly
> every time I interrupted GRUB with the GDB I was landing in OpenBIOS's
> milliseconds call and then I had to step out back into GRUB which could be
> a hundred or more instructions. This reduces the probability that interrupting
> GRUB lands in the firmware when GRUB is blocked waiting on a keypress.

In general I think this cover letter should be split into three parts and they
should go into relevant patches.

Additionally, if you check pointers for NULL please use "== NULL"
comparison explicitly instead of shortened version like "if (pointer)".

Otherwise patches LGTM.

Daniel

Re: [PATCH 0/3] Net fix and improvements

[Daniel Kiper]

On Fri, Mar 18, 2022 at 01:51:30AM -0500, Glenn Washburn wrote:
> v2 updates:
>  * Use == NULL in conditionals
>  * Update commit messages to have more context from previous cover letter
>
> Glenn
>
> Glenn Washburn (3):
>   net: Unset grub_net_poll_cards_idle when net module has been unloaded
>   net: Avoid unnecessary calls to grub_net_tcp_retransmit
>   net/tcp: Only call grub_get_time_ms when there are sockets to
>     potentially retransmit for

For all patches Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>...

Daniel

[PATCH 0/3] Net fix and improvements

[Glenn Washburn]

v2 updates:
 * Use == NULL in conditionals
 * Update commit messages to have more context from previous cover letter

Glenn

Glenn Washburn (3):
  net: Unset grub_net_poll_cards_idle when net module has been unloaded
  net: Avoid unnecessary calls to grub_net_tcp_retransmit
  net/tcp: Only call grub_get_time_ms when there are sockets to
    potentially retransmit for

 grub-core/net/net.c | 5 +++--
 grub-core/net/tcp.c | 9 +++++++--
 2 files changed, 10 insertions(+), 4 deletions(-)

Range-diff:
1:  324f068453 ! 1:  76bbc12ed1 net: Unset grub_net_poll_cards_idle when net module has been unloaded
    @@ Metadata
      ## Commit message ##
         net: Unset grub_net_poll_cards_idle when net module has been unloaded
     
    +    This looks like it was a copy/paste error. If the net module is unloaded,
    +    grub_net_poll_cards_idle should be NULL so that GRUB does not try to call a
    +    function which now doesn't exist.
    +
      ## grub-core/net/net.c ##
     @@ grub-core/net/net.c: GRUB_MOD_FINI(net)
        grub_net_open = NULL;
2:  2e55f210c7 ! 2:  4efca58dab net: Avoid unnecessary calls to grub_net_tcp_retransmit
    @@ Commit message
     
         In grub_net_poll_cards_idle_real, only call grub_net_tcp_retransmit if there
         are network cards found. If there are no network card found, there can be no
    -    tcp sockets to transmit on.
    +    tcp sockets to transmit on. So no need to go through that logic.
     
      ## grub-core/net/net.c ##
     @@ grub-core/net/net.c: grub_net_poll_cards_idle_real (void)
    @@ grub-core/net/net.c: grub_net_poll_cards_idle_real (void)
            receive_packets (card, 0);
        }
     -  grub_net_tcp_retransmit ();
    -+  if (grub_net_cards)
    ++  if (grub_net_cards == NULL)
     +    grub_net_tcp_retransmit ();
      }
      
3:  7bef6e122f ! 3:  a6328b14cd net/tcp: Only call grub_get_time_ms when there are sockets to potentially retransmit for
    @@ Metadata
      ## Commit message ##
         net/tcp: Only call grub_get_time_ms when there are sockets to potentially retransmit for
     
    -    If there are no TCP sockets, this call to grub_get_time_ms is unneeded. This
    -    prevents a call into the firmware on some platforms.
    +    If the machine has network cards found, but there are no tcp open sockets
    +    (because the user doesn't use the network to boot), then
    +    grub_net_tcp_retransmit() should be a noop. Thus GRUB doesn't need to call
    +    grub_get_time_ms(), which does a call into firmware on powerpc-ieee1275,
    +    and probably other targets. So only call grub_get_time_ms() if there are tcp
    +    sockets.
    +
    +    Aside from improving performace, its also useful to stay out of the firmware
    +    as much as possible when debugging via QEMU because its a pain to get back
    +    in to GRUB execution. grub_net_tcp_retransmit() can get called very
    +    frequently via grub_net_poll_cards_idle() when GRUB is waiting for a
    +    keypress (grub_getkey_noblock() calls grub_net_poll_cards_idle()). This can
    +    be annoying when debugging an issue in GRUB on powerpc in QEMU with GDB when
    +    GRUB is waiting for a keypress because interrupting via GDB nearly always
    +    lands in the OpenBIOS firmware's milliseconds call.
     
      ## grub-core/net/tcp.c ##
     @@ grub-core/net/tcp.c: void
    @@ grub-core/net/tcp.c: void
     -  grub_uint64_t limit_time = ctime - TCP_RETRANSMISSION_TIMEOUT;
     +  grub_uint64_t ctime = 0, limit_time = 0;
     +
    -+  if (tcp_sockets)
    ++  if (tcp_sockets == NULL)
     +    {
     +      ctime = grub_get_time_ms ();
     +      limit_time = ctime - TCP_RETRANSMISSION_TIMEOUT;
-- 
2.27.0