Re: [PATCH] sparc64: boot performance improvements

[Vladimir 'phcoder' Serbinenko]

Le 10 oct. 2015 3:31 AM, "Eric Snowberg" <address@hidden> a écrit :
>
>
> > On Oct 9, 2015, at 12:50 AM, Andrei Borzenkov <address@hidden> wrote:
> >
> > On Thu, Oct 8, 2015 at 2:20 AM, Eric Snowberg <address@hidden> wrote:
> >>
> >>> On Oct 7, 2015, at 2:36 AM, Andrei Borzenkov <address@hidden> wrote:
> >>>
> >>> On Tue, Oct 6, 2015 at 8:39 PM, Eric Snowberg <address@hidden> 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 <address@hidden>
> >>>> ---
> >>>> 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
> >>>> address@hidden
> >>>> https://lists.gnu.org/mailman/listinfo/grub-devel
> >>>
> >>> _______________________________________________
> >>> Grub-devel mailing list
> >>> address@hidden
> >>> https://lists.gnu.org/mailman/listinfo/grub-devel
> >>
> >>
> >> _______________________________________________
> >> Grub-devel mailing list
> >> address@hidden
> >> https://lists.gnu.org/mailman/listinfo/grub-devel
> >
> > _______________________________________________
> > Grub-devel mailing list
> > address@hidden
> > https://lists.gnu.org/mailman/listinfo/grub-devel
>
>
> _______________________________________________
> Grub-devel mailing list
> address@hidden
> https://lists.gnu.org/mailman/listinfo/grub-devel