On Thu, Oct 8, 2015 at 2:20 AM, Eric Snowberg <eric.snowb...@oracle.com> wrote: > >> On Oct 7, 2015, at 2:36 AM, Andrei Borzenkov <arvidj...@gmail.com> wrote: >> >> On Tue, Oct 6, 2015 at 8:39 PM, Eric Snowberg <eric.snowb...@oracle.com> >> wrote: >>> Keep of devices open. This can save 6 - 7 seconds per open call and >>> can decrease boot times from over 10 minutes to 29 seconds on >>> larger SPARC systems. The open/close calls with some vendors' >>> SAS controllers cause the entire card to be reinitialized after >>> each close. >>> >> >> Is it necessary to close these handles before launching kernel? > > On SPARC hardware it is not necessary. I would be interested to hear if it > is necessary on other IEEE1275 platforms. > >> It sounds like it can accumulate quite a lot of them - are there any >> memory limits/restrictions? Also your patch is rather generic and so >> applies to any IEEE1275 platform, I think some of them may have less >> resources. Just trying to understand what run-time cost is. > > The most I have seen are two entries in the linked list. So the additional > memory requirements are very small. I have tried this on a T2000, T4, and T5.
I thought you were speaking about *larger* SPARC servers :) Anyway I'd still prefer if this would be explicitly restricted to Oracle servers. Please look at grub-core/kern/ieee1275/cmain.c:grub_ieee1275_find_options() if new quirk can be added to detect your platform(s). > > The path name sent into the grub_ieee1275_open function is not consistent > throughout the code, even though it is referencing the same device. > Originally I tried having a global containing the path sent into > grub_ieee1275_open, but this failed due to the various ways of referencing > the same device name. That is why I added the linked list just to be safe. > Caching the grub_ieee1275_ihandle_t this way saves a significant amount of > time, since OF calls are expensive. We were seeing the same device opened > 50+ times in the process of displaying the grub menu and then launching the > kernel. > >> >>> Signed-off-by: Eric Snowberg <eric.snowb...@oracle.com> >>> --- >>> grub-core/kern/ieee1275/ieee1275.c | 39 >>> ++++++++++++++++++++++++++++++++++++ >>> 1 files changed, 39 insertions(+), 0 deletions(-) >>> >>> diff --git a/grub-core/kern/ieee1275/ieee1275.c >>> b/grub-core/kern/ieee1275/ieee1275.c >>> index 9821702..30f973b 100644 >>> --- a/grub-core/kern/ieee1275/ieee1275.c >>> +++ b/grub-core/kern/ieee1275/ieee1275.c >>> @@ -19,11 +19,24 @@ >>> >>> #include <grub/ieee1275/ieee1275.h> >>> #include <grub/types.h> >>> +#include <grub/misc.h> >>> +#include <grub/list.h> >>> +#include <grub/mm.h> >>> >>> #define IEEE1275_PHANDLE_INVALID ((grub_ieee1275_cell_t) -1) >>> #define IEEE1275_IHANDLE_INVALID ((grub_ieee1275_cell_t) 0) >>> #define IEEE1275_CELL_INVALID ((grub_ieee1275_cell_t) -1) >>> >>> +struct grub_of_opened_device >>> +{ >>> + struct grub_of_opened_device *next; >>> + struct grub_of_opened_device **prev; >>> + grub_ieee1275_ihandle_t ihandle; >>> + char *path; >>> +}; >>> + >>> +static struct grub_of_opened_device *grub_of_opened_devices; >>> + >>> >>> >>> int >>> @@ -452,6 +465,18 @@ grub_ieee1275_open (const char *path, >>> grub_ieee1275_ihandle_t *result) >>> } >>> args; >>> >>> + struct grub_of_opened_device *dev; >>> + >>> + FOR_LIST_ELEMENTS(dev, grub_of_opened_devices) >>> + if (grub_strcmp(dev->path, path) == 0) >>> + break; >>> + >>> + if (dev) >>> + { >>> + *result = dev->ihandle; >>> + return 0; >>> + } >>> + >>> INIT_IEEE1275_COMMON (&args.common, "open", 1, 1); >>> args.path = (grub_ieee1275_cell_t) path; >>> >>> @@ -460,6 +485,11 @@ grub_ieee1275_open (const char *path, >>> grub_ieee1275_ihandle_t *result) >>> *result = args.result; >>> if (args.result == IEEE1275_IHANDLE_INVALID) >>> return -1; >>> + >>> + dev = grub_zalloc(sizeof(struct grub_of_opened_device)); >> >> Error check > > I’ll resubmit V2 with this change > > > >>> + dev->path = grub_strdup(path); >> >> Ditto >> > > I’ll resubmit V2 with this change > > >>> + dev->ihandle = args.result; >>> + grub_list_push(GRUB_AS_LIST_P (&grub_of_opened_devices), GRUB_AS_LIST >>> (dev)); >>> return 0; >>> } >>> >>> @@ -473,6 +503,15 @@ grub_ieee1275_close (grub_ieee1275_ihandle_t ihandle) >>> } >>> args; >>> >>> + struct grub_of_opened_device *dev; >>> + >>> + FOR_LIST_ELEMENTS(dev, grub_of_opened_devices) >>> + if (dev->ihandle == ihandle) >>> + break; >>> + >>> + if (dev) >>> + return 0; >>> + >> >> How can we come here (i.e. can we get open handle without >> grub_ieee1275_open)? >> > > I don’t see it being possible with SPARC and would assume other platforms > could not get there either. I’m not familiar with the other IEEE1275 > platforms to know if this would be appropriate, but If there isn’t a platform > that needs this close function, could the function be changed to do nothing > but return 0? > > > >>> INIT_IEEE1275_COMMON (&args.common, "close", 1, 0); >>> args.ihandle = ihandle; >>> >>> -- >>> 1.7.1 >>> >>> >>> _______________________________________________ >>> Grub-devel mailing list >>> Grub-devel@gnu.org >>> https://lists.gnu.org/mailman/listinfo/grub-devel >> >> _______________________________________________ >> Grub-devel mailing list >> Grub-devel@gnu.org >> https://lists.gnu.org/mailman/listinfo/grub-devel > > > _______________________________________________ > Grub-devel mailing list > Grub-devel@gnu.org > https://lists.gnu.org/mailman/listinfo/grub-devel _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel