[RFC] ieee1275: Avoiding many unnecessary open/close

* [RFC] ieee1275: Avoiding many unnecessary open/close
@ 2020-09-24 17:11 diegodo
  2020-09-24 23:58 ` Eric Snowberg
  0 siblings, 1 reply; 5+ messages in thread
From: diegodo @ 2020-09-24 17:11 UTC (permalink / raw)
  To: grub-devel

Hi,

we are facing some performance issues with ieee1275 platform (ppc64le 
architecture to be more specific) and I would like to suggest a couple 
of changes to try to avoid them.

Sometimes the system can take more than 10 minutes to boot (in fact, it 
can takes more than 30 and 40 minutes in some cases). From my 
investigations, I noticied the following points:

1. Some types of storage devices (NVMe, Fibre Channel, ...) can have a 
long time for shutdown notification. Our investigation here showed us 
this shutdown increase with the size of the storage. For example, 6.4TB 
NVMe can takes something like 7 seconds just to issue a shutdown 
notification.

2. The grub calls a lot for OPEN/READ/CLOSE for each device. I ran some 
tests with just one disk (NVMe 6.4TB) and found that, even with one 
disk, grub call the OPEN/CLOSE functions more than 60 times. Considering 
the opening time of the device something like 1.5 seconds, we have 
almost 9 minutes just opening and closing the disk.

I did some changes in the code and the time during the boot dropped 
drastically - from 492s to only 15s.

The idea is to handle the _actual close_ while _opening_ the disk. So, 
during the _grub_ofdisk_open_ function, we check if the requested disk 
is the same as the already opened and close the previous one if we are 
dealing with a different disk. Yet, I removed the OPEN/CLOSE calls 
inside the _grub_ofdisk_get_block_size_ function and let it just 
checking for the block size.

What are your suggestions? Is there a better way to handle this? I 
really appreciate your help here.

Thanks


---
  grub-core/disk/ieee1275/ofdisk.c | 63 +++++++++++++++++---------------
  1 file changed, 34 insertions(+), 29 deletions(-)

diff --git a/grub-core/disk/ieee1275/ofdisk.c 
b/grub-core/disk/ieee1275/ofdisk.c
index d887d4b..d0ee4ae 100644
--- a/grub-core/disk/ieee1275/ofdisk.c
+++ b/grub-core/disk/ieee1275/ofdisk.c
@@ -44,7 +44,7 @@ struct ofdisk_hash_ent
  };

  static grub_err_t
-grub_ofdisk_get_block_size (const char *device, grub_uint32_t 
*block_size,
+grub_ofdisk_get_block_size (grub_uint32_t *block_size,
  			    struct ofdisk_hash_ent *op);

  #define OFDISK_HASH_SZ	8
@@ -461,6 +461,7 @@ grub_ofdisk_open (const char *name, grub_disk_t 
disk)
    grub_ssize_t actual;
    grub_uint32_t block_size = 0;
    grub_err_t err;
+  struct ofdisk_hash_ent *op;

    if (grub_strncmp (name, "ieee1275/", sizeof ("ieee1275/") - 1) != 0)
        return grub_error (GRUB_ERR_UNKNOWN_DEVICE,
@@ -471,6 +472,34 @@ grub_ofdisk_open (const char *name, grub_disk_t 
disk)

    grub_dprintf ("disk", "Opening `%s'.\n", devpath);

+  op = ofdisk_hash_find (devpath);
+  if (!op)
+    op = ofdisk_hash_add (devpath, NULL);
+  if (!op)
+    {
+      grub_free (devpath);
+      return grub_errno;
+    }
+
+  /* Check if the call to open is the same to the last disk already 
opened */
+  if (last_devpath && !grub_strcmp(op->open_path,last_devpath))
+  {
+      goto finish;
+  }
+
+ /* If not, we need to close the previous disk and open the new one */
+  else {
+    if (last_ihandle){
+        grub_ieee1275_close (last_ihandle);
+    }
+    last_ihandle = 0;
+    last_devpath = NULL;
+
+    grub_ieee1275_open (op->open_path, &last_ihandle);
+    if (! last_ihandle)
+      return grub_error (GRUB_ERR_UNKNOWN_DEVICE, "can't open device");
+  }
+
    if (grub_ieee1275_finddevice (devpath, &dev))
      {
        grub_free (devpath);
@@ -491,25 +520,18 @@ grub_ofdisk_open (const char *name, grub_disk_t 
disk)
        return grub_error (GRUB_ERR_UNKNOWN_DEVICE, "not a block 
device");
      }

+
+  finish:
    /* XXX: There is no property to read the number of blocks.  There
       should be a property `#blocks', but it is not there.  Perhaps it
       is possible to use seek for this.  */
    disk->total_sectors = GRUB_DISK_SIZE_UNKNOWN;

    {
-    struct ofdisk_hash_ent *op;
-    op = ofdisk_hash_find (devpath);
-    if (!op)
-      op = ofdisk_hash_add (devpath, NULL);
-    if (!op)
-      {
-        grub_free (devpath);
-        return grub_errno;
-      }
      disk->id = (unsigned long) op;
      disk->data = op->open_path;

-    err = grub_ofdisk_get_block_size (devpath, &block_size, op);
+    err = grub_ofdisk_get_block_size (&block_size, op);
      if (err)
        {
          grub_free (devpath);
@@ -532,13 +554,6 @@ grub_ofdisk_open (const char *name, grub_disk_t 
disk)
  static void
  grub_ofdisk_close (grub_disk_t disk)
  {
-  if (disk->data == last_devpath)
-    {
-      if (last_ihandle)
-	grub_ieee1275_close (last_ihandle);
-      last_ihandle = 0;
-      last_devpath = NULL;
-    }
    disk->data = 0;
  }

@@ -685,7 +700,7 @@ grub_ofdisk_init (void)
  }

  static grub_err_t
-grub_ofdisk_get_block_size (const char *device, grub_uint32_t 
*block_size,
+grub_ofdisk_get_block_size (grub_uint32_t *block_size,
  			    struct ofdisk_hash_ent *op)
  {
    struct size_args_ieee1275
@@ -698,16 +713,6 @@ grub_ofdisk_get_block_size (const char *device, 
grub_uint32_t *block_size,
        grub_ieee1275_cell_t size2;
      } args_ieee1275;

-  if (last_ihandle)
-    grub_ieee1275_close (last_ihandle);
-
-  last_ihandle = 0;
-  last_devpath = NULL;
-
-  grub_ieee1275_open (device, &last_ihandle);
-  if (! last_ihandle)
-    return grub_error (GRUB_ERR_UNKNOWN_DEVICE, "can't open device");
-
    *block_size = 0;

    if (op->block_size_fails >= 2)
-- 
2.21.3


^ permalink raw reply related	[flat|nested] 5+ messages in thread