From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.71) id 1aYdQx-0005Js-2H for mharc-grub-devel@gnu.org; Wed, 24 Feb 2016 12:41:07 -0500 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37678) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aYdQt-0005DR-2E for grub-devel@gnu.org; Wed, 24 Feb 2016 12:41:04 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aYdQo-00043g-Ue for grub-devel@gnu.org; Wed, 24 Feb 2016 12:41:02 -0500 Received: from mail-lb0-x22e.google.com ([2a00:1450:4010:c04::22e]:35627) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aYdQo-00042u-HL for grub-devel@gnu.org; Wed, 24 Feb 2016 12:40:58 -0500 Received: by mail-lb0-x22e.google.com with SMTP id bc4so14691516lbc.2 for ; Wed, 24 Feb 2016 09:40:58 -0800 (PST) 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=FtcPMxvefGKvyegs/IvdapXJd4O5lePL8MCWelI21GE=; b=hLRfkNBQ7tIbc328DaleGWPrsKx9fwLbhCTLQXJ+5LPxfCbwjnw8917rCQpGWYCFDv NfaXKkyOyOZNfNfqEKH+GCCHNCXgRYbadIw1IU6YSn1126dRV6cyoJfjhlKoiO/ya8Rl +MrlLBgXp9rfdy2/Y8Zc2qh3LrFl/wEUIzr4FiEdjJ65OHzk+qK05FQmdKOgHpMJf5BA UOQWV5JiAvci+dhzgS0lku7tHqvALVMG4ijfvo6h45A/WEDye5g70BOg2102wAve64T3 Mh3s55leRgdV6MNygi7cPOBD2uusP7WgzQ/yhbU2RZRnUs9XJS1lxVKcjSRGdysidLra yURw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:subject:to:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-type; bh=FtcPMxvefGKvyegs/IvdapXJd4O5lePL8MCWelI21GE=; b=Zmy2hdgP83Iuq0A/ahOviZpDT81VUDc7WDxIkSG7mfYKv1EoUSj2R+XCTK9HXgbbCB 8/q90MzbbVZirtULwpEMu+9ECIbPcDppDoDziZuTCxuJR4WJoP09oQadf77VDqul5nGH BxCZIWJKyTs+ArstFl5Umz2o4v5SS5iBPHS+6Wc7/EcSU4ydwRgSTlR490YKraDsQUpE SSCks+P19B0JF57nL3QLk+acO8cJ0RG5GPhyP1wEotUEbyLT+bLprZZqO3xQFpgNaTiK MaKDiaW2yLLIIWvHUNLm1gb1tSpfmolr0AGQ0YfQhzkLrv7hcnP9BobNlCi+1oUUI7ft ctSA== X-Gm-Message-State: AG10YOTXnPUCS+k1czrXHa4a4JrUG7ZL3fEwnfyF1MTtu/96nPmaET2KdvP5+B6d6EtvTw== X-Received: by 10.112.17.41 with SMTP id l9mr6645695lbd.95.1456335657716; Wed, 24 Feb 2016 09:40:57 -0800 (PST) Received: from [192.168.1.41] (ppp109-252-76-159.pppoe.spdop.ru. [109.252.76.159]) by smtp.gmail.com with ESMTPSA id rh2sm517981lbb.36.2016.02.24.09.40.56 for (version=TLSv1/SSLv3 cipher=OTHER); Wed, 24 Feb 2016 09:40:56 -0800 (PST) Subject: Re: [PATCH 1/2] disk: Add support for device-specific malloc function To: grub-devel@gnu.org References: <1455898714-25127-1-git-send-email-leif.lindholm@linaro.org> <1455898714-25127-2-git-send-email-leif.lindholm@linaro.org> <56CABF64.7020003@gmail.com> <20160222140255.GO1159@bivouac.eciton.net> <56CB3DB9.4050102@gmail.com> <20160224115921.GU1159@bivouac.eciton.net> <20160224135738.GV1159@bivouac.eciton.net> From: Andrei Borzenkov Message-ID: <56CDEB27.6060606@gmail.com> Date: Wed, 24 Feb 2016 20:40:55 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.5.1 MIME-Version: 1.0 In-Reply-To: <20160224135738.GV1159@bivouac.eciton.net> Content-Type: multipart/mixed; boundary="------------070306040201040408010005" X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] X-Received-From: 2a00:1450:4010:c04::22e 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: Wed, 24 Feb 2016 17:41:05 -0000 This is a multi-part message in MIME format. --------------070306040201040408010005 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 24.02.2016 16:57, Leif Lindholm пишет: > On Wed, Feb 24, 2016 at 03:09:13PM +0300, Andrei Borzenkov wrote: >>>> Could you test attached patch with your alignment fixes on top. This >>>> implements my idea of using shared buffers. Seems to work in naive testing. >>> >>> Testing this with a grub-mkstandalone image, I get: >>> >>> kern/dl.c:556: flushing 0x10f1 bytes at 0x9ffb5ac20 >>> kern/dl.c:649: module name: tar >>> kern/dl.c:650: init function: 0x9ffb5b220 >>> kern/disk.c:217: Opening `memdisk'... >>> kern/fs.c:56: Detecting tarfs... >>> >>> And then spectacular crash in UEFI due to an EL2 translation fault. >> >> To be sure - is it with my patch alone or with your patches? If some >> more patches are used - could you send exact diff to trunk to avoid >> misunderstanding? > > Double checked with only your patch on top of > 1b782e902e69516f82158203674d4951a40c82d4 (previously running with > _only_ my alignment fixup in efidisk.c). Same behaviour. I cannot reproduce it on x86_64 (also with mm-debug enabled) and I do not know how to load standalone image on ppc; is it possible to use QEMU to run ARM64 (I assume you have it)? If not what are options to test it? Anyway, there was one problem I fixed later (although I did not get any issue before as well), I attach updated version. Thank you for testing! --------------070306040201040408010005 Content-Type: text/x-patch; name="disk-direct-cache-read-v2.patch" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="disk-direct-cache-read-v2.patch" From: Andrei Borzenkov Subject: [PATCH] disk: read into cache directly --- grub-core/kern/disk.c | 95 ++++++++++++++++++++++++++++++--------------------- grub-core/lib/disk.c | 6 ++-- include/grub/disk.h | 11 +++++- 3 files changed, 70 insertions(+), 42 deletions(-) diff --git a/grub-core/kern/disk.c b/grub-core/kern/disk.c index 789f8c0..04ab2ab 100644 --- a/grub-core/kern/disk.c +++ b/grub-core/kern/disk.c @@ -56,6 +56,20 @@ grub_err_t (*grub_disk_write_weak) (grub_disk_t disk, #include "disk_common.c" void +grub_disk_cache_free (struct grub_cache_buffer *buf) +{ + if (!buf->count) + /* FIXME This means corruption; what can we do? */ + return; + + if (!--buf->count) + { + grub_free (buf->data); + grub_free (buf); + } +} + +void grub_disk_cache_invalidate_all (void) { unsigned i; @@ -64,10 +78,10 @@ grub_disk_cache_invalidate_all (void) { struct grub_disk_cache *cache = grub_disk_cache_table + i; - if (cache->data && ! cache->lock) + if (cache->buffer && ! cache->lock) { - grub_free (cache->data); - cache->data = 0; + grub_disk_cache_free (cache->buffer); + cache->buffer = 0; } } } @@ -82,14 +96,14 @@ grub_disk_cache_fetch (unsigned long dev_id, unsigned long disk_id, cache_index = grub_disk_cache_get_index (dev_id, disk_id, sector); cache = grub_disk_cache_table + cache_index; - if (cache->dev_id == dev_id && cache->disk_id == disk_id + if (cache->buffer && cache->dev_id == dev_id && cache->disk_id == disk_id && cache->sector == sector) { cache->lock = 1; #if DISK_CACHE_STATS grub_disk_cache_hits++; #endif - return cache->data; + return cache->buffer->data + cache->offset; } #if DISK_CACHE_STATS @@ -116,28 +130,36 @@ grub_disk_cache_unlock (unsigned long dev_id, unsigned long disk_id, static grub_err_t grub_disk_cache_store (unsigned long dev_id, unsigned long disk_id, - grub_disk_addr_t sector, const char *data) + grub_disk_addr_t sector, grub_disk_addr_t n, char *data) { - unsigned cache_index; - struct grub_disk_cache *cache; + struct grub_cache_buffer *buf; + grub_addr_t offset; - cache_index = grub_disk_cache_get_index (dev_id, disk_id, sector); - cache = grub_disk_cache_table + cache_index; + buf = grub_malloc (sizeof (*buf)); + if (! buf) + return grub_errno; + buf->data = data; + buf->count = 0; - cache->lock = 1; - grub_free (cache->data); - cache->data = 0; - cache->lock = 0; + for (offset = 0 ; n > 0; sector += GRUB_DISK_CACHE_SIZE, offset += (GRUB_DISK_CACHE_SIZE * GRUB_DISK_SECTOR_SIZE), n--) + { + unsigned cache_index; + struct grub_disk_cache *cache; - cache->data = grub_malloc (GRUB_DISK_SECTOR_SIZE << GRUB_DISK_CACHE_BITS); - if (! cache->data) - return grub_errno; + cache_index = grub_disk_cache_get_index (dev_id, disk_id, sector); + cache = grub_disk_cache_table + cache_index; - grub_memcpy (cache->data, data, - GRUB_DISK_SECTOR_SIZE << GRUB_DISK_CACHE_BITS); - cache->dev_id = dev_id; - cache->disk_id = disk_id; - cache->sector = sector; + cache->lock = 1; + if (cache->buffer) + grub_disk_cache_free (cache->buffer); + cache->buffer = buf; + cache->offset = offset; + buf->count++; + cache->lock = 0; + cache->dev_id = dev_id; + cache->disk_id = disk_id; + cache->sector = sector; + } return GRUB_ERR_NONE; } @@ -350,13 +372,11 @@ grub_disk_read_small_real (grub_disk_t disk, grub_disk_addr_t sector, /* Copy it and store it in the disk cache. */ grub_memcpy (buf, tmp_buf + offset, size); grub_disk_cache_store (disk->dev->id, disk->id, - sector, tmp_buf); - grub_free (tmp_buf); + sector, 1, tmp_buf); return GRUB_ERR_NONE; } } - grub_free (tmp_buf); grub_errno = GRUB_ERR_NONE; { @@ -373,10 +393,6 @@ grub_disk_read_small_real (grub_disk_t disk, grub_disk_addr_t sector, num = ((size + offset + (1ULL << (disk->log_sector_size)) - 1) >> (disk->log_sector_size)); - tmp_buf = grub_malloc (num << disk->log_sector_size); - if (!tmp_buf) - return grub_errno; - if ((disk->dev->read) (disk, transform_sector (disk, aligned_sector), num, tmp_buf)) { @@ -481,22 +497,25 @@ grub_disk_read (grub_disk_t disk, grub_disk_addr_t sector, if (agglomerate) { - grub_disk_addr_t i; + void *cache = grub_malloc (agglomerate << (GRUB_DISK_CACHE_BITS + GRUB_DISK_SECTOR_BITS)); + if (!cache) + return grub_errno; err = (disk->dev->read) (disk, transform_sector (disk, sector), agglomerate << (GRUB_DISK_CACHE_BITS + GRUB_DISK_SECTOR_BITS - disk->log_sector_size), - buf); + cache); if (err) - return err; + { + grub_free (cache); + return err; + } - for (i = 0; i < agglomerate; i ++) - grub_disk_cache_store (disk->dev->id, disk->id, - sector + (i << GRUB_DISK_CACHE_BITS), - (char *) buf - + (i << (GRUB_DISK_CACHE_BITS - + GRUB_DISK_SECTOR_BITS))); + grub_memcpy (buf, cache, + agglomerate << (GRUB_DISK_CACHE_BITS + GRUB_DISK_SECTOR_BITS)); + grub_disk_cache_store (disk->dev->id, disk->id, + sector, agglomerate, cache); if (disk->read_hook) diff --git a/grub-core/lib/disk.c b/grub-core/lib/disk.c index 0f18688..07fb117 100644 --- a/grub-core/lib/disk.c +++ b/grub-core/lib/disk.c @@ -42,11 +42,11 @@ grub_disk_cache_invalidate (unsigned long dev_id, unsigned long disk_id, cache = grub_disk_cache_table + cache_index; if (cache->dev_id == dev_id && cache->disk_id == disk_id - && cache->sector == sector && cache->data) + && cache->sector == sector && cache->buffer) { cache->lock = 1; - grub_free (cache->data); - cache->data = 0; + grub_disk_cache_free (cache->buffer); + cache->buffer = 0; cache->lock = 0; } } diff --git a/include/grub/disk.h b/include/grub/disk.h index b385af8..9e60367 100644 --- a/include/grub/disk.h +++ b/include/grub/disk.h @@ -233,16 +233,25 @@ grub_stop_disk_firmware (void) } } +struct grub_cache_buffer + { + char *data; + unsigned count; + }; + /* Disk cache. */ struct grub_disk_cache { enum grub_disk_dev_id dev_id; unsigned long disk_id; grub_disk_addr_t sector; - char *data; + struct grub_cache_buffer *buffer; + grub_addr_t offset; int lock; }; +void EXPORT_FUNC(grub_disk_cache_free) (struct grub_cache_buffer *buf); + extern struct grub_disk_cache EXPORT_VAR(grub_disk_cache_table)[GRUB_DISK_CACHE_NUM]; #if defined (GRUB_UTIL) -- tg: (bc22096..) e/disk-cache-align (depends on: master) --------------070306040201040408010005--