From 50836e76a1e756f6e5160737af224ffb24029d38 Mon Sep 17 00:00:00 2001 From: David Saada Date: Thu, 8 Nov 2018 19:54:18 +0200 Subject: [PATCH] Improve the efficiency of BufferedBlockDevice --- .../buffered_block_device/main.cpp | 226 +++++++++++------- .../blockdevice/BufferedBlockDevice.cpp | 160 ++++++++----- .../storage/blockdevice/BufferedBlockDevice.h | 15 +- 3 files changed, 252 insertions(+), 149 deletions(-) diff --git a/features/storage/TESTS/blockdevice/buffered_block_device/main.cpp b/features/storage/TESTS/blockdevice/buffered_block_device/main.cpp index dc25bdab7a..6889cb4c2e 100644 --- a/features/storage/TESTS/blockdevice/buffered_block_device/main.cpp +++ b/features/storage/TESTS/blockdevice/buffered_block_device/main.cpp @@ -24,105 +24,155 @@ using namespace utest::v1; static const bd_size_t heap_erase_size = 512; -static const bd_size_t heap_prog_size = heap_erase_size; -static const bd_size_t heap_read_size = 256; static const bd_size_t num_blocks = 4; +typedef struct { + bd_size_t read_size; + bd_size_t prog_size; +} sizes_t; + +static const int num_tests = 4; + +sizes_t sizes[num_tests] = { + {1, 1}, + {1, 128}, + {4, 256}, + {256, 512} +}; + void functionality_test() { - uint8_t *dummy = new (std::nothrow) uint8_t[num_blocks * heap_erase_size + heap_prog_size]; - TEST_SKIP_UNLESS_MESSAGE(dummy, "Not enough memory for test"); - delete[] dummy; + for (int i = 0; i < num_tests; i++) { + bd_size_t heap_read_size = sizes[i].read_size; + bd_size_t heap_prog_size = sizes[i].prog_size; - HeapBlockDevice heap_bd(num_blocks * heap_erase_size, heap_read_size, heap_prog_size, heap_erase_size); - BufferedBlockDevice bd(&heap_bd); + printf("Testing read size of %lld, prog size of %lld\n", heap_read_size, heap_prog_size); - int err = bd.init(); - TEST_ASSERT_EQUAL(0, err); + uint8_t *read_buf, *write_buf; + read_buf = new (std::nothrow) uint8_t[heap_erase_size]; + TEST_SKIP_UNLESS_MESSAGE(read_buf, "Not enough memory for test"); + write_buf = new (std::nothrow) uint8_t[heap_erase_size]; + TEST_SKIP_UNLESS_MESSAGE(write_buf, "Not enough memory for test"); - uint8_t *read_buf, *write_buf; - read_buf = new (std::nothrow) uint8_t[heap_prog_size]; - TEST_SKIP_UNLESS_MESSAGE(read_buf, "Not enough memory for test"); - write_buf = new (std::nothrow) uint8_t[heap_prog_size]; - TEST_SKIP_UNLESS_MESSAGE(write_buf, "Not enough memory for test"); + uint8_t *dummy = new (std::nothrow) uint8_t[num_blocks * heap_erase_size + heap_prog_size + heap_read_size]; + TEST_SKIP_UNLESS_MESSAGE(dummy, "Not enough memory for test"); + delete[] dummy; - TEST_ASSERT_EQUAL(1, bd.get_read_size()); - TEST_ASSERT_EQUAL(1, bd.get_program_size()); - TEST_ASSERT_EQUAL(heap_erase_size, bd.get_erase_size()); + HeapBlockDevice *heap_bd = new HeapBlockDevice(num_blocks * heap_erase_size, heap_read_size, heap_prog_size, heap_erase_size); + BufferedBlockDevice *bd = new BufferedBlockDevice(heap_bd); - for (bd_size_t i = 0; i < num_blocks; i++) { - memset(write_buf, i, heap_prog_size); - err = heap_bd.program(write_buf, i * heap_prog_size, heap_prog_size); + int err = bd->init(); TEST_ASSERT_EQUAL(0, err); + + TEST_ASSERT_EQUAL(1, bd->get_read_size()); + TEST_ASSERT_EQUAL(1, bd->get_program_size()); + TEST_ASSERT_EQUAL(heap_erase_size, bd->get_erase_size()); + + for (bd_size_t i = 0; i < num_blocks; i++) { + memset(write_buf, i, heap_erase_size); + err = heap_bd->program(write_buf, i * heap_erase_size, heap_erase_size); + // Heap BD allocates memory on each program, so failure here indicates + // lack of memory - just skip test. + TEST_SKIP_UNLESS_MESSAGE(!err, "Not enough memory for test"); + } + + err = bd->read(read_buf, heap_erase_size + heap_erase_size / 2, 1); + TEST_ASSERT_EQUAL(0, err); + TEST_ASSERT_EQUAL(1, read_buf[0]); + + err = bd->read(read_buf, 2 * heap_erase_size + heap_erase_size / 2, 4); + TEST_ASSERT_EQUAL(0, err); + memset(write_buf, 2, 4); + TEST_ASSERT_EQUAL_UINT8_ARRAY(write_buf, read_buf, 4); + + memset(write_buf, 1, heap_erase_size); + memset(write_buf + 64, 0x5A, 8); + memset(write_buf + 72, 0xA5, 8); + err = bd->program(write_buf + 64, heap_erase_size + 64, 8); + TEST_ASSERT_EQUAL(0, err); + err = bd->program(write_buf + 72, heap_erase_size + 72, 8); + TEST_ASSERT_EQUAL(0, err); + err = bd->read(read_buf, heap_erase_size, heap_erase_size); + TEST_ASSERT_EQUAL(0, err); + TEST_ASSERT_EQUAL_UINT8_ARRAY(write_buf, read_buf, heap_erase_size); + memset(write_buf, 1, heap_erase_size); + // Underlying BD should not be updated before sync + err = heap_bd->read(read_buf, heap_erase_size, heap_erase_size); + TEST_ASSERT_EQUAL(0, err); + if (heap_prog_size > 1) { + TEST_ASSERT_EQUAL_UINT8_ARRAY(write_buf, read_buf, heap_erase_size); + } + err = bd->sync(); + TEST_ASSERT_EQUAL(0, err); + memset(write_buf + 64, 0x5A, 8); + memset(write_buf + 72, 0xA5, 8); + // Should be updated now + err = bd->read(read_buf, heap_erase_size, heap_erase_size); + TEST_ASSERT_EQUAL(0, err); + TEST_ASSERT_EQUAL_UINT8_ARRAY(write_buf, read_buf, heap_erase_size); + err = heap_bd->read(read_buf, heap_erase_size, heap_erase_size); + TEST_ASSERT_EQUAL(0, err); + TEST_ASSERT_EQUAL_UINT8_ARRAY(write_buf, read_buf, heap_erase_size); + + memset(write_buf, 0xAA, 16); + memset(write_buf + 16, 0xBB, 16); + err = bd->program(write_buf, 3 * heap_erase_size - 16, 32); + TEST_ASSERT_EQUAL(0, err); + // First block should sync, but second still shouldn't + memset(write_buf, 2, heap_erase_size - 16); + memset(write_buf + heap_erase_size - 16, 0xAA, 16); + err = heap_bd->read(read_buf, 2 * heap_erase_size, heap_erase_size); + TEST_ASSERT_EQUAL(0, err); + TEST_ASSERT_EQUAL_UINT8_ARRAY(write_buf, read_buf, heap_erase_size); + memset(write_buf, 3, heap_erase_size); + err = heap_bd->read(read_buf, 3 * heap_erase_size, heap_erase_size); + TEST_ASSERT_EQUAL(0, err); + if (heap_prog_size > 1) { + TEST_ASSERT_EQUAL_UINT8_ARRAY(write_buf, read_buf, heap_erase_size); + } + memset(write_buf, 0xBB, 16); + err = bd->read(read_buf, 3 * heap_erase_size, heap_erase_size); + TEST_ASSERT_EQUAL(0, err); + TEST_ASSERT_EQUAL_UINT8_ARRAY(write_buf, read_buf, heap_erase_size); + // Writing to another block should automatically sync + err = bd->program(write_buf, 15, 1); + TEST_ASSERT_EQUAL(0, err); + err = heap_bd->read(read_buf, 3 * heap_erase_size, heap_erase_size); + TEST_ASSERT_EQUAL(0, err); + TEST_ASSERT_EQUAL_UINT8_ARRAY(write_buf, read_buf, heap_erase_size); + err = bd->read(read_buf, 3 * heap_erase_size, heap_erase_size); + TEST_ASSERT_EQUAL(0, err); + TEST_ASSERT_EQUAL_UINT8_ARRAY(write_buf, read_buf, heap_erase_size); + + // Unaligned reads and writes + memset(write_buf, 2, 41); + memset(write_buf + 41, 0x39, 21); + err = bd->program(write_buf + 41, 2 * heap_erase_size + 41, 21); + TEST_ASSERT_EQUAL(0, err); + err = heap_bd->read(read_buf, 2 * heap_erase_size, heap_erase_size); + TEST_ASSERT_EQUAL(0, err); + if (heap_prog_size > 1) { + TEST_ASSERT_EQUAL_UINT8_ARRAY(write_buf, read_buf, 41); + TEST_ASSERT_EQUAL_UINT8_ARRAY(write_buf, read_buf + 41, 21); + } + err = bd->read(read_buf, 2 * heap_erase_size + 4, 41 + 21 - 4); + TEST_ASSERT_EQUAL(0, err); + TEST_ASSERT_EQUAL_UINT8_ARRAY(write_buf + 4, read_buf, 41 + 21 - 4); + bd->sync(); + err = heap_bd->read(read_buf, 2 * heap_erase_size, heap_erase_size); + TEST_ASSERT_EQUAL(0, err); + TEST_ASSERT_EQUAL_UINT8_ARRAY(write_buf, read_buf, 41 + 21); + err = bd->read(read_buf, 2 * heap_erase_size + 10, 41 + 21 - 10); + TEST_ASSERT_EQUAL(0, err); + TEST_ASSERT_EQUAL_UINT8_ARRAY(write_buf + 10, read_buf, 41 + 21 - 10); + + bd->deinit(); + + delete[] read_buf; + delete[] write_buf; + delete bd; + delete heap_bd; } - - err = bd.read(read_buf, heap_prog_size + heap_prog_size / 2, 1); - TEST_ASSERT_EQUAL(0, err); - TEST_ASSERT_EQUAL(1, read_buf[0]); - - err = bd.read(read_buf, 2 * heap_prog_size + heap_prog_size / 2, 4); - TEST_ASSERT_EQUAL(0, err); - memset(write_buf, 2, 4); - TEST_ASSERT_EQUAL_UINT8_ARRAY(write_buf, read_buf, 4); - - memset(write_buf, 1, heap_prog_size); - memset(write_buf + 64, 0x5A, 8); - memset(write_buf + 72, 0xA5, 8); - err = bd.program(write_buf + 64, heap_prog_size + 64, 8); - TEST_ASSERT_EQUAL(0, err); - err = bd.program(write_buf + 72, heap_prog_size + 72, 8); - TEST_ASSERT_EQUAL(0, err); - err = bd.read(read_buf, heap_prog_size, heap_prog_size); - TEST_ASSERT_EQUAL(0, err); - TEST_ASSERT_EQUAL_UINT8_ARRAY(write_buf, read_buf, heap_prog_size); - memset(write_buf, 1, heap_prog_size); - // Underlying BD should not be updated before sync - err = heap_bd.read(read_buf, heap_prog_size, heap_prog_size); - TEST_ASSERT_EQUAL(0, err); - TEST_ASSERT_EQUAL_UINT8_ARRAY(write_buf, read_buf, heap_prog_size); - err = bd.sync(); - TEST_ASSERT_EQUAL(0, err); - memset(write_buf + 64, 0x5A, 8); - memset(write_buf + 72, 0xA5, 8); - // Should be updated now - err = bd.read(read_buf, heap_prog_size, heap_prog_size); - TEST_ASSERT_EQUAL(0, err); - TEST_ASSERT_EQUAL_UINT8_ARRAY(write_buf, read_buf, heap_prog_size); - err = heap_bd.read(read_buf, heap_prog_size, heap_prog_size); - TEST_ASSERT_EQUAL(0, err); - TEST_ASSERT_EQUAL_UINT8_ARRAY(write_buf, read_buf, heap_prog_size); - - memset(write_buf, 0xAA, 16); - memset(write_buf + 16, 0xBB, 16); - err = bd.program(write_buf, 3 * heap_prog_size - 16, 32); - TEST_ASSERT_EQUAL(0, err); - // First block should sync, but second still shouldn't - memset(write_buf, 2, heap_prog_size - 16); - memset(write_buf + heap_prog_size - 16, 0xAA, 16); - err = heap_bd.read(read_buf, 2 * heap_prog_size, heap_prog_size); - TEST_ASSERT_EQUAL(0, err); - TEST_ASSERT_EQUAL_UINT8_ARRAY(write_buf, read_buf, heap_prog_size); - memset(write_buf, 3, heap_prog_size); - err = heap_bd.read(read_buf, 3 * heap_prog_size, heap_prog_size); - TEST_ASSERT_EQUAL(0, err); - TEST_ASSERT_EQUAL_UINT8_ARRAY(write_buf, read_buf, heap_prog_size); - memset(write_buf, 0xBB, 16); - err = bd.read(read_buf, 3 * heap_prog_size, heap_prog_size); - TEST_ASSERT_EQUAL(0, err); - TEST_ASSERT_EQUAL_UINT8_ARRAY(write_buf, read_buf, heap_prog_size); - // Moving to another block should automatically sync - err = bd.read(read_buf, 15, 1); - TEST_ASSERT_EQUAL(0, err); - TEST_ASSERT_EQUAL(0, read_buf[0]); - err = heap_bd.read(read_buf, 3 * heap_prog_size, heap_prog_size); - TEST_ASSERT_EQUAL(0, err); - TEST_ASSERT_EQUAL_UINT8_ARRAY(write_buf, read_buf, heap_prog_size); - err = bd.read(read_buf, 3 * heap_prog_size, heap_prog_size); - TEST_ASSERT_EQUAL(0, err); - TEST_ASSERT_EQUAL_UINT8_ARRAY(write_buf, read_buf, heap_prog_size); - - delete[] read_buf; - delete[] write_buf; } // Test setup diff --git a/features/storage/blockdevice/BufferedBlockDevice.cpp b/features/storage/blockdevice/BufferedBlockDevice.cpp index 24c615710a..75e6cf5092 100644 --- a/features/storage/blockdevice/BufferedBlockDevice.cpp +++ b/features/storage/blockdevice/BufferedBlockDevice.cpp @@ -26,7 +26,8 @@ static inline uint32_t align_down(bd_size_t val, bd_size_t size) } BufferedBlockDevice::BufferedBlockDevice(BlockDevice *bd) - : _bd(bd), _bd_program_size(0), _curr_aligned_addr(0), _flushed(true), _cache(0), _init_ref_count(0), _is_initialized(false) + : _bd(bd), _bd_program_size(0), _bd_read_size(0), _write_cache_addr(0), _write_cache_valid(false), + _write_cache(0), _read_buf(0), _init_ref_count(0), _is_initialized(false) { } @@ -48,14 +49,19 @@ int BufferedBlockDevice::init() return err; } + _bd_read_size = _bd->get_read_size(); _bd_program_size = _bd->get_program_size(); + _bd_size = _bd->size(); - if (!_cache) { - _cache = new uint8_t[_bd_program_size]; + if (!_write_cache) { + _write_cache = new uint8_t[_bd_program_size]; } - _curr_aligned_addr = _bd->size(); - _flushed = true; + if (!_read_buf) { + _read_buf = new uint8_t[_bd_read_size]; + } + + invalidate_write_cache(); _is_initialized = true; return BD_ERROR_OK; @@ -73,34 +79,44 @@ int BufferedBlockDevice::deinit() return BD_ERROR_OK; } - delete[] _cache; - _cache = 0; + delete[] _write_cache; + _write_cache = 0; + delete[] _read_buf; + _read_buf = 0; _is_initialized = false; return _bd->deinit(); } int BufferedBlockDevice::flush() { + MBED_ASSERT(_write_cache); if (!_is_initialized) { return BD_ERROR_DEVICE_ERROR; } - if (!_flushed) { - int ret = _bd->program(_cache, _curr_aligned_addr, _bd_program_size); + if (_write_cache_valid) { + int ret = _bd->program(_write_cache, _write_cache_addr, _bd_program_size); if (ret) { return ret; } - _flushed = true; + invalidate_write_cache(); } return 0; } +void BufferedBlockDevice::invalidate_write_cache() +{ + _write_cache_addr = _bd_size; + _write_cache_valid = false; +} + int BufferedBlockDevice::sync() { if (!_is_initialized) { return BD_ERROR_DEVICE_ERROR; } + MBED_ASSERT(_write_cache); int ret = flush(); if (ret) { return ret; @@ -110,36 +126,52 @@ int BufferedBlockDevice::sync() int BufferedBlockDevice::read(void *b, bd_addr_t addr, bd_size_t size) { - MBED_ASSERT(_cache); if (!_is_initialized) { return BD_ERROR_DEVICE_ERROR; } - bool moved_unit = false; - - bd_addr_t aligned_addr = align_down(addr, _bd_program_size); + MBED_ASSERT(_write_cache && _read_buf); + // Common case - no need to involve write cache or read buffer + if (_bd->is_valid_read(addr, size) && + ((addr + size <= _write_cache_addr) || (addr > _write_cache_addr + _bd_program_size))) { + return _bd->read(b, addr, size); + } uint8_t *buf = static_cast(b); - if (aligned_addr != _curr_aligned_addr) { - // Need to flush if moved to another program unit - flush(); - _curr_aligned_addr = aligned_addr; - moved_unit = true; - } - + // Read logic: Split read to chunks, according to whether we cross the write cache while (size) { - _curr_aligned_addr = align_down(addr, _bd_program_size); - if (moved_unit) { - int ret = _bd->read(_cache, _curr_aligned_addr, _bd_program_size); + bd_size_t chunk; + bool read_from_bd = true; + if (addr < _write_cache_addr) { + chunk = std::min(size, _write_cache_addr - addr); + } else if ((addr >= _write_cache_addr) && (addr < _write_cache_addr + _bd_program_size)) { + // One case we need to take our data from cache + chunk = std::min(size, _bd_program_size - addr % _bd_program_size); + memcpy(buf, _write_cache + addr % _bd_program_size, chunk); + read_from_bd = false; + } else { + chunk = size; + } + + // Now, in case we read from the BD, make sure we are aligned with its read size. + // If not, use read buffer as a helper. + if (read_from_bd) { + bd_size_t offs_in_read_buf = addr % _bd_read_size; + int ret; + if (offs_in_read_buf || (chunk < _bd_read_size)) { + chunk = std::min(chunk, _bd_read_size - offs_in_read_buf); + ret = _bd->read(_read_buf, addr - offs_in_read_buf, _bd_read_size); + memcpy(buf, _read_buf + offs_in_read_buf, chunk); + } else { + chunk = align_down(chunk, _bd_read_size); + ret = _bd->read(buf, addr, chunk); + } if (ret) { return ret; } } - bd_addr_t offs_in_buf = addr - _curr_aligned_addr; - bd_size_t chunk = std::min(_bd_program_size - offs_in_buf, size); - memcpy(buf, _cache + offs_in_buf, chunk); - moved_unit = true; + buf += chunk; addr += chunk; size -= chunk; @@ -154,58 +186,68 @@ int BufferedBlockDevice::program(const void *b, bd_addr_t addr, bd_size_t size) return BD_ERROR_DEVICE_ERROR; } + MBED_ASSERT(_write_cache); + int ret; - bool moved_unit = false; bd_addr_t aligned_addr = align_down(addr, _bd_program_size); const uint8_t *buf = static_cast (b); // Need to flush if moved to another program unit - if (aligned_addr != _curr_aligned_addr) { - flush(); - _curr_aligned_addr = aligned_addr; - moved_unit = true; + if (aligned_addr != _write_cache_addr) { + ret = flush(); + if (ret) { + return ret; + } + _write_cache_addr = aligned_addr; } + // Write logic: Keep data in cache as long as we don't reach the end of the program unit. + // Otherwise, program to the underlying BD. while (size) { - _curr_aligned_addr = align_down(addr, _bd_program_size); - bd_addr_t offs_in_buf = addr - _curr_aligned_addr; - bd_size_t chunk = std::min(_bd_program_size - offs_in_buf, size); + _write_cache_addr = align_down(addr, _bd_program_size); + bd_addr_t offs_in_buf = addr - _write_cache_addr; + bd_size_t chunk; + if (offs_in_buf) { + chunk = std::min(_bd_program_size - offs_in_buf, size); + } else if (size >= _bd_program_size) { + chunk = align_down(size, _bd_program_size); + } else { + chunk = size; + } + const uint8_t *prog_buf; if (chunk < _bd_program_size) { - // If moved a unit, and program doesn't cover entire unit, it means we don't have the entire - // program unit cached - need to complete it from underlying BD - if (moved_unit) { - ret = _bd->read(_cache, _curr_aligned_addr, _bd_program_size); + // If cache not valid, and program doesn't cover an entire unit, it means we need to + // read it from the underlying BD + if (!_write_cache_valid) { + ret = _bd->read(_write_cache, _write_cache_addr, _bd_program_size); if (ret) { return ret; } } - memcpy(_cache + offs_in_buf, buf, chunk); - prog_buf = _cache; + memcpy(_write_cache + offs_in_buf, buf, chunk); + prog_buf = _write_cache; } else { - // No need to copy data to our cache on each iteration. Just make sure it's updated - // on the last iteration, when size is not greater than program size (can't be smaller, as - // this is covered in the previous condition). prog_buf = buf; - if (size == _bd_program_size) { - memcpy(_cache, buf, _bd_program_size); - } } - // Don't flush on the last iteration, just on all preceding ones. - if (size > chunk) { - ret = _bd->program(prog_buf, _curr_aligned_addr, _bd_program_size); + // Only program if we reached the end of a program unit + if (!((offs_in_buf + chunk) % _bd_program_size)) { + ret = _bd->program(prog_buf, _write_cache_addr, std::max(chunk, _bd_program_size)); if (ret) { return ret; } - _bd->sync(); + ret = _bd->sync(); + if (ret) { + return ret; + } + invalidate_write_cache(); } else { - _flushed = false; + _write_cache_valid = true; } - moved_unit = true; buf += chunk; addr += chunk; size -= chunk; @@ -221,6 +263,9 @@ int BufferedBlockDevice::erase(bd_addr_t addr, bd_size_t size) return BD_ERROR_DEVICE_ERROR; } + if ((_write_cache_addr >= addr) && (_write_cache_addr <= addr + size)) { + invalidate_write_cache(); + } return _bd->erase(addr, size); } @@ -231,9 +276,8 @@ int BufferedBlockDevice::trim(bd_addr_t addr, bd_size_t size) return BD_ERROR_DEVICE_ERROR; } - if ((_curr_aligned_addr >= addr) && (_curr_aligned_addr <= addr + size)) { - _flushed = true; - _curr_aligned_addr = _bd->size(); + if ((_write_cache_addr >= addr) && (_write_cache_addr <= addr + size)) { + invalidate_write_cache(); } return _bd->trim(addr, size); } @@ -281,5 +325,5 @@ bd_size_t BufferedBlockDevice::size() const return 0; } - return _bd->size(); + return _bd_size; } diff --git a/features/storage/blockdevice/BufferedBlockDevice.h b/features/storage/blockdevice/BufferedBlockDevice.h index 563dc08ae4..1afae28e56 100644 --- a/features/storage/blockdevice/BufferedBlockDevice.h +++ b/features/storage/blockdevice/BufferedBlockDevice.h @@ -150,9 +150,12 @@ public: protected: BlockDevice *_bd; bd_size_t _bd_program_size; - bd_size_t _curr_aligned_addr; - bool _flushed; - uint8_t *_cache; + bd_size_t _bd_read_size; + bd_size_t _bd_size; + bd_size_t _write_cache_addr; + bool _write_cache_valid; + uint8_t *_write_cache; + uint8_t *_read_buf; uint32_t _init_ref_count; bool _is_initialized; @@ -162,6 +165,12 @@ protected: */ int flush(); + /** Invalidate write cache + * + * @return none + */ + void invalidate_write_cache(); + };