From c38c3ff3f0b68a7050f1b38b5c8e10a56b9d4429 Mon Sep 17 00:00:00 2001 From: Angus Gratton Date: Thu, 27 Feb 2020 11:57:00 +1100 Subject: [PATCH] spi_flash: Remove 16KB free internal heap limit for esp_flash_read() into PSRAM Allocation of the temporary internal buffer will now repeat until a small enough buffer can be allocated, and only fail if less than a 256 byte block of internal RAM is free. Adds unit test for the same, and generic test utility for creating memory pressure. --- components/spi_flash/esp_flash_api.c | 28 ++++++--- components/spi_flash/test/test_esp_flash.c | 60 +++++++++++++++---- .../test_utils/include/test_utils.h | 28 +++++++++ .../components/test_utils/test_utils.c | 38 ++++++++++++ 4 files changed, 136 insertions(+), 18 deletions(-) diff --git a/components/spi_flash/esp_flash_api.c b/components/spi_flash/esp_flash_api.c index faa3a61a5b..7073ab62e7 100644 --- a/components/spi_flash/esp_flash_api.c +++ b/components/spi_flash/esp_flash_api.c @@ -497,11 +497,25 @@ esp_err_t IRAM_ATTR esp_flash_read(esp_flash_t *chip, void *buffer, uint32_t add bool direct_read = chip->host->supports_direct_read(chip->host, buffer); uint8_t* temp_buffer = NULL; + //each time, we at most read this length + //after that, we release the lock to allow some other operations + size_t read_chunk_size = MIN(MAX_READ_CHUNK, length); + if (!direct_read) { - uint32_t length_to_allocate = MIN(MAX_READ_CHUNK, length); - length_to_allocate = (length_to_allocate+3)&(~3); - temp_buffer = heap_caps_malloc(length_to_allocate, MALLOC_CAP_INTERNAL | MALLOC_CAP_8BIT); - ESP_LOGV(TAG, "allocate temp buffer: %p (%d)", temp_buffer, length_to_allocate); + /* Allocate temporary internal buffer to use for the actual read. If the preferred size + doesn't fit in free internal memory, allocate the largest available free block. + + (May need to shrink read_chunk_size and retry due to race conditions with other tasks + also allocating from the heap.) + */ + unsigned retries = 5; + while(temp_buffer == NULL && retries--) { + read_chunk_size = MIN(read_chunk_size, heap_caps_get_largest_free_block(MALLOC_CAP_INTERNAL | MALLOC_CAP_8BIT)); + read_chunk_size = (read_chunk_size + 3) & ~3; + temp_buffer = heap_caps_malloc(read_chunk_size, MALLOC_CAP_INTERNAL | MALLOC_CAP_8BIT); + } + ESP_LOGV(TAG, "allocate temp buffer: %p (%d)", temp_buffer, read_chunk_size); + if (temp_buffer == NULL) { return ESP_ERR_NO_MEM; } @@ -516,9 +530,9 @@ esp_err_t IRAM_ATTR esp_flash_read(esp_flash_t *chip, void *buffer, uint32_t add } //if required (dma buffer allocated), read to the buffer instead of the original buffer uint8_t* buffer_to_read = (temp_buffer)? temp_buffer : buffer; - //each time, we at most read this length - //after that, we release the lock to allow some other operations - uint32_t length_to_read = MIN(MAX_READ_CHUNK, length); + + // Length we will read this iteration is either the chunk size or the remaining length, whichever is smaller + size_t length_to_read = MIN(read_chunk_size, length); if (err == ESP_OK) { err = chip->chip_drv->read(chip, buffer_to_read, address, length_to_read); diff --git a/components/spi_flash/test/test_esp_flash.c b/components/spi_flash/test/test_esp_flash.c index b223a93a0e..b89d53f510 100644 --- a/components/spi_flash/test/test_esp_flash.c +++ b/components/spi_flash/test/test_esp_flash.c @@ -3,7 +3,7 @@ #include #include #include - +#include #include #include "esp_flash.h" #include "driver/spi_common_internal.h" @@ -597,11 +597,26 @@ static void test_write_large_buffer(esp_flash_t *chip, const uint8_t *source, si #ifdef CONFIG_SPIRAM_USE_MALLOC +/* Utility: Read into a small internal RAM buffer using esp_flash_read() and compare what + we read with 'buffer' */ +static void s_test_compare_flash_contents_small_reads(esp_flash_t *chip, const uint8_t *buffer, size_t offs, size_t len) +{ + const size_t INTERNAL_BUF_SZ = 1024; // Should fit in internal RAM + uint8_t *ibuf = heap_caps_malloc(INTERNAL_BUF_SZ, MALLOC_CAP_8BIT|MALLOC_CAP_INTERNAL); + TEST_ASSERT_NOT_NULL(ibuf); + + for (int i = 0; i < len; i += INTERNAL_BUF_SZ) { + size_t to_read = MIN(INTERNAL_BUF_SZ, len - i); + ESP_ERROR_CHECK( esp_flash_read(chip, ibuf, offs + i, to_read) ); + TEST_ASSERT_EQUAL_HEX8_ARRAY(buffer + i, ibuf, to_read); + } + + free(ibuf); +} + static void test_flash_read_large_psram_buffer(esp_flash_t *chip) { const size_t BUF_SZ = 256 * 1024; // Too large for internal RAM - const size_t INTERNAL_BUF_SZ = 1024; // Should fit in internal RAM - _Static_assert(BUF_SZ % INTERNAL_BUF_SZ == 0, "should be a multiple"); const size_t TEST_OFFS = 0x1000; // Can be any offset, really uint8_t *buf = heap_caps_malloc(BUF_SZ, MALLOC_CAP_8BIT|MALLOC_CAP_SPIRAM); @@ -610,18 +625,41 @@ static void test_flash_read_large_psram_buffer(esp_flash_t *chip) ESP_ERROR_CHECK( esp_flash_read(chip, buf, TEST_OFFS, BUF_SZ) ); // Read back the same into smaller internal memory buffer and check it all matches - uint8_t *ibuf = heap_caps_malloc(INTERNAL_BUF_SZ, MALLOC_CAP_8BIT|MALLOC_CAP_INTERNAL); - TEST_ASSERT_NOT_NULL(ibuf); + s_test_compare_flash_contents_small_reads(chip, buf, TEST_OFFS, BUF_SZ); - for (int i = 0; i < BUF_SZ; i += INTERNAL_BUF_SZ) { - ESP_ERROR_CHECK( esp_flash_read(chip, ibuf, TEST_OFFS + i, INTERNAL_BUF_SZ) ); - TEST_ASSERT_EQUAL_HEX8_ARRAY(buf + i, ibuf, INTERNAL_BUF_SZ); - } - - free(ibuf); free(buf); } FLASH_TEST_CASE("esp_flash_read large PSRAM buffer", test_flash_read_large_psram_buffer); + +/* similar to above test, but perform it under memory pressure */ +static void test_flash_read_large_psram_buffer_low_internal_mem(esp_flash_t *chip) +{ + const size_t BUF_SZ = 256 * 1024; // Too large for internal RAM + const size_t REMAINING_INTERNAL = 1024; // Exhaust internal memory until maximum free block is less than this + const size_t TEST_OFFS = 0x8000; + + /* Exhaust the available free internal memory */ + test_utils_exhaust_memory_rec erec = test_utils_exhaust_memory(MALLOC_CAP_INTERNAL|MALLOC_CAP_8BIT, REMAINING_INTERNAL); + + uint8_t *buf = heap_caps_malloc(BUF_SZ, MALLOC_CAP_8BIT|MALLOC_CAP_SPIRAM); + TEST_ASSERT_NOT_NULL(buf); + + /* Calling esp_flash_read() here will need to allocate a small internal buffer, + so check it works. */ + ESP_ERROR_CHECK( esp_flash_read(chip, buf, TEST_OFFS, BUF_SZ) ); + + test_utils_free_exhausted_memory(erec); + + // Read back the same into smaller internal memory buffer and check it all matches + s_test_compare_flash_contents_small_reads(chip, buf, TEST_OFFS, BUF_SZ); + + free(buf); +} + +FLASH_TEST_CASE("esp_flash_read large PSRAM buffer low memory", test_flash_read_large_psram_buffer_low_internal_mem); + + + #endif diff --git a/tools/unit-test-app/components/test_utils/include/test_utils.h b/tools/unit-test-app/components/test_utils/include/test_utils.h index eb95f34f7f..c4783faf14 100644 --- a/tools/unit-test-app/components/test_utils/include/test_utils.h +++ b/tools/unit-test-app/components/test_utils/include/test_utils.h @@ -236,3 +236,31 @@ esp_err_t test_utils_set_leak_level(size_t leak_level, esp_type_leak_t type, esp * return Leak level */ size_t test_utils_get_leak_level(esp_type_leak_t type, esp_comp_leak_t component); + + + +typedef struct test_utils_exhaust_memory_record_s *test_utils_exhaust_memory_rec; + +/** + * Limit the largest free block of memory with a particular capability set to + * 'limit' bytes (meaning an allocation of 'limit' should succeed at least once, + * but any allocation of more bytes will fail.) + * + * Returns a record pointer which needs to be passed back in to test_utils_free_exhausted_memory + * before the test completes, to avoid a major memory leak. + * + * @param caps Capabilities of memory to exhause + * @param limit The size to limit largest free block to + * @return Record pointer to pass to test_utils_free_exhausted_memory() once done + */ +test_utils_exhaust_memory_rec test_utils_exhaust_memory(uint32_t caps, size_t limit); + + +/** + * Call to free memory which was taken up by test_utils_exhaust_memory() call + * + * @param rec Result previously returned from test_utils_exhaust_memory() + */ +void test_utils_free_exhausted_memory(test_utils_exhaust_memory_rec rec); + + diff --git a/tools/unit-test-app/components/test_utils/test_utils.c b/tools/unit-test-app/components/test_utils/test_utils.c index fc16db9f03..435051edb5 100644 --- a/tools/unit-test-app/components/test_utils/test_utils.c +++ b/tools/unit-test-app/components/test_utils/test_utils.c @@ -143,3 +143,41 @@ size_t test_utils_get_leak_level(esp_type_leak_t type_of_leak, esp_comp_leak_t c } return leak_level; } + + +#define EXHAUST_MEMORY_ENTRIES 100 + +struct test_utils_exhaust_memory_record_s { + int *entries[EXHAUST_MEMORY_ENTRIES]; +}; + +test_utils_exhaust_memory_rec test_utils_exhaust_memory(uint32_t caps, size_t limit) +{ + int idx = 0; + test_utils_exhaust_memory_rec rec = calloc(1, sizeof(struct test_utils_exhaust_memory_record_s)); + TEST_ASSERT_NOT_NULL_MESSAGE(rec, "test_utils_exhaust_memory: not enough free memory to allocate record structure!"); + + while (idx < EXHAUST_MEMORY_ENTRIES) { + size_t free_caps = heap_caps_get_largest_free_block(caps); + if (free_caps <= limit) { + return rec; // done! + } + rec->entries[idx] = heap_caps_malloc(free_caps - limit, caps); + TEST_ASSERT_NOT_NULL_MESSAGE(rec->entries[idx], + "test_utils_exhaust_memory: something went wrong while freeing up memory, is another task using heap?"); + heap_caps_check_integrity_all(true); + idx++; + } + + TEST_FAIL_MESSAGE("test_utils_exhaust_memory: The heap with the requested caps is too fragmented, increase EXHAUST_MEMORY_ENTRIES or defrag the heap!"); + abort(); +} + +void test_utils_free_exhausted_memory(test_utils_exhaust_memory_rec rec) +{ + for (int i = 0; i < EXHAUST_MEMORY_ENTRIES; i++) { + free(rec->entries[i]); + } + free(rec); +} +