Le 10 oct. 2015 3:31 AM, "Eric Snowberg" <eric.snowb...@oracle.com> a
écrit :
>
>
> > On Oct 9, 2015, at 12:50 AM, Andrei Borzenkov <arvidj...@gmail.com>
wrote:
> >
> > 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).
> >
>
> That makes sense, I’ll restrict it to all sun4v equipment.
>
Not good enough. Some of sun4v probably have the bug I told about in the
other e-mail
> >
> >>
> >> 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
>
>
> _______________________________________________
> 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
