> On Nov 16, 2020, at 10:29 AM, diegodo <[email protected]> wrote: > > On 2020-09-24 20:58, Eric Snowberg wrote: >>> On Sep 24, 2020, at 11:11 AM, diegodo <[email protected]> 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 >>> [email protected] >>> 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 _______________________________________________ Grub-devel mailing list [email protected] https://lists.gnu.org/mailman/listinfo/grub-devel
