From ecd0143497a10e073470ec7ac38dea851d902412 Mon Sep 17 00:00:00 2001 From: Dmitriy Musatkin <63878209+DmitriyMusatkin@users.noreply.github.com> Date: Wed, 29 Nov 2023 17:12:38 -0500 Subject: [PATCH] Mem limiter validation (#385) Co-authored-by: Dengke Tang --- include/aws/s3/private/s3_buffer_pool.h | 3 ++ include/aws/s3/s3.h | 1 + source/s3.c | 2 + source/s3_buffer_pool.c | 50 ++++++++++++++++++------- source/s3_client.c | 2 +- tests/CMakeLists.txt | 2 + tests/s3_buffer_pool_tests.c | 31 +++++++++++++++ 7 files changed, 76 insertions(+), 15 deletions(-) diff --git a/include/aws/s3/private/s3_buffer_pool.h b/include/aws/s3/private/s3_buffer_pool.h index 431abea75..43d543864 100644 --- a/include/aws/s3/private/s3_buffer_pool.h +++ b/include/aws/s3/private/s3_buffer_pool.h @@ -38,6 +38,9 @@ struct aws_s3_buffer_pool_usage_stats { * buffer reserved for overhead of the pool */ size_t mem_limit; + /* Max size of buffer to be allocated from primary. */ + size_t primary_cutoff; + /* How much mem is used in primary storage. includes memory used by blocks * that are waiting on all allocs to release before being put back in circulation. */ size_t primary_used; diff --git a/include/aws/s3/s3.h b/include/aws/s3/s3.h index c931f1b15..68015d392 100644 --- a/include/aws/s3/s3.h +++ b/include/aws/s3/s3.h @@ -42,6 +42,7 @@ enum aws_s3_errors { AWS_ERROR_S3_REQUEST_TIME_TOO_SKEWED, AWS_ERROR_S3_FILE_MODIFIED, AWS_ERROR_S3_EXCEEDS_MEMORY_LIMIT, + AWS_ERROR_S3_INVALID_MEMORY_LIMIT_CONFIG, AWS_ERROR_S3_END_RANGE = AWS_ERROR_ENUM_END_RANGE(AWS_C_S3_PACKAGE_ID) }; diff --git a/source/s3.c b/source/s3.c index 4e8dd8347..44cc8bb00 100644 --- a/source/s3.c +++ b/source/s3.c @@ -42,6 +42,8 @@ static struct aws_error_info s_errors[] = { AWS_DEFINE_ERROR_INFO_S3(AWS_ERROR_S3_REQUEST_TIME_TOO_SKEWED, "RequestTimeTooSkewed error received from S3."), AWS_DEFINE_ERROR_INFO_S3(AWS_ERROR_S3_FILE_MODIFIED, "The file was modified during upload."), AWS_DEFINE_ERROR_INFO_S3(AWS_ERROR_S3_EXCEEDS_MEMORY_LIMIT, "Request was not created due to used memory exceeding memory limit."), + AWS_DEFINE_ERROR_INFO_S3(AWS_ERROR_S3_INVALID_MEMORY_LIMIT_CONFIG, "Specified memory configuration is invalid for the system. " + "Memory limit should be at least 1GiB. Part size and max part size should be smaller than memory limit."), }; /* clang-format on */ diff --git a/source/s3_buffer_pool.c b/source/s3_buffer_pool.c index 2706c3267..fe62fe041 100644 --- a/source/s3_buffer_pool.c +++ b/source/s3_buffer_pool.c @@ -50,8 +50,19 @@ static size_t s_block_list_initial_capacity = 5; * client as well as any allocations overruns due to memory waste in the pool. */ static const size_t s_buffer_pool_reserved_mem = MB_TO_BYTES(128); +/* + * How many chunks make up a block in primary storage. + */ static const size_t s_chunks_per_block = 16; +/* + * Max size of chunks in primary. + * Effectively if client part size is above the following number, primary + * storage along with buffer reuse is disabled and all buffers are allocated + * directly using allocator. + */ +static const size_t s_max_chunk_size_for_buffer_reuse = MB_TO_BYTES(64); + struct aws_s3_buffer_pool { struct aws_allocator *base_allocator; struct aws_mutex mutex; @@ -124,29 +135,39 @@ struct aws_s3_buffer_pool *aws_s3_buffer_pool_new( if (mem_limit < GB_TO_BYTES(1)) { AWS_LOGF_ERROR( - AWS_LS_S3_CLIENT, "Failed to initialize buffer pool. Min supported value for Memory Limit is 1GB."); - aws_raise_error(AWS_ERROR_INVALID_ARGUMENT); + AWS_LS_S3_CLIENT, + "Failed to initialize buffer pool. " + "Minimum supported value for Memory Limit is 1GB."); + aws_raise_error(AWS_ERROR_S3_INVALID_MEMORY_LIMIT_CONFIG); return NULL; } - if (!(chunk_size == 0 || (chunk_size > (1024) && chunk_size % 1024 == 0))) { - AWS_LOGF_ERROR( + if (chunk_size < (1024) || chunk_size % (4 * 1024) != 0) { + AWS_LOGF_WARN( AWS_LS_S3_CLIENT, - "Failed to initialize buffer pool. Chunk size must be either 0 or more than 1 KB and size must be 1 KB " - "aligned."); - aws_raise_error(AWS_ERROR_INVALID_ARGUMENT); - return NULL; + "Part size specified on the client can lead to suboptimal performance. " + "Consider specifying size in multiples of 4KiB. Ideal part size for most transfers is " + "1MiB multiple between 8MiB and 16MiB. Note: the client will automatically scale part size " + "if its not sufficient to transfer data within the maximum number of parts"); } size_t adjusted_mem_lim = mem_limit - s_buffer_pool_reserved_mem; - if (chunk_size * s_chunks_per_block > adjusted_mem_lim) { - AWS_LOGF_ERROR( + /* + * TODO: There is several things we can consider tweaking here: + * - if chunk size is a weird number of bytes, force it to the closest page size? + * - grow chunk size max based on overall mem lim (ex. for 4gb it might be + * 64mb, but for 8gb it can be 128mb) + * - align chunk size to better fill available mem? some chunk sizes can + * result in memory being wasted because overall limit does not divide + * nicely into chunks + */ + if (chunk_size > s_max_chunk_size_for_buffer_reuse || chunk_size * s_chunks_per_block > adjusted_mem_lim) { + AWS_LOGF_WARN( AWS_LS_S3_CLIENT, - "Failed to initialize buffer pool. Chunk size is too large for the memory limit. " - "Consider adjusting memory limit or part size."); - aws_raise_error(AWS_ERROR_INVALID_ARGUMENT); - return NULL; + "Part size specified on the client is too large for automatic buffer reuse. " + "Consider specifying a smaller part size to improve performance and memory utilization"); + chunk_size = 0; } struct aws_s3_buffer_pool *buffer_pool = aws_mem_calloc(allocator, 1, sizeof(struct aws_s3_buffer_pool)); @@ -405,6 +426,7 @@ struct aws_s3_buffer_pool_usage_stats aws_s3_buffer_pool_get_usage(struct aws_s3 struct aws_s3_buffer_pool_usage_stats ret = (struct aws_s3_buffer_pool_usage_stats){ .mem_limit = buffer_pool->mem_limit, + .primary_cutoff = buffer_pool->primary_size_cutoff, .primary_allocated = buffer_pool->primary_allocated, .primary_used = buffer_pool->primary_used, .primary_reserved = buffer_pool->primary_reserved, diff --git a/source/s3_client.c b/source/s3_client.c index 553402752..cba9cf047 100644 --- a/source/s3_client.c +++ b/source/s3_client.c @@ -365,7 +365,7 @@ struct aws_s3_client *aws_s3_client_new( AWS_LS_S3_CLIENT, "Cannot create client from client_config; configured max part size should not exceed memory limit." "size."); - aws_raise_error(AWS_ERROR_INVALID_ARGUMENT); + aws_raise_error(AWS_ERROR_S3_INVALID_MEMORY_LIMIT_CONFIG); goto on_early_fail; } diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 83645f341..19123aa46 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -315,9 +315,11 @@ add_test_case(parallel_read_stream_from_file_sanity_test) add_test_case(parallel_read_stream_from_large_file_test) add_test_case(test_s3_buffer_pool_threaded_allocs_and_frees) +add_test_case(test_s3_buffer_pool_large_chunk_threaded_allocs_and_frees) add_test_case(test_s3_buffer_pool_limits) add_test_case(test_s3_buffer_pool_trim) add_test_case(test_s3_buffer_pool_reservation_hold) +add_test_case(test_s3_buffer_pool_too_small) add_net_test_case(test_s3_put_object_buffer_pool_trim) add_net_test_case(client_update_upload_part_timeout) diff --git a/tests/s3_buffer_pool_tests.c b/tests/s3_buffer_pool_tests.c index 9b4cad64d..40c6cb288 100644 --- a/tests/s3_buffer_pool_tests.c +++ b/tests/s3_buffer_pool_tests.c @@ -73,6 +73,25 @@ static int s_test_s3_buffer_pool_threaded_allocs_and_frees(struct aws_allocator } AWS_TEST_CASE(test_s3_buffer_pool_threaded_allocs_and_frees, s_test_s3_buffer_pool_threaded_allocs_and_frees) +static int s_test_s3_buffer_pool_large_chunk_threaded_allocs_and_frees(struct aws_allocator *allocator, void *ctx) { + (void)allocator; + (void)ctx; + + struct aws_s3_buffer_pool *buffer_pool = aws_s3_buffer_pool_new(allocator, MB_TO_BYTES(65), GB_TO_BYTES(2)); + + struct aws_s3_buffer_pool_usage_stats stats = aws_s3_buffer_pool_get_usage(buffer_pool); + ASSERT_INT_EQUALS(0, stats.primary_cutoff); + + s_thread_test(allocator, s_threaded_alloc_worker, buffer_pool); + + aws_s3_buffer_pool_destroy(buffer_pool); + + return 0; +} +AWS_TEST_CASE( + test_s3_buffer_pool_large_chunk_threaded_allocs_and_frees, + s_test_s3_buffer_pool_large_chunk_threaded_allocs_and_frees) + static int s_test_s3_buffer_pool_limits(struct aws_allocator *allocator, void *ctx) { (void)allocator; (void)ctx; @@ -186,3 +205,15 @@ static int s_test_s3_buffer_pool_reservation_hold(struct aws_allocator *allocato return 0; }; AWS_TEST_CASE(test_s3_buffer_pool_reservation_hold, s_test_s3_buffer_pool_reservation_hold) + +static int s_test_s3_buffer_pool_too_small(struct aws_allocator *allocator, void *ctx) { + (void)allocator; + (void)ctx; + + struct aws_s3_buffer_pool *buffer_pool = aws_s3_buffer_pool_new(allocator, MB_TO_BYTES(8), MB_TO_BYTES(512)); + ASSERT_NULL(buffer_pool); + ASSERT_INT_EQUALS(AWS_ERROR_S3_INVALID_MEMORY_LIMIT_CONFIG, aws_last_error()); + + return 0; +}; +AWS_TEST_CASE(test_s3_buffer_pool_too_small, s_test_s3_buffer_pool_too_small)