Re: [PATCH] sparc64: boot performance improvements

[Eric Snowberg]

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

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