From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.90_1) id 1keiK7-0003nF-Kr for mharc-grub-devel@gnu.org; Mon, 16 Nov 2020 12:29:52 -0500 Received: from eggs.gnu.org ([2001:470:142:3::10]:50976) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1keiK4-0003mz-2j for grub-devel@gnu.org; Mon, 16 Nov 2020 12:29:50 -0500 Received: from mx0a-001b2d01.pphosted.com ([148.163.156.1]:37696) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1keiJy-00022F-9n for grub-devel@gnu.org; Mon, 16 Nov 2020 12:29:47 -0500 Received: from pps.filterd (m0098410.ppops.net [127.0.0.1]) by mx0a-001b2d01.pphosted.com (8.16.0.42/8.16.0.42) with SMTP id 0AGGYaob052721; Mon, 16 Nov 2020 12:29:38 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ibm.com; h=mime-version : date : from : to : cc : subject : in-reply-to : references : message-id : content-type : content-transfer-encoding; s=pp1; bh=V6sHaftLrXINMrADFq2uJtmJF4RzXr+CDGVnxAuRhag=; b=Cl8wyV+XdM1Rg3Qv82cDlipkmgdpf+AdfjMmhnXYr8GPQqZUz0koL6HBynYeCyL05kQl Jzqf6J3zKJD5hZDWBcrJd6Q2VJaiKEjSxk/FvY54OzQrmyxEDCSNkgsj9hd/agICSxDi hb2b1jX37QqLbDCB0oG7p2HbrHxtseC+2xluFXo1HToKMhhOsg2sEprWwB0Ku+9bSD9Z LHAuN0eEFicLUqUK8ldnzcpStwao/yYvrRjlIFcn1JSppEZT+UCAF2itrUbqDiwIKaZr TmY+5nzD6n2KZL0g3+3I8Ker/TnvTFcodfqqTf8hzY7nLhmTyf51prMU6aTfARa4L/wH dg== Received: from ppma02dal.us.ibm.com (a.bd.3ea9.ip4.static.sl-reverse.com [169.62.189.10]) by mx0a-001b2d01.pphosted.com with ESMTP id 34uvwa1pu0-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Mon, 16 Nov 2020 12:29:38 -0500 Received: from pps.filterd (ppma02dal.us.ibm.com [127.0.0.1]) by ppma02dal.us.ibm.com (8.16.0.42/8.16.0.42) with SMTP id 0AGH81JS012617; Mon, 16 Nov 2020 17:29:37 GMT Received: from b01cxnp22033.gho.pok.ibm.com (b01cxnp22033.gho.pok.ibm.com [9.57.198.23]) by ppma02dal.us.ibm.com with ESMTP id 34t6v94jsc-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Mon, 16 Nov 2020 17:29:37 +0000 Received: from b01ledav002.gho.pok.ibm.com (b01ledav002.gho.pok.ibm.com [9.57.199.107]) by b01cxnp22033.gho.pok.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id 0AGHTaqY13369666 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Mon, 16 Nov 2020 17:29:37 GMT Received: from b01ledav002.gho.pok.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id D6EC0124055; Mon, 16 Nov 2020 17:29:36 +0000 (GMT) Received: from b01ledav002.gho.pok.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id 7CE43124053; Mon, 16 Nov 2020 17:29:36 +0000 (GMT) Received: from ltc.linux.ibm.com (unknown [9.16.170.189]) by b01ledav002.gho.pok.ibm.com (Postfix) with ESMTP; Mon, 16 Nov 2020 17:29:36 +0000 (GMT) MIME-Version: 1.0 Date: Mon, 16 Nov 2020 14:29:36 -0300 From: diegodo To: Eric Snowberg Cc: The development of GNU GRUB Subject: Re: [RFC] ieee1275: Avoiding many unnecessary open/close In-Reply-To: <14A57B2B-5ED1-4B65-A10E-3C41725C3BDC@oracle.com> References: <14A57B2B-5ED1-4B65-A10E-3C41725C3BDC@oracle.com> Message-ID: X-Sender: diegodo@linux.vnet.ibm.com User-Agent: Roundcube Webmail/1.0.1 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-TM-AS-GCONF: 00 X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.312, 18.0.737 definitions=2020-11-16_08:2020-11-13, 2020-11-16 signatures=0 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 malwarescore=0 suspectscore=0 mlxlogscore=999 mlxscore=0 bulkscore=0 clxscore=1011 spamscore=0 impostorscore=0 phishscore=0 priorityscore=1501 lowpriorityscore=0 adultscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2009150000 definitions=main-2011160099 Received-SPF: none client-ip=148.163.156.1; envelope-from=diegodo@linux.vnet.ibm.com; helo=mx0a-001b2d01.pphosted.com X-detected-operating-system: by eggs.gnu.org: First seen = 2020/11/16 12:29:39 X-ACL-Warn: Detected OS = Linux 3.x [generic] [fuzzy] X-Spam_score_int: -19 X-Spam_score: -2.0 X-Spam_bar: -- X-Spam_report: (-2.0 / 5.0 requ) BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_EF=-0.1, RCVD_IN_DNSWL_NONE=-0.0001, RCVD_IN_MSPIKE_H2=-0.001, SPF_HELO_NONE=0.001, SPF_NONE=0.001 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: grub-devel@gnu.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: The development of GNU GRUB List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 16 Nov 2020 17:29:51 -0000 On 2020-09-24 20:58, Eric Snowberg wrote: >> On Sep 24, 2020, at 11:11 AM, diegodo >> wrote: >> >> Hi, >> >> we are facing some performance issues with ieee1275 platform (ppc64le >> architecture to be more specific) and I would like to suggest a couple >> of changes to try to avoid them. >> >> Sometimes the system can take more than 10 minutes to boot (in fact, >> it can takes more than 30 and 40 minutes in some cases). From my >> investigations, I noticied the following points: >> >> 1. Some types of storage devices (NVMe, Fibre Channel, ...) can have a >> long time for shutdown notification. Our investigation here showed us >> this shutdown increase with the size of the storage. For example, >> 6.4TB NVMe can takes something like 7 seconds just to issue a shutdown >> notification. >> >> 2. The grub calls a lot for OPEN/READ/CLOSE for each device. I ran >> some tests with just one disk (NVMe 6.4TB) and found that, even with >> one disk, grub call the OPEN/CLOSE functions more than 60 times. >> Considering the opening time of the device something like 1.5 seconds, >> we have almost 9 minutes just opening and closing the disk. >> >> I did some changes in the code and the time during the boot dropped >> drastically - from 492s to only 15s. >> >> The idea is to handle the _actual close_ while _opening_ the disk. So, >> during the _grub_ofdisk_open_ function, we check if the requested disk >> is the same as the already opened and close the previous one if we are >> dealing with a different disk. Yet, I removed the OPEN/CLOSE calls >> inside the _grub_ofdisk_get_block_size_ function and let it just >> checking for the block size. >> >> What are your suggestions? Is there a better way to handle this? I >> really appreciate your help here. > > I ran into the same issue with the ieee1275 platform on SPARC when > using > ofdisk. It would take 20+ minutes to get to the GRUB menu. I found the > same > problem, open and close can take a long time and upper level grub code > would > issue hundreds of them. I ended up writing obdisk.c. Within it, each > disk is > only opened once. I tried to make it as generic as possible. You might > consider > moving from ofdisk to obdisk. If you did, ppc64le device specific code > would > need to be included in iterate_devtree. > > It also includes new features not found in ofdisk, such as hot-plug > support and > the ability to find disks that don’t have a device alias. > > >> Thanks >> >> >> --- >> grub-core/disk/ieee1275/ofdisk.c | 63 +++++++++++++++++--------------- >> 1 file changed, 34 insertions(+), 29 deletions(-) >> >> diff --git a/grub-core/disk/ieee1275/ofdisk.c >> b/grub-core/disk/ieee1275/ofdisk.c >> index d887d4b..d0ee4ae 100644 >> --- a/grub-core/disk/ieee1275/ofdisk.c >> +++ b/grub-core/disk/ieee1275/ofdisk.c >> @@ -44,7 +44,7 @@ struct ofdisk_hash_ent >> }; >> >> static grub_err_t >> -grub_ofdisk_get_block_size (const char *device, grub_uint32_t >> *block_size, >> +grub_ofdisk_get_block_size (grub_uint32_t *block_size, >> struct ofdisk_hash_ent *op); >> >> #define OFDISK_HASH_SZ 8 >> @@ -461,6 +461,7 @@ grub_ofdisk_open (const char *name, grub_disk_t >> disk) >> grub_ssize_t actual; >> grub_uint32_t block_size = 0; >> grub_err_t err; >> + struct ofdisk_hash_ent *op; >> >> if (grub_strncmp (name, "ieee1275/", sizeof ("ieee1275/") - 1) != 0) >> return grub_error (GRUB_ERR_UNKNOWN_DEVICE, >> @@ -471,6 +472,34 @@ grub_ofdisk_open (const char *name, grub_disk_t >> disk) >> >> grub_dprintf ("disk", "Opening `%s'.\n", devpath); >> >> + op = ofdisk_hash_find (devpath); >> + if (!op) >> + op = ofdisk_hash_add (devpath, NULL); >> + if (!op) >> + { >> + grub_free (devpath); >> + return grub_errno; >> + } >> + >> + /* Check if the call to open is the same to the last disk already >> opened */ >> + if (last_devpath && !grub_strcmp(op->open_path,last_devpath)) >> + { >> + goto finish; >> + } >> + >> + /* If not, we need to close the previous disk and open the new one >> */ >> + else { >> + if (last_ihandle){ >> + grub_ieee1275_close (last_ihandle); >> + } >> + last_ihandle = 0; >> + last_devpath = NULL; >> + >> + grub_ieee1275_open (op->open_path, &last_ihandle); >> + if (! last_ihandle) >> + return grub_error (GRUB_ERR_UNKNOWN_DEVICE, "can't open >> device"); >> + } >> + >> if (grub_ieee1275_finddevice (devpath, &dev)) >> { >> grub_free (devpath); >> @@ -491,25 +520,18 @@ grub_ofdisk_open (const char *name, grub_disk_t >> disk) >> return grub_error (GRUB_ERR_UNKNOWN_DEVICE, "not a block >> device"); >> } >> >> + >> + finish: >> /* XXX: There is no property to read the number of blocks. There >> should be a property `#blocks', but it is not there. Perhaps it >> is possible to use seek for this. */ >> disk->total_sectors = GRUB_DISK_SIZE_UNKNOWN; >> >> { >> - struct ofdisk_hash_ent *op; >> - op = ofdisk_hash_find (devpath); >> - if (!op) >> - op = ofdisk_hash_add (devpath, NULL); >> - if (!op) >> - { >> - grub_free (devpath); >> - return grub_errno; >> - } >> disk->id = (unsigned long) op; >> disk->data = op->open_path; >> >> - err = grub_ofdisk_get_block_size (devpath, &block_size, op); >> + err = grub_ofdisk_get_block_size (&block_size, op); >> if (err) >> { >> grub_free (devpath); >> @@ -532,13 +554,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; >> } >> >> @@ -685,7 +700,7 @@ grub_ofdisk_init (void) >> } >> >> static grub_err_t >> -grub_ofdisk_get_block_size (const char *device, grub_uint32_t >> *block_size, >> +grub_ofdisk_get_block_size (grub_uint32_t *block_size, >> struct ofdisk_hash_ent *op) >> { >> struct size_args_ieee1275 >> @@ -698,16 +713,6 @@ grub_ofdisk_get_block_size (const char *device, >> grub_uint32_t *block_size, >> grub_ieee1275_cell_t size2; >> } args_ieee1275; >> >> - if (last_ihandle) >> - grub_ieee1275_close (last_ihandle); >> - >> - last_ihandle = 0; >> - last_devpath = NULL; >> - >> - grub_ieee1275_open (device, &last_ihandle); >> - if (! last_ihandle) >> - return grub_error (GRUB_ERR_UNKNOWN_DEVICE, "can't open device"); >> - >> *block_size = 0; >> >> if (op->block_size_fails >= 2) >> -- >> 2.21.3 >> >> _______________________________________________ >> Grub-devel mailing list >> Grub-devel@gnu.org >> https://lists.gnu.org/mailman/listinfo/grub-devel Thanks for the help here Eric and good job on writing this new module. Well, I'm still considering this patch because: 1. We are facing some problems with powerpc and it seems the code of ofdisk.c is somewhat bogus. IMO would be nice to have this code at least working well on community. It would help us almost instantly with our problems regarding grub on powerpc. 2. Although we are considering/discussing to switch to obdisk module, this will need an extra effort and time on writing specific code, testing, etc. From our tests here with powerpc, this code is promising to solve many problems that we are currently facing. So, since we already have obdisk for SPARC, do you think it could be a problem to update this code as suggested in the patch? Thanks a lot