> On Oct 9, 2015, at 12:50 AM, Andrei Borzenkov <arvidj...@gmail.com> wrote:
>
> On Thu, Oct 8, 2015 at 2:20 AM, Eric Snowberg <eric.snowb...@oracle.com>
> wrote:
>>
>>> On Oct 7, 2015, at 2:36 AM, Andrei Borzenkov <arvidj...@gmail.com> wrote:
>>>
>>> On Tue, Oct 6, 2015 at 8:39 PM, Eric Snowberg <eric.snowb...@oracle.com>
>>> 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 <eric.snowb...@oracle.com>
>>>> ---
>>>> 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
>>>> Grub-devel@gnu.org
>>>> https://lists.gnu.org/mailman/listinfo/grub-devel
>>>
>>> _______________________________________________
>>> Grub-devel mailing list
>>> Grub-devel@gnu.org
>>> https://lists.gnu.org/mailman/listinfo/grub-devel
>>
>>
>> _______________________________________________
>> Grub-devel mailing list
>> Grub-devel@gnu.org
>> https://lists.gnu.org/mailman/listinfo/grub-devel
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
