Re: [PATCH] sparc64: boot performance improvements

[Eric Snowberg]

> On Oct 10, 2015, at 1:35 AM, Vladimir 'phcoder' Serbinenko <address@hidden> 
> wrote:
> 
> 
> 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

I can get access to every sun4v platform.  Could you provide any additional 
information on which sun4v systems had this failure.  If so, I’ll try to submit 
a fix for that problem as well.  It seems like there could be a better way to 
handle this than resetting the SAS or SATA HBA before each read operation.

There are many problems with the upstream grub2 implementation for sun4v.  I 
will be submitting additional follow on patches, since it currently will not 
even boot to the grub menu.  My intention is to get grub2 working on every 
sun4v platform.


> > >
> > >>
> > >> 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
> _______________________________________________
> Grub-devel mailing list
> address@hidden
> https://lists.gnu.org/mailman/listinfo/grub-devel