Re: [PATCH] sparc64: boot performance improvements

[Vladimir 'φ-coder/phcoder' Serbinenko]

On 11.10.2015 18:49, Eric Snowberg wrote:
> 
>> 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.
> 
>
See http://permalink.gmane.org/gmane.comp.boot-loaders.grub.devel/10533
for holding open handle.
Do you readily have a scenario when you need multiple handles open?
Can you try following naive optimisation:
diff --git a/grub-core/disk/ieee1275/ofdisk.c
b/grub-core/disk/ieee1275/ofdisk.c
index 331769b..34237f9 100644
--- a/grub-core/disk/ieee1275/ofdisk.c
+++ b/grub-core/disk/ieee1275/ofdisk.c
@@ -448,13 +448,6 @@ grub_ofdisk_open (const char *name, grub_disk_t disk)
 static void
 grub_ofdisk_close (grub_disk_t disk)
 {
-  if (disk->data == last_devpath)
-    {
-      if (last_ihandle)
-       grub_ieee1275_close (last_ihandle);
-      last_ihandle = 0;
-      last_devpath = NULL;
-    }
   disk->data = 0;
 }


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

signature.asc
Description: OpenPGP digital signature