Re: [PATCH] sparc64: boot performance improvements

[Eric Snowberg]

> 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.

> 
>> 
>> 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