* [PATCH v2 0/3] some disk reworking reposted
@ 2016-03-02 0:14 Leif Lindholm
2016-03-02 0:14 ` [PATCH v2 1/3] disk: Add support for device-specific malloc function Leif Lindholm
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Leif Lindholm @ 2016-03-02 0:14 UTC (permalink / raw)
To: grub-devel
So, looking into this a bit more, thinking of how I could potentially
manage to sneak this into 2.02, and so on, I decided to rebase Andrei's
direct cache buffer patch onto the others.
Changes since v1:
- disk_malloc() function now used as default for all disks, rather than
a runtime check for presence of disk-specific one
- adding Andrei's direct cache buffer use patch
(obviously happy for this one to be treated separately, but made sense
to include the rebased version with this posting)
Andrei Borzenkov (1):
disk: read into cache directly
Leif Lindholm (2):
disk: Add support for device-specific malloc function
efidisk: implement alignment-respecting malloc function
grub-core/disk/efi/efidisk.c | 8 ++++
grub-core/kern/disk.c | 105 +++++++++++++++++++++++++++----------------
grub-core/lib/disk.c | 6 +--
include/grub/disk.h | 16 ++++++-
4 files changed, 92 insertions(+), 43 deletions(-)
--
2.1.4
^ permalink raw reply [flat|nested] 10+ messages in thread* [PATCH v2 1/3] disk: Add support for device-specific malloc function 2016-03-02 0:14 [PATCH v2 0/3] some disk reworking reposted Leif Lindholm @ 2016-03-02 0:14 ` Leif Lindholm 2016-03-02 0:14 ` [PATCH v2 2/3] efidisk: implement alignment-respecting " Leif Lindholm 2016-03-02 0:14 ` [PATCH v2 3/3] disk: read into cache directly Leif Lindholm 2 siblings, 0 replies; 10+ messages in thread From: Leif Lindholm @ 2016-03-02 0:14 UTC (permalink / raw) To: grub-devel 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. --- grub-core/kern/disk.c | 11 +++++++++-- include/grub/disk.h | 5 +++++ 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/grub-core/kern/disk.c b/grub-core/kern/disk.c index 789f8c0..60a920b 100644 --- a/grub-core/kern/disk.c +++ b/grub-core/kern/disk.c @@ -184,6 +184,12 @@ find_part_sep (const char *name) return NULL; } +static void * +disk_malloc (struct grub_disk *dummy __attribute__ ((unused)), grub_size_t size) +{ + return grub_malloc (size); +} + grub_disk_t grub_disk_open (const char *name) { @@ -199,6 +205,7 @@ grub_disk_open (const char *name) if (! disk) return 0; disk->log_sector_size = GRUB_DISK_SECTOR_BITS; + disk->malloc = disk_malloc; /* Can be overridden by individual driver. */ /* Default 1MiB of maximum agglomerate. */ disk->max_agglomerate = 1048576 >> (GRUB_DISK_SECTOR_BITS + GRUB_DISK_CACHE_BITS); @@ -331,7 +338,7 @@ grub_disk_read_small_real (grub_disk_t disk, grub_disk_addr_t sector, } /* Allocate a temporary buffer. */ - tmp_buf = grub_malloc (GRUB_DISK_SECTOR_SIZE << GRUB_DISK_CACHE_BITS); + tmp_buf = disk->malloc (disk, GRUB_DISK_SECTOR_SIZE << GRUB_DISK_CACHE_BITS); if (! tmp_buf) return grub_errno; @@ -373,7 +380,7 @@ 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); + tmp_buf = disk->malloc (disk, num << disk->log_sector_size); if (!tmp_buf) return grub_errno; diff --git a/include/grub/disk.h b/include/grub/disk.h index b385af8..0fdd779 100644 --- a/include/grub/disk.h +++ b/include/grub/disk.h @@ -111,6 +111,8 @@ typedef void (*grub_disk_read_hook_t) (grub_disk_addr_t sector, unsigned offset, unsigned length, void *data); +typedef void *(*grub_disk_malloc_t) (struct grub_disk *disk, grub_size_t size); + /* Disk. */ struct grub_disk { @@ -144,6 +146,9 @@ struct grub_disk /* Device-specific data. */ void *data; + + /* Device-specific malloc function. */ + grub_disk_malloc_t malloc; }; typedef struct grub_disk *grub_disk_t; -- 2.1.4 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v2 2/3] efidisk: implement alignment-respecting malloc function 2016-03-02 0:14 [PATCH v2 0/3] some disk reworking reposted Leif Lindholm 2016-03-02 0:14 ` [PATCH v2 1/3] disk: Add support for device-specific malloc function Leif Lindholm @ 2016-03-02 0:14 ` Leif Lindholm 2016-03-02 3:46 ` Andrei Borzenkov 2016-03-02 0:14 ` [PATCH v2 3/3] disk: read into cache directly Leif Lindholm 2 siblings, 1 reply; 10+ messages in thread From: Leif Lindholm @ 2016-03-02 0:14 UTC (permalink / raw) To: grub-devel Implement a driver-specific malloc function that allocates a buffer with the alignment specified by grub_efi_block_io_media structure. --- grub-core/disk/efi/efidisk.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/grub-core/disk/efi/efidisk.c b/grub-core/disk/efi/efidisk.c index 3b79f7b..ac99ea7 100644 --- a/grub-core/disk/efi/efidisk.c +++ b/grub-core/disk/efi/efidisk.c @@ -457,6 +457,13 @@ get_device (struct grub_efidisk_data *devices, int num) return 0; } +static void * +aligned_malloc (struct grub_disk *disk, grub_size_t size) +{ + struct grub_efidisk_data *d = disk->data; + return grub_memalign (d->block_io->media->io_align, size); +} + static grub_err_t grub_efidisk_open (const char *name, struct grub_disk *disk) { @@ -512,6 +519,7 @@ grub_efidisk_open (const char *name, struct grub_disk *disk) (1U << disk->log_sector_size) < m->block_size; disk->log_sector_size++); disk->data = d; + disk->malloc = aligned_malloc; grub_dprintf ("efidisk", "opening %s succeeded\n", name); -- 2.1.4 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/3] efidisk: implement alignment-respecting malloc function 2016-03-02 0:14 ` [PATCH v2 2/3] efidisk: implement alignment-respecting " Leif Lindholm @ 2016-03-02 3:46 ` Andrei Borzenkov 2016-03-02 14:00 ` Leif Lindholm 0 siblings, 1 reply; 10+ messages in thread From: Andrei Borzenkov @ 2016-03-02 3:46 UTC (permalink / raw) To: grub-devel 02.03.2016 03:14, Leif Lindholm пишет: > Implement a driver-specific malloc function that allocates a buffer > with the alignment specified by grub_efi_block_io_media structure. > --- > grub-core/disk/efi/efidisk.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/grub-core/disk/efi/efidisk.c b/grub-core/disk/efi/efidisk.c > index 3b79f7b..ac99ea7 100644 > --- a/grub-core/disk/efi/efidisk.c > +++ b/grub-core/disk/efi/efidisk.c > @@ -457,6 +457,13 @@ get_device (struct grub_efidisk_data *devices, int num) > return 0; > } > > +static void * > +aligned_malloc (struct grub_disk *disk, grub_size_t size) > +{ > + struct grub_efidisk_data *d = disk->data; > + return grub_memalign (d->block_io->media->io_align, size); > +} > + > static grub_err_t > grub_efidisk_open (const char *name, struct grub_disk *disk) > { > @@ -512,6 +519,7 @@ grub_efidisk_open (const char *name, struct grub_disk *disk) > (1U << disk->log_sector_size) < m->block_size; > disk->log_sector_size++); > disk->data = d; > + disk->malloc = aligned_malloc; > > grub_dprintf ("efidisk", "opening %s succeeded\n", name); > > Did you drop sanity check in grub_efidisk_open intentionally? ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/3] efidisk: implement alignment-respecting malloc function 2016-03-02 3:46 ` Andrei Borzenkov @ 2016-03-02 14:00 ` Leif Lindholm 0 siblings, 0 replies; 10+ messages in thread From: Leif Lindholm @ 2016-03-02 14:00 UTC (permalink / raw) To: The development of GNU GRUB On Wed, Mar 02, 2016 at 06:46:52AM +0300, Andrei Borzenkov wrote: > 02.03.2016 03:14, Leif Lindholm пишет: > > Implement a driver-specific malloc function that allocates a buffer > > with the alignment specified by grub_efi_block_io_media structure. > > --- > > grub-core/disk/efi/efidisk.c | 8 ++++++++ > > 1 file changed, 8 insertions(+) > > > > diff --git a/grub-core/disk/efi/efidisk.c b/grub-core/disk/efi/efidisk.c > > index 3b79f7b..ac99ea7 100644 > > --- a/grub-core/disk/efi/efidisk.c > > +++ b/grub-core/disk/efi/efidisk.c > > @@ -457,6 +457,13 @@ get_device (struct grub_efidisk_data *devices, int num) > > return 0; > > } > > > > +static void * > > +aligned_malloc (struct grub_disk *disk, grub_size_t size) > > +{ > > + struct grub_efidisk_data *d = disk->data; > > + return grub_memalign (d->block_io->media->io_align, size); > > +} > > + > > static grub_err_t > > grub_efidisk_open (const char *name, struct grub_disk *disk) > > { > > @@ -512,6 +519,7 @@ grub_efidisk_open (const char *name, struct grub_disk *disk) > > (1U << disk->log_sector_size) < m->block_size; > > disk->log_sector_size++); > > disk->data = d; > > + disk->malloc = aligned_malloc; > > > > grub_dprintf ("efidisk", "opening %s succeeded\n", name); > > Did you drop sanity check in grub_efidisk_open intentionally? Not consiously, but my unconsious may well have done it intentionally. Mmmno, I left it out because 51f375d688529b7c1819ba40188ee52b9333887c already includes that. / Leif ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2 3/3] disk: read into cache directly 2016-03-02 0:14 [PATCH v2 0/3] some disk reworking reposted Leif Lindholm 2016-03-02 0:14 ` [PATCH v2 1/3] disk: Add support for device-specific malloc function Leif Lindholm 2016-03-02 0:14 ` [PATCH v2 2/3] efidisk: implement alignment-respecting " Leif Lindholm @ 2016-03-02 0:14 ` Leif Lindholm 2016-03-02 0:22 ` Vladimir 'phcoder' Serbinenko 2016-03-03 0:04 ` Elliott, Robert (Persistent Memory) 2 siblings, 2 replies; 10+ messages in thread From: Leif Lindholm @ 2016-03-02 0:14 UTC (permalink / raw) To: grub-devel From: Andrei Borzenkov <arvidjaar@gmail.com> <patch description> --- grub-core/kern/disk.c | 96 +++++++++++++++++++++++++++++++-------------------- grub-core/lib/disk.c | 6 ++-- include/grub/disk.h | 11 +++++- 3 files changed, 71 insertions(+), 42 deletions(-) diff --git a/grub-core/kern/disk.c b/grub-core/kern/disk.c index 60a920b..0acf660 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; } @@ -357,13 +379,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; { @@ -380,10 +400,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 = disk->malloc (disk, num << disk->log_sector_size); - if (!tmp_buf) - return grub_errno; - if ((disk->dev->read) (disk, transform_sector (disk, aligned_sector), num, tmp_buf)) { @@ -488,22 +504,26 @@ grub_disk_read (grub_disk_t disk, grub_disk_addr_t sector, if (agglomerate) { - grub_disk_addr_t i; + void *cache = disk->malloc (disk, 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 0fdd779..bbb7830 100644 --- a/include/grub/disk.h +++ b/include/grub/disk.h @@ -238,16 +238,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) -- 2.1.4 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v2 3/3] disk: read into cache directly 2016-03-02 0:14 ` [PATCH v2 3/3] disk: read into cache directly Leif Lindholm @ 2016-03-02 0:22 ` Vladimir 'phcoder' Serbinenko 2016-03-02 3:46 ` Andrei Borzenkov 2016-03-03 0:04 ` Elliott, Robert (Persistent Memory) 1 sibling, 1 reply; 10+ messages in thread From: Vladimir 'phcoder' Serbinenko @ 2016-03-02 0:22 UTC (permalink / raw) To: The development of GNU GRUB [-- Attachment #1: Type: text/plain, Size: 8549 bytes --] Is there any way this patch reclaims unused memory in case of partial cache eviction? If not it could potentially water lots of memory. One thing you need to consider is that initrd for Solaris can easily be 60% of total RAM (300 MiB on 512MiB machine) What if requested read is bigger than risk cache size? Le mer. 2 mars 2016 01:15, Leif Lindholm <leif.lindholm@linaro.org> a écrit : > From: Andrei Borzenkov <arvidjaar@gmail.com> > > <patch description> > --- > grub-core/kern/disk.c | 96 > +++++++++++++++++++++++++++++++-------------------- > grub-core/lib/disk.c | 6 ++-- > include/grub/disk.h | 11 +++++- > 3 files changed, 71 insertions(+), 42 deletions(-) > > diff --git a/grub-core/kern/disk.c b/grub-core/kern/disk.c > index 60a920b..0acf660 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; > } > @@ -357,13 +379,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; > > { > @@ -380,10 +400,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 = disk->malloc (disk, num << disk->log_sector_size); > - if (!tmp_buf) > - return grub_errno; > - > if ((disk->dev->read) (disk, transform_sector (disk, aligned_sector), > num, tmp_buf)) > { > @@ -488,22 +504,26 @@ grub_disk_read (grub_disk_t disk, grub_disk_addr_t > sector, > > if (agglomerate) > { > - grub_disk_addr_t i; > + void *cache = disk->malloc (disk, 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 0fdd779..bbb7830 100644 > --- a/include/grub/disk.h > +++ b/include/grub/disk.h > @@ -238,16 +238,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) > -- > 2.1.4 > > > _______________________________________________ > Grub-devel mailing list > Grub-devel@gnu.org > https://lists.gnu.org/mailman/listinfo/grub-devel > [-- Attachment #2: Type: text/html, Size: 10528 bytes --] ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 3/3] disk: read into cache directly 2016-03-02 0:22 ` Vladimir 'phcoder' Serbinenko @ 2016-03-02 3:46 ` Andrei Borzenkov 2016-03-02 9:55 ` Vladimir 'phcoder' Serbinenko 0 siblings, 1 reply; 10+ messages in thread From: Andrei Borzenkov @ 2016-03-02 3:46 UTC (permalink / raw) To: grub-devel 02.03.2016 03:22, Vladimir 'phcoder' Serbinenko пишет: > Is there any way this patch reclaims unused memory in case of partial cache > eviction? Not yet, I wanted to make sure base looks sane first. Are there any cases of partial eviction besides blocklist write? > If not it could potentially water lots of memory. One thing you > need to consider is that initrd for Solaris can easily be 60% of total RAM > (300 MiB on 512MiB machine) What if requested read is bigger than risk > cache size? > Not sure I parse it; but yes, it puts more stress on cache by requiring unfragmented memory. If we follow this route we need to gracefully fall back to cache element size. Do we have any reasonable chance to separate aligned and non-aligned memory pools? ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 3/3] disk: read into cache directly 2016-03-02 3:46 ` Andrei Borzenkov @ 2016-03-02 9:55 ` Vladimir 'phcoder' Serbinenko 0 siblings, 0 replies; 10+ messages in thread From: Vladimir 'phcoder' Serbinenko @ 2016-03-02 9:55 UTC (permalink / raw) To: The development of GNU GRUB [-- Attachment #1: Type: text/plain, Size: 1855 bytes --] Le Wed, Mar 2, 2016 à 4:46 AM, Andrei Borzenkov <arvidjaar@gmail.com> a écrit : > 02.03.2016 03:22, Vladimir 'phcoder' Serbinenko пишет: > > Is there any way this patch reclaims unused memory in case of partial > cache > > eviction? > > Not yet, I wanted to make sure base looks sane first. Are there any > cases of partial eviction besides blocklist write? > > I mean that if you read directly 1M, then you invalidate 255 x 4K, then you really have only 4K of cache left but you still use 1M of RAM. This creates additional memory pressure. Good thing is that grub_malloc will invalidate whole cache and reclaim memory when it's unabkle to find any other spot but it still increases memory fragmentation as grub_malloc prefers squeezing before reclaiming memory. > > If not it could potentially water lots of memory. One thing you > > need to consider is that initrd for Solaris can easily be 60% of total > RAM > > (300 MiB on 512MiB machine) What if requested read is bigger than risk > > cache size? > > > > Not sure I parse it; but yes, it puts more stress on cache by requiring > unfragmented memory. If we follow this route we need to gracefully fall > back to cache element size. > > Problem that in this case caller already has 300M of RAM in buffer it passed and you can't allocate another 300M as machine simply doesn't have so much RAM. What's the cost of core size of your approach? What's speed benefit between your approach and just fixing read_small like Leif proposes? > Do we have any reasonable chance to separate aligned and non-aligned > memory pools? > > How would you allocate 60% of RAM in single chunk in this arrangement? > _______________________________________________ > Grub-devel mailing list > Grub-devel@gnu.org > https://lists.gnu.org/mailman/listinfo/grub-devel > [-- Attachment #2: Type: text/html, Size: 2716 bytes --] ^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [PATCH v2 3/3] disk: read into cache directly 2016-03-02 0:14 ` [PATCH v2 3/3] disk: read into cache directly Leif Lindholm 2016-03-02 0:22 ` Vladimir 'phcoder' Serbinenko @ 2016-03-03 0:04 ` Elliott, Robert (Persistent Memory) 1 sibling, 0 replies; 10+ messages in thread From: Elliott, Robert (Persistent Memory) @ 2016-03-03 0:04 UTC (permalink / raw) To: The development of GNU GRUB > -----Original Message----- > From: grub-devel-bounces+elliott=hp.com@gnu.org [mailto:grub-devel- > bounces+elliott=hp.com@gnu.org] On Behalf Of Leif Lindholm > Sent: Tuesday, March 01, 2016 6:14 PM > To: grub-devel@gnu.org > Subject: [PATCH v2 3/3] disk: read into cache directly > > From: Andrei Borzenkov <arvidjaar@gmail.com> > ... > 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); > + } > +} ... > - 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; ... > + if (cache->buffer) > + grub_disk_cache_free (cache->buffer); It's pretty common for free() functions to do NULL pointer checks so the caller doesn't have to; grub_free() is one such example. That makes cleanup code a bit safer. You might want to follow that pattern for this function. --- Robert Elliott, HPE Persistent Memory ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2016-03-03 0:05 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-03-02 0:14 [PATCH v2 0/3] some disk reworking reposted Leif Lindholm 2016-03-02 0:14 ` [PATCH v2 1/3] disk: Add support for device-specific malloc function Leif Lindholm 2016-03-02 0:14 ` [PATCH v2 2/3] efidisk: implement alignment-respecting " Leif Lindholm 2016-03-02 3:46 ` Andrei Borzenkov 2016-03-02 14:00 ` Leif Lindholm 2016-03-02 0:14 ` [PATCH v2 3/3] disk: read into cache directly Leif Lindholm 2016-03-02 0:22 ` Vladimir 'phcoder' Serbinenko 2016-03-02 3:46 ` Andrei Borzenkov 2016-03-02 9:55 ` Vladimir 'phcoder' Serbinenko 2016-03-03 0:04 ` Elliott, Robert (Persistent Memory)
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).