From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.71) id 1ZqN6z-0007at-HQ for mharc-grub-devel@gnu.org; Sun, 25 Oct 2015 11:21:33 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:60253) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZqN6w-0007aK-No for grub-devel@gnu.org; Sun, 25 Oct 2015 11:21:32 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZqN6u-0006KH-ML for grub-devel@gnu.org; Sun, 25 Oct 2015 11:21:30 -0400 Received: from mail-wi0-x234.google.com ([2a00:1450:400c:c05::234]:36358) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZqN6u-0006KD-DF for grub-devel@gnu.org; Sun, 25 Oct 2015 11:21:28 -0400 Received: by wicfx6 with SMTP id fx6so83952482wic.1 for ; Sun, 25 Oct 2015 08:21:27 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=subject:to:references:from:message-id:date:user-agent:mime-version :in-reply-to:content-type; bh=kIAhhU1uwOoDsNu3jdhU23tlfB9ZtKW6g+lHLKexoHk=; b=ca7ud5NlRsmM1AqlK+VFLUyl2G9untXTKqL+Auf/m/Yat6rjZivw6NqG2Ocn6QMddp CnF30deNX2JROFByswo/F+Gc4FPZz/o4cgJDNRTNfSsvWuVO92MfcrCvv6mIRvsS8Zkj BSHUn90POBaXNKbrhZIa3O/4iqiXmowACldc87pGk0JHPvQRfkyPupVu9YffHf6OGzKw xF0oQWMmluFSnkQO17WT0BasHJNR+n5aKp0NM8aKqIMobmZEoGJSIR8jYM35kBYmFI1n 158059oXD/T39BlKI5iuSB4ZZ5vmljNOAvx/rqwe2poXBAZN9laSBS8Z1mIlqckSDAZU f2Lw== X-Received: by 10.180.218.199 with SMTP id pi7mr16079860wic.75.1445786487477; Sun, 25 Oct 2015 08:21:27 -0700 (PDT) Received: from ?IPv6:2a02:120b:2c08:1c10:863a:4bff:fe50:abc4? ([2a02:120b:2c08:1c10:863a:4bff:fe50:abc4]) by smtp.gmail.com with ESMTPSA id h7sm33998520wjz.7.2015.10.25.08.21.26 for (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Sun, 25 Oct 2015 08:21:26 -0700 (PDT) Subject: Re: [PATCH] sparc64: boot performance improvements To: The development of GNU GRUB References: <1444153184-252565-1-git-send-email-eric.snowberg@oracle.com> <3853568C-8AB7-46F3-A022-26EAD4438CF2@oracle.com> From: =?UTF-8?Q?Vladimir_'=cf=86-coder/phcoder'_Serbinenko?= Message-ID: <562CF356.5000603@gmail.com> Date: Sun, 25 Oct 2015 16:20:54 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Icedove/38.2.0 MIME-Version: 1.0 In-Reply-To: Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="jGAFxbCseFLKLQvdANXi6R1nbVkU0D0M5" X-detected-operating-system: by eggs.gnu.org: Error: Malformed IPv6 address (bad octet value). X-Received-From: 2a00:1450:400c:c05::234 X-BeenThere: grub-devel@gnu.org X-Mailman-Version: 2.1.14 Precedence: list Reply-To: The development of GNU GRUB List-Id: The development of GNU GRUB List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 25 Oct 2015 15:21:32 -0000 This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --jGAFxbCseFLKLQvdANXi6R1nbVkU0D0M5 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 11.10.2015 18:49, Eric Snowberg wrote: >=20 >> On Oct 10, 2015, at 1:35 AM, Vladimir 'phcoder' Serbinenko wrote: >> >> >> Le 10 oct. 2015 3:31 AM, "Eric Snowberg" a = =C3=A9crit : >>> >>> >>>> On Oct 9, 2015, at 12:50 AM, Andrei Borzenkov = wrote: >>>> >>>> On Thu, Oct 8, 2015 at 2:20 AM, Eric Snowberg wrote: >>>>> >>>>>> On Oct 7, 2015, at 2:36 AM, Andrei Borzenkov = wrote: >>>>>> >>>>>> On Tue, Oct 6, 2015 at 8:39 PM, Eric Snowberg 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 he= ar if it is necessary on other IEEE1275 platforms. >>>>> >>>>>> It sounds like it can accumulate quite a lot of them - are there a= ny >>>>>> memory limits/restrictions? Also your patch is rather generic and = so >>>>>> applies to any IEEE1275 platform, I think some of them may have le= ss >>>>>> resources. Just trying to understand what run-time cost is. >>>>> >>>>> The most I have seen are two entries in the linked list. So the ad= ditional memory requirements are very small. I have tried this on a T200= 0, 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=E2=80=99ll restrict it to all sun4v equipment. >>> >> Not good enough. Some of sun4v probably have the bug I told about in t= he other e-mail >=20 > I can get access to every sun4v platform. Could you provide any additi= onal information on which sun4v systems had this failure. If so, I=E2=80= =99ll 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 b= efore each read operation. >=20 > 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 =3D=3D last_devpath) - { - if (last_ihandle) - grub_ieee1275_close (last_ihandle); - last_ihandle =3D 0; - last_devpath =3D NULL; - } disk->data =3D 0; } > There are many problems with the upstream grub2 implementation for sun4= v. 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 worki= ng on every sun4v platform. >=20 Could you start with >=20 >>>> >>>>> >>>>> The path name sent into the grub_ieee1275_open function is not cons= istent throughout the code, even though it is referencing the same device= =2E Originally I tried having a global containing the path sent into gru= b_ieee1275_open, but this failed due to the various ways of referencing t= he same device name. That is why I added the linked list just to be safe= =2E Caching the grub_ieee1275_ihandle_t this way saves a significant amo= unt of time, since OF calls are expensive. We were seeing the same devic= e opened 50+ times in the process of displaying the grub menu and then la= unching the kernel. >>>>> >>>>>> >>>>>>> Signed-off-by: Eric Snowberg >>>>>>> --- >>>>>>> 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 >>>>>>> #include >>>>>>> +#include >>>>>>> +#include >>>>>>> +#include >>>>>>> >>>>>>> #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_i= eee1275_ihandle_t *result) >>>>>>> } >>>>>>> args; >>>>>>> >>>>>>> + struct grub_of_opened_device *dev; >>>>>>> + >>>>>>> + FOR_LIST_ELEMENTS(dev, grub_of_opened_devices) >>>>>>> + if (grub_strcmp(dev->path, path) =3D=3D 0) >>>>>>> + break; >>>>>>> + >>>>>>> + if (dev) >>>>>>> + { >>>>>>> + *result =3D dev->ihandle; >>>>>>> + return 0; >>>>>>> + } >>>>>>> + >>>>>>> INIT_IEEE1275_COMMON (&args.common, "open", 1, 1); >>>>>>> args.path =3D (grub_ieee1275_cell_t) path; >>>>>>> >>>>>>> @@ -460,6 +485,11 @@ grub_ieee1275_open (const char *path, grub_i= eee1275_ihandle_t *result) >>>>>>> *result =3D args.result; >>>>>>> if (args.result =3D=3D IEEE1275_IHANDLE_INVALID) >>>>>>> return -1; >>>>>>> + >>>>>>> + dev =3D grub_zalloc(sizeof(struct grub_of_opened_device)); >>>>>> >>>>>> Error check >>>>> >>>>> I=E2=80=99ll resubmit V2 with this change >>>>> >>>>> >>>>> >>>>>>> + dev->path =3D grub_strdup(path); >>>>>> >>>>>> Ditto >>>>>> >>>>> >>>>> I=E2=80=99ll resubmit V2 with this change >>>>> >>>>> >>>>>>> + dev->ihandle =3D 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 =3D=3D ihandle) >>>>>>> + break; >>>>>>> + >>>>>>> + if (dev) >>>>>>> + return 0; >>>>>>> + >>>>>> >>>>>> How can we come here (i.e. can we get open handle without grub_iee= e1275_open)? >>>>>> >>>>> >>>>> I don=E2=80=99t see it being possible with SPARC and would assume o= ther platforms could not get there either. I=E2=80=99m not familiar with = the other IEEE1275 platforms to know if this would be appropriate, but If= there isn=E2=80=99t 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 =3D 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 >> _______________________________________________ >> Grub-devel mailing list >> Grub-devel@gnu.org >> https://lists.gnu.org/mailman/listinfo/grub-devel >=20 >=20 > _______________________________________________ > Grub-devel mailing list > Grub-devel@gnu.org > https://lists.gnu.org/mailman/listinfo/grub-devel >=20 --jGAFxbCseFLKLQvdANXi6R1nbVkU0D0M5 Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iF4EAREKAAYFAlYs83UACgkQmBXlbbo5nOvvlAD7B0m39WjH9AGZvmdTx3MK2qEq BbWTm6nWTlCLwAx9UDsBAIl69Q9kZFtuofAhAX83KnyjcjluY/VIWlWDvbf7KD8b =7jb+ -----END PGP SIGNATURE----- --jGAFxbCseFLKLQvdANXi6R1nbVkU0D0M5--