Re: [PATCH] sparc64: boot performance improvements

[Andrei Borzenkov]

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


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