From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.71) id 1aXtmm-0002m6-03 for mharc-grub-devel@gnu.org; Mon, 22 Feb 2016 11:56:36 -0500 Received: from eggs.gnu.org ([2001:4830:134:3::10]:39442) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aXtmj-0002fu-2X for grub-devel@gnu.org; Mon, 22 Feb 2016 11:56:34 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aXtmf-0003Hn-00 for grub-devel@gnu.org; Mon, 22 Feb 2016 11:56:33 -0500 Received: from mail-lb0-x236.google.com ([2a00:1450:4010:c04::236]:36162) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aXtme-0003Hb-Fk for grub-devel@gnu.org; Mon, 22 Feb 2016 11:56:28 -0500 Received: by mail-lb0-x236.google.com with SMTP id x1so85556588lbj.3 for ; Mon, 22 Feb 2016 08:56:28 -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=OUprolOJzd6aS89C1FGbWmFRPbhQqEpnTMlS6EsbXjY=; b=KT+noZ3E+Qx3w+87epOkCjEUZ5WYaIOI+MvB1BgugS6nqaiqMcH3/xZ6wTK4xhQoFP mlxwHcq7vbKVRgAwAa8wNpBQK8Thyw+55hRydiNG1s6TRlNMtK/mWoLU2NkO10XS2Pvw 90SuKQXHbuVqsbtYwvVU9HHXKUJz38qWtvye/0vU5oF0xcl/4rEMlg59H7OHHLkSs+Uf ppn/FQisOWuUlnbdeLtWAz97FcNb/vHa0mXaAtlsCy9DK1rsJ3CsuEjIlUWYLn75d2bJ 1KLf+1pLnW7xUdxtN/493WRFxLkMkIaiYGGMFXpJ8FPCZDHZ2PoiOr2uN19ING6ep+4R inGg== 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=OUprolOJzd6aS89C1FGbWmFRPbhQqEpnTMlS6EsbXjY=; b=cm5c0ooBhgf7Df1dAWKidMR+PUukmTXjRVcesU/d3Qjx10GbvpZ07aA0OJ8i2rIFHr t3xvau4FHGxh25Ekdi2+eOdFX+i+cuSad4z2y71AS3k1ObveRcAJ2LGFU+dWbra1yCwm U+PKe50+RYbChtHEsZpTu7Q8EB6t627CQBLTDYtb97Yqw4+pToPb0ipKF4UL1Jquh2N5 qatokXPyWgmkwy3YUHP+Yv+JsL39h0nUYehAHNsFNBdEwdQn1wjUGQ9hIJBaLtaPBjsP Z2Eo3kcP7eXAP9N2R6xZVm1BzDS/hN7WbW0VVeXrv2SWg2MkLTSK+dfrbFwF5FXX7kzD cfEQ== X-Gm-Message-State: AG10YOSV+huNP4gjr7JnKI2l3McvOXaOPOVsyIojtHtBI9OneSB9vTH2juSgdgTK4ws8bA== X-Received: by 10.112.137.129 with SMTP id qi1mr10467085lbb.31.1456160187613; Mon, 22 Feb 2016 08:56:27 -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 un6sm1147977lbb.18.2016.02.22.08.56.26 for (version=TLSv1/SSLv3 cipher=OTHER); Mon, 22 Feb 2016 08:56:26 -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> From: Andrei Borzenkov X-Enigmail-Draft-Status: N1110 Message-ID: <56CB3DB9.4050102@gmail.com> Date: Mon, 22 Feb 2016 19:56:25 +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: <20160222140255.GO1159@bivouac.eciton.net> Content-Type: multipart/mixed; boundary="------------000005050200020003020002" X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] X-Received-From: 2a00:1450:4010:c04::236 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: Mon, 22 Feb 2016 16:56:34 -0000 This is a multi-part message in MIME format. --------------000005050200020003020002 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit 22.02.2016 17:02, Leif Lindholm пишет: > On Mon, Feb 22, 2016 at 10:57:24AM +0300, Andrei Borzenkov wrote: >> 19.02.2016 19:18, Leif Lindholm пишет: >>> Some disk types have allocation requirements beyond normal grub_malloc. >>> Add a function pointer to grub_disk_t and a wrapper function in >>> kern/disk.c making use of that function if available, to enable these >>> disk drivers to implement their own malloc. >> >> The problem is not (only) grub_disk_read_small(), but this part in >> grub_disk_read: >> >> if (agglomerate) >> { >> grub_disk_addr_t i; >> >> err = (disk->dev->read) (disk, transform_sector (disk, sector), >> agglomerate << (GRUB_DISK_CACHE_BITS >> + GRUB_DISK_SECTOR_BITS >> - disk->log_sector_size), >> buf); >> >> which reads directly into user supplied buffer. May be we can allocate >> contiguous cache block here but put pointers to multiple chunks inside >> it. Possible implementation is to have second layer of reference counted >> memory blocks with cache entries containing pointer + offset into them. > > Whoops! > > Understood. > > So how about merging the two concepts? > Including a patch to go with (after) the previous two to catch any > remaining unaligned accesses in grub_efidisk_readwrite(). > With this applied, I get no fixups from a normal Linux boot (linux + > initrd), but see them when exploring filesystems from the command > line. > > Whilst a bit clunky, this seems much short-term preferable to going > back and redesigning the disk subsystem to understand that alignment > matters. Although given the number of exceptions we seem to be > amassing, that does not sound like a bad idea for future. > 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. --------------000005050200020003020002 Content-Type: text/x-patch; name="disk-direct-cache-read.patch" Content-Transfer-Encoding: quoted-printable Content-Disposition: attachment; filename="disk-direct-cache-read.patch" From: Andrei Borzenkov Subject: [PATCH] disk: read into cache directly --- grub-core/kern/disk.c | 94 ++++++++++++++++++++++++++++++---------------= ------ grub-core/lib/disk.c | 6 ++-- include/grub/disk.h | 11 +++++- 3 files changed, 69 insertions(+), 42 deletions(-) diff --git a/grub-core/kern/disk.c b/grub-core/kern/disk.c index 789f8c0..945c146 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" =20 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 =3D grub_disk_cache_table + i; =20 - if (cache->data && ! cache->lock) + if (cache->buffer && ! cache->lock) { - grub_free (cache->data); - cache->data =3D 0; + grub_disk_cache_free (cache->buffer); + cache->buffer =3D 0; } } } @@ -82,14 +96,14 @@ grub_disk_cache_fetch (unsigned long dev_id, unsigned= long disk_id, cache_index =3D grub_disk_cache_get_index (dev_id, disk_id, sector); cache =3D grub_disk_cache_table + cache_index; =20 - if (cache->dev_id =3D=3D dev_id && cache->disk_id =3D=3D disk_id + if (cache->buffer && cache->dev_id =3D=3D dev_id && cache->disk_id =3D= =3D disk_id && cache->sector =3D=3D sector) { cache->lock =3D 1; #if DISK_CACHE_STATS grub_disk_cache_hits++; #endif - return cache->data; + return cache->buffer->data + cache->offset; } =20 #if DISK_CACHE_STATS @@ -116,28 +130,35 @@ grub_disk_cache_unlock (unsigned long dev_id, unsig= ned long disk_id, =20 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; =20 - cache_index =3D grub_disk_cache_get_index (dev_id, disk_id, sector); - cache =3D grub_disk_cache_table + cache_index; + buf =3D grub_malloc (sizeof (*buf)); + if (! buf) + return grub_errno; + buf->data =3D data; + buf->count =3D 0; =20 - cache->lock =3D 1; - grub_free (cache->data); - cache->data =3D 0; - cache->lock =3D 0; + for (offset =3D 0 ; n > 0; sector +=3D GRUB_DISK_CACHE_SIZE, offset +=3D= (GRUB_DISK_CACHE_SIZE * GRUB_DISK_SECTOR_SIZE), n--) + { + unsigned cache_index; + struct grub_disk_cache *cache; =20 - cache->data =3D grub_malloc (GRUB_DISK_SECTOR_SIZE << GRUB_DISK_CACHE_= BITS); - if (! cache->data) - return grub_errno; + cache_index =3D grub_disk_cache_get_index (dev_id, disk_id, sector= ); + cache =3D grub_disk_cache_table + cache_index; =20 - grub_memcpy (cache->data, data, - GRUB_DISK_SECTOR_SIZE << GRUB_DISK_CACHE_BITS); - cache->dev_id =3D dev_id; - cache->disk_id =3D disk_id; - cache->sector =3D sector; + cache->lock =3D 1; + grub_disk_cache_free (cache->buffer); + cache->buffer =3D buf; + cache->offset =3D offset; + buf->count++; + cache->lock =3D 0; + cache->dev_id =3D dev_id; + cache->disk_id =3D disk_id; + cache->sector =3D sector; + } =20 return GRUB_ERR_NONE; } @@ -350,13 +371,11 @@ grub_disk_read_small_real (grub_disk_t disk, grub_d= isk_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; } } =20 - grub_free (tmp_buf); grub_errno =3D GRUB_ERR_NONE; =20 { @@ -373,10 +392,6 @@ grub_disk_read_small_real (grub_disk_t disk, grub_di= sk_addr_t sector, num =3D ((size + offset + (1ULL << (disk->log_sector_size)) - 1) >> (disk->log_sector_size)); =20 - tmp_buf =3D grub_malloc (num << disk->log_sector_size); - if (!tmp_buf) - return grub_errno; - =20 if ((disk->dev->read) (disk, transform_sector (disk, aligned_sector)= , num, tmp_buf)) { @@ -481,22 +496,25 @@ grub_disk_read (grub_disk_t disk, grub_disk_addr_t = sector, =20 if (agglomerate) { - grub_disk_addr_t i; + void *cache =3D grub_malloc (agglomerate << (GRUB_DISK_CACHE_BITS + G= RUB_DISK_SECTOR_BITS)); + if (!cache) + return grub_errno; =20 err =3D (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; + } =20 - for (i =3D 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); =20 =20 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, uns= igned long disk_id, cache =3D grub_disk_cache_table + cache_index; =20 if (cache->dev_id =3D=3D dev_id && cache->disk_id =3D=3D disk_id - && cache->sector =3D=3D sector && cache->data) + && cache->sector =3D=3D sector && cache->buffer) { cache->lock =3D 1; - grub_free (cache->data); - cache->data =3D 0; + grub_disk_cache_free (cache->buffer); + cache->buffer =3D 0; cache->lock =3D 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) } } =20 +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; }; =20 +void EXPORT_FUNC(grub_disk_cache_free) (struct grub_cache_buffer *buf); + extern struct grub_disk_cache EXPORT_VAR(grub_disk_cache_table)[GRUB_DIS= K_CACHE_NUM]; =20 #if defined (GRUB_UTIL) --=20 tg: (bc22096..) e/disk-cache-align (depends on: master) --------------000005050200020003020002--