* [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
* Re: [RFC] ieee1275: Avoiding many unnecessary open/close 2020-09-24 17:11 [RFC] ieee1275: Avoiding many unnecessary open/close diegodo @ 2020-09-24 23:58 ` Eric Snowberg 2020-11-16 17:29 ` diegodo 0 siblings, 1 reply; 5+ messages in thread From: Eric Snowberg @ 2020-09-24 23:58 UTC (permalink / raw) To: The development of GNU GRUB; +Cc: diegodo > On Sep 24, 2020, at 11:11 AM, diegodo <diegodo@linux.vnet.ibm.com> wrote: > > 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. I ran into the same issue with the ieee1275 platform on SPARC when using ofdisk. It would take 20+ minutes to get to the GRUB menu. I found the same problem, open and close can take a long time and upper level grub code would issue hundreds of them. I ended up writing obdisk.c. Within it, each disk is only opened once. I tried to make it as generic as possible. You might consider moving from ofdisk to obdisk. If you did, ppc64le device specific code would need to be included in iterate_devtree. It also includes new features not found in ofdisk, such as hot-plug support and the ability to find disks that don’t have a device alias. > 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 > > _______________________________________________ > Grub-devel mailing list > Grub-devel@gnu.org > https://lists.gnu.org/mailman/listinfo/grub-devel ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [RFC] ieee1275: Avoiding many unnecessary open/close 2020-09-24 23:58 ` Eric Snowberg @ 2020-11-16 17:29 ` diegodo 2020-11-20 22:52 ` Eric Snowberg 0 siblings, 1 reply; 5+ messages in thread From: diegodo @ 2020-11-16 17:29 UTC (permalink / raw) To: Eric Snowberg; +Cc: The development of GNU GRUB On 2020-09-24 20:58, Eric Snowberg wrote: >> On Sep 24, 2020, at 11:11 AM, diegodo <diegodo@linux.vnet.ibm.com> >> wrote: >> >> 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. > > I ran into the same issue with the ieee1275 platform on SPARC when > using > ofdisk. It would take 20+ minutes to get to the GRUB menu. I found the > same > problem, open and close can take a long time and upper level grub code > would > issue hundreds of them. I ended up writing obdisk.c. Within it, each > disk is > only opened once. I tried to make it as generic as possible. You might > consider > moving from ofdisk to obdisk. If you did, ppc64le device specific code > would > need to be included in iterate_devtree. > > It also includes new features not found in ofdisk, such as hot-plug > support and > the ability to find disks that don’t have a device alias. > > >> 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 >> >> _______________________________________________ >> Grub-devel mailing list >> Grub-devel@gnu.org >> https://lists.gnu.org/mailman/listinfo/grub-devel Thanks for the help here Eric and good job on writing this new module. Well, I'm still considering this patch because: 1. We are facing some problems with powerpc and it seems the code of ofdisk.c is somewhat bogus. IMO would be nice to have this code at least working well on community. It would help us almost instantly with our problems regarding grub on powerpc. 2. Although we are considering/discussing to switch to obdisk module, this will need an extra effort and time on writing specific code, testing, etc. From our tests here with powerpc, this code is promising to solve many problems that we are currently facing. So, since we already have obdisk for SPARC, do you think it could be a problem to update this code as suggested in the patch? Thanks a lot ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [RFC] ieee1275: Avoiding many unnecessary open/close 2020-11-16 17:29 ` diegodo @ 2020-11-20 22:52 ` Eric Snowberg 0 siblings, 0 replies; 5+ messages in thread From: Eric Snowberg @ 2020-11-20 22:52 UTC (permalink / raw) To: The development of GNU GRUB; +Cc: diegodo > On Nov 16, 2020, at 10:29 AM, diegodo <diegodo@linux.vnet.ibm.com> wrote: > > On 2020-09-24 20:58, Eric Snowberg wrote: >>> On Sep 24, 2020, at 11:11 AM, diegodo <diegodo@linux.vnet.ibm.com> wrote: >>> 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. >> I ran into the same issue with the ieee1275 platform on SPARC when using >> ofdisk. It would take 20+ minutes to get to the GRUB menu. I found the same >> problem, open and close can take a long time and upper level grub code would >> issue hundreds of them. I ended up writing obdisk.c. Within it, each disk is >> only opened once. I tried to make it as generic as possible. You might consider >> moving from ofdisk to obdisk. If you did, ppc64le device specific code would >> need to be included in iterate_devtree. >> It also includes new features not found in ofdisk, such as hot-plug support and >> the ability to find disks that don’t have a device alias. >>> 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 >>> _______________________________________________ >>> Grub-devel mailing list >>> Grub-devel@gnu.org >>> https://lists.gnu.org/mailman/listinfo/grub-devel > > > Thanks for the help here Eric and good job on writing this new module. > > Well, I'm still considering this patch because: > > 1. We are facing some problems with powerpc and it seems the code of ofdisk.c is somewhat bogus. IMO would be nice to have this code at least working well on community. It would help us almost instantly with our problems regarding grub on powerpc. > 2. Although we are considering/discussing to switch to obdisk module, this will need an extra effort and time on writing specific code, testing, etc. > > From our tests here with powerpc, this code is promising to solve many problems that we are currently facing. > > So, since we already have obdisk for SPARC, do you think it could be a problem to update this code as suggested in the patch? > Years ago I tried submitting a patch very similar to yours [1]. The response I got back from the maintainers was there was concern it may impact some of the other ieee1275 platforms. I didn’t have the resources to test them all. Personally I don’t see how anyone can use ofdisk. It contains a terrible dangling pointer bug that prevented me from adding any new code [2]. 1. https://lists.gnu.org/archive/html/grub-devel/2016-06/msg00038.html 2. https://lists.gnu.org/archive/html/grub-devel/2016-06/msg00035.html ^ permalink raw reply [flat|nested] 5+ messages in thread
[parent not found: <d96a9f91e68155c2510d29c5efa944b5 () linux ! vnet ! ibm ! com>]
* Re: [RFC] ieee1275: Avoiding many unnecessary open/close [not found] <d96a9f91e68155c2510d29c5efa944b5 () linux ! vnet ! ibm ! com> @ 2020-09-25 7:20 ` Michael Neuling 0 siblings, 0 replies; 5+ messages in thread From: Michael Neuling @ 2020-09-25 7:20 UTC (permalink / raw) To: Diego Domingos, grub-devel Diego, This is awesome. I was digging into this a while back and found a similar problem but never got a patch. Testing your patch on my POWER8 phyp machine here gives at least an order of magnitude speed up in boot time. Regards, Mikey On Thu, 2020-09-24 at 17:11 +0000, diegodo wrote: > 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 [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-11-20 22:53 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-09-24 17:11 [RFC] ieee1275: Avoiding many unnecessary open/close diegodo
2020-09-24 23:58 ` Eric Snowberg
2020-11-16 17:29 ` diegodo
2020-11-20 22:52 ` Eric Snowberg
[not found] <d96a9f91e68155c2510d29c5efa944b5 () linux ! vnet ! ibm ! com>
2020-09-25 7:20 ` Michael Neuling
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox