-
Notifications
You must be signed in to change notification settings - Fork 955
Add maxmemory-reserved parameter for evicting key earlier to avoid OOM #831
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: unstable
Are you sure you want to change the base?
Changes from all commits
279efc5
8581a7e
a2d4d8f
fb8adf9
ad389c8
6d4d8b6
f2f1e9d
acac020
2603e0b
62784f7
67231ba
355e253
5348e90
1cc8943
8cb5d20
9c21792
46e0f2c
6542eff
c8e217f
f891244
28be0fa
41780a1
4a803c4
4be3b3c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2496,18 +2496,33 @@ static int updateReplBacklogSize(const char **err) { | |
| return 1; | ||
| } | ||
|
|
||
| static int updateMaxmemoryReserved(const char **err) { | ||
| if (server.maxmemory) { | ||
| if (isMaxmemoryReservedLessThanMaxmemory(err) == C_ERR) { | ||
| return 0; | ||
| } | ||
| calculateKeyEvictionMemory(); | ||
| startEvictionTimeProc(); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this should be controlled by
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is one case: we set maxmemory and then increase the maxmemory-reserved value, the used memory value just exceed the maxmemory-reserved threshold. By calling this function, the key eviction could be triggered earlier.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it is confusing to trigger the key eviction cron job when updating |
||
| } else { | ||
| if (server.maxmemory_reserved) { | ||
| *err = "Current maxmemory value is 0, the new reserved memory is invalid"; | ||
| return 0; | ||
| } | ||
| server.key_eviction_memory = 0; | ||
| } | ||
| return 1; | ||
| } | ||
|
|
||
| static int updateMaxmemory(const char **err) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The logic in this function is also very similar to
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The logic seems slightly different, but I think they should have the same logic. I don't like to have configs that throw errors based off of other configs. So if you said reserved > maxmemory, then it should just clamp it to zero and evict everything. (Basically the same behavior as if you set maxmemory to zero) |
||
| UNUSED(err); | ||
| if (server.maxmemory) { | ||
| size_t used = zmalloc_used_memory() - freeMemoryGetNotCountedMemory(); | ||
| if (server.maxmemory < used) { | ||
| serverLog(LL_WARNING, | ||
| "WARNING: the new maxmemory value set via CONFIG SET (%llu) is smaller than the current memory " | ||
| "usage (%zu). This will result in key eviction and/or the inability to accept new write commands " | ||
| "depending on the maxmemory-policy.", | ||
| server.maxmemory, used); | ||
| if (isMaxmemoryReservedLessThanMaxmemory(err) == C_ERR) { | ||
| return 0; | ||
| } | ||
| calculateKeyEvictionMemory(); | ||
| startEvictionTimeProc(); | ||
| } else { | ||
| server.maxmemory_reserved = 0; | ||
| server.key_eviction_memory = 0; | ||
| } | ||
| return 1; | ||
| } | ||
|
|
@@ -3329,6 +3344,7 @@ standardConfig static_configs[] = { | |
|
|
||
| /* Unsigned Long Long configs */ | ||
| createULongLongConfig("maxmemory", NULL, MODIFIABLE_CONFIG, 0, ULLONG_MAX, server.maxmemory, 0, MEMORY_CONFIG, NULL, updateMaxmemory), | ||
| createULongLongConfig("maxmemory-reserved", NULL, MODIFIABLE_CONFIG, 0, ULLONG_MAX, server.maxmemory_reserved, 0, MEMORY_CONFIG, NULL, updateMaxmemoryReserved), | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it would be appreciate if we support both percentage and absolute values, just like
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we said we were going to punt the percent to a later point. If we make the config a LLONG_MAX, then we can support negative values in the future. So Maybe let's just do that?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Have we used negative values for percentage already? I vaguely remember we did but not 100% sure. Also have we used the negative value hack for things other percentage? I am good either way (adding percentage support or not). |
||
| createULongLongConfig("cluster-link-sendbuf-limit", NULL, MODIFIABLE_CONFIG, 0, ULLONG_MAX, server.cluster_link_msg_queue_limit_bytes, 0, MEMORY_CONFIG, NULL, NULL), | ||
|
|
||
| /* Size_t configs */ | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -377,7 +377,7 @@ size_t freeMemoryGetNotCountedMemory(void) { | |||||||||||||
| * limit. | ||||||||||||||
| * (Populated both for C_ERR and C_OK) | ||||||||||||||
| */ | ||||||||||||||
| int getMaxmemoryState(size_t *total, size_t *logical, size_t *tofree, float *level) { | ||||||||||||||
| int getMaxmemoryState(size_t *total, size_t *logical, size_t *tofree, float *level, unsigned long long maxmemory) { | ||||||||||||||
| size_t mem_reported, mem_used, mem_tofree; | ||||||||||||||
|
|
||||||||||||||
| /* Check if we are over the memory usage limit. If we are not, no need | ||||||||||||||
|
|
@@ -386,11 +386,12 @@ int getMaxmemoryState(size_t *total, size_t *logical, size_t *tofree, float *lev | |||||||||||||
| if (total) *total = mem_reported; | ||||||||||||||
|
|
||||||||||||||
| /* We may return ASAP if there is no need to compute the level. */ | ||||||||||||||
| if (!server.maxmemory) { | ||||||||||||||
| if (!maxmemory) { | ||||||||||||||
| if (level) *level = 0; | ||||||||||||||
| return C_OK; | ||||||||||||||
| } | ||||||||||||||
| if (mem_reported <= server.maxmemory && !level) return C_OK; | ||||||||||||||
|
|
||||||||||||||
| if (mem_reported <= maxmemory && !level) return C_OK; | ||||||||||||||
|
|
||||||||||||||
| /* Remove the size of replicas output buffers and AOF buffer from the | ||||||||||||||
| * count of used memory. */ | ||||||||||||||
|
|
@@ -399,15 +400,19 @@ int getMaxmemoryState(size_t *total, size_t *logical, size_t *tofree, float *lev | |||||||||||||
| mem_used = (mem_used > overhead) ? mem_used - overhead : 0; | ||||||||||||||
|
|
||||||||||||||
| /* Compute the ratio of memory usage. */ | ||||||||||||||
| if (level) *level = (float)mem_used / (float)server.maxmemory; | ||||||||||||||
| if (level) { | ||||||||||||||
| *level = (float)mem_used / (float)server.maxmemory; | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| if (mem_reported <= server.maxmemory) return C_OK; | ||||||||||||||
| if (mem_reported <= maxmemory) return C_OK; | ||||||||||||||
|
|
||||||||||||||
| /* Check if we are still over the memory limit. */ | ||||||||||||||
| if (mem_used <= server.maxmemory) return C_OK; | ||||||||||||||
| /* if function parameter 'maxmemory' is equal to maxmemory and mem_used > maxmemory then OOM / | ||||||||||||||
| / if function parameter 'maxmemory' is equal to maxmemory_soft and mem_used > function parameter 'maxmemory' then there is no OOM but eviction happens */ | ||||||||||||||
| if (mem_used <= maxmemory) return C_OK; | ||||||||||||||
|
|
||||||||||||||
| /* Compute how much memory we need to free. */ | ||||||||||||||
| mem_tofree = mem_used - server.maxmemory; | ||||||||||||||
| mem_tofree = mem_used - server.key_eviction_memory; | ||||||||||||||
|
|
||||||||||||||
| if (logical) *logical = mem_used; | ||||||||||||||
| if (tofree) *tofree = mem_tofree; | ||||||||||||||
|
|
@@ -522,20 +527,24 @@ int performEvictions(void) { | |||||||||||||
| if (!isSafeToPerformEvictions()) return EVICT_OK; | ||||||||||||||
|
|
||||||||||||||
| int keys_freed = 0; | ||||||||||||||
| size_t mem_reported, mem_tofree; | ||||||||||||||
| size_t mem_reported, mem_tofree, mem_used; | ||||||||||||||
| long long mem_freed = 0; /* Maybe become negative */ | ||||||||||||||
| mstime_t latency, eviction_latency; | ||||||||||||||
| long long delta; | ||||||||||||||
| int replicas = listLength(server.replicas); | ||||||||||||||
| int result = EVICT_FAIL; | ||||||||||||||
|
|
||||||||||||||
| if (getMaxmemoryState(&mem_reported, NULL, &mem_tofree, NULL) == C_OK) { | ||||||||||||||
| if (getMaxmemoryState(&mem_reported, &mem_used, &mem_tofree, NULL, server.key_eviction_memory) == C_OK) { | ||||||||||||||
| result = EVICT_OK; | ||||||||||||||
| goto update_metrics; | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| if (server.maxmemory_policy == MAXMEMORY_NO_EVICTION || (iAmPrimary() && server.import_mode)) { | ||||||||||||||
| result = EVICT_FAIL; /* We need to free memory, but policy forbids or we are in import mode. */ | ||||||||||||||
| if (mem_used >= server.maxmemory) { | ||||||||||||||
| result = EVICT_FAIL; /* We need to free memory, but policy forbids or we are in import mode. */ | ||||||||||||||
| } else { | ||||||||||||||
| result = EVICT_OK; /* used_memory greater than key_eviction_memory, but not reach OOM */ | ||||||||||||||
| } | ||||||||||||||
| goto update_metrics; | ||||||||||||||
|
Comment on lines
+545
to
548
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We still want to perform eviction when used_memory is greater than key_eviction_memory I think
Suggested change
|
||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
|
|
@@ -697,7 +706,7 @@ int performEvictions(void) { | |||||||||||||
| * across the dbAsyncDelete() call, while the thread can | ||||||||||||||
| * release the memory all the time. */ | ||||||||||||||
| if (server.lazyfree_lazy_eviction) { | ||||||||||||||
| if (getMaxmemoryState(NULL, NULL, NULL, NULL) == C_OK) { | ||||||||||||||
| if (getMaxmemoryState(NULL, NULL, NULL, NULL, server.key_eviction_memory) == C_OK) { | ||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should we update
Suggested change
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we should not change it, otherwise some existing test cases are broken
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see - updating |
||||||||||||||
| break; | ||||||||||||||
| } | ||||||||||||||
| } | ||||||||||||||
|
|
@@ -712,21 +721,23 @@ int performEvictions(void) { | |||||||||||||
| } | ||||||||||||||
| } | ||||||||||||||
| } else { | ||||||||||||||
| goto cant_free; /* nothing to free... */ | ||||||||||||||
| break; | ||||||||||||||
| } | ||||||||||||||
| } | ||||||||||||||
| /* at this point, the memory is OK, or we have reached the time limit */ | ||||||||||||||
| result = (isEvictionProcRunning) ? EVICT_RUNNING : EVICT_OK; | ||||||||||||||
|
|
||||||||||||||
| cant_free: | ||||||||||||||
| if (mem_freed >= (long long)(mem_used - server.key_eviction_memory)) { | ||||||||||||||
| /* at this point, the memory is OK, or we have reached the time limit */ | ||||||||||||||
| result = (isEvictionProcRunning) ? EVICT_RUNNING : EVICT_OK; | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| if (result == EVICT_FAIL) { | ||||||||||||||
| /* At this point, we have run out of evictable items. It's possible | ||||||||||||||
| * that some items are being freed in the lazyfree thread. Perform a | ||||||||||||||
| * short wait here if such jobs exist, but don't wait long. */ | ||||||||||||||
| mstime_t lazyfree_latency; | ||||||||||||||
| latencyStartMonitor(lazyfree_latency); | ||||||||||||||
| while (bioPendingJobsOfType(BIO_LAZY_FREE) && elapsedUs(evictionTimer) < eviction_time_limit_us) { | ||||||||||||||
| if (getMaxmemoryState(NULL, NULL, NULL, NULL) == C_OK) { | ||||||||||||||
| if (getMaxmemoryState(NULL, NULL, NULL, NULL, server.key_eviction_memory) == C_OK) { | ||||||||||||||
| result = EVICT_OK; | ||||||||||||||
| break; | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -2785,6 +2785,17 @@ void initServer(void) { | |||||||||||||||||||||
| server.client_mem_usage_buckets = NULL; | ||||||||||||||||||||||
| resetReplicationBuffer(); | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| if (server.maxmemory) { | ||||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this logic is repeated in |
||||||||||||||||||||||
| const char *err = NULL; | ||||||||||||||||||||||
| if (isMaxmemoryReservedLessThanMaxmemory(&err) == C_ERR) { | ||||||||||||||||||||||
| serverLog(LL_WARNING, "%s", err); | ||||||||||||||||||||||
| exit(1); | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| server.key_eviction_memory = server.maxmemory - server.maxmemory_reserved; | ||||||||||||||||||||||
| } else { | ||||||||||||||||||||||
| server.key_eviction_memory = 0; | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| /* Make sure the locale is set on startup based on the config file. */ | ||||||||||||||||||||||
| if (setlocale(LC_COLLATE, server.locale_collate) == NULL) { | ||||||||||||||||||||||
| serverLog(LL_WARNING, "Failed to configure LOCALE for invalid locale name."); | ||||||||||||||||||||||
|
|
@@ -5717,6 +5728,7 @@ sds genValkeyInfoString(dict *section_dict, int all_sections, int everything) { | |||||||||||||||||||||
| char used_memory_scripts_hmem[64]; | ||||||||||||||||||||||
| char used_memory_rss_hmem[64]; | ||||||||||||||||||||||
| char maxmemory_hmem[64]; | ||||||||||||||||||||||
| char maxmemory_reserved_hmem[64]; | ||||||||||||||||||||||
| size_t zmalloc_used = zmalloc_used_memory(); | ||||||||||||||||||||||
| size_t total_system_mem = server.system_memory_size; | ||||||||||||||||||||||
| const char *evict_policy = evictPolicyToString(); | ||||||||||||||||||||||
|
|
@@ -5738,6 +5750,7 @@ sds genValkeyInfoString(dict *section_dict, int all_sections, int everything) { | |||||||||||||||||||||
| bytesToHuman(used_memory_scripts_hmem, sizeof(used_memory_scripts_hmem), mh->lua_caches + mh->functions_caches); | ||||||||||||||||||||||
| bytesToHuman(used_memory_rss_hmem, sizeof(used_memory_rss_hmem), server.cron_malloc_stats.process_rss); | ||||||||||||||||||||||
| bytesToHuman(maxmemory_hmem, sizeof(maxmemory_hmem), server.maxmemory); | ||||||||||||||||||||||
| bytesToHuman(maxmemory_reserved_hmem, sizeof(maxmemory_reserved_hmem), server.maxmemory_reserved); | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| if (sections++) info = sdscat(info, "\r\n"); | ||||||||||||||||||||||
| info = sdscatprintf( | ||||||||||||||||||||||
|
|
@@ -5776,6 +5789,8 @@ sds genValkeyInfoString(dict *section_dict, int all_sections, int everything) { | |||||||||||||||||||||
| "maxmemory:%lld\r\n", server.maxmemory, | ||||||||||||||||||||||
| "maxmemory_human:%s\r\n", maxmemory_hmem, | ||||||||||||||||||||||
| "maxmemory_policy:%s\r\n", evict_policy, | ||||||||||||||||||||||
| "maxmemory_reserved:%lld\r\n", server.maxmemory_reserved, | ||||||||||||||||||||||
| "maxmemory_reserved_human:%s\r\n", maxmemory_reserved_hmem, | ||||||||||||||||||||||
|
Comment on lines
5789
to
+5793
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
I would like to consider this. from a user perspective, they want to see the actual limit, they don't want to do the math themselves.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I do not like to do the mathematics as well. Agree with you. I will update it after we finalize the naming for this pr. Thanks
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Users still need to do some math to know when eviction happens. The math will be easier for users if we add a field like With a field like this, users can just check this to know how near eviction is. I remember this doc PR and linked issue: valkey-io/valkey-doc#194. It's the problem for users to understand this math. |
||||||||||||||||||||||
| "allocator_frag_ratio:%.2f\r\n", mh->allocator_frag, | ||||||||||||||||||||||
| "allocator_frag_bytes:%zu\r\n", mh->allocator_frag_bytes, | ||||||||||||||||||||||
| "allocator_rss_ratio:%.2f\r\n", mh->allocator_rss, | ||||||||||||||||||||||
|
|
@@ -6761,6 +6776,23 @@ int validateProcTitleTemplate(const char *template) { | |||||||||||||||||||||
| return ok; | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| int isMaxmemoryReservedLessThanMaxmemory(const char **err) { | ||||||||||||||||||||||
| if (server.maxmemory <= server.maxmemory_reserved) { | ||||||||||||||||||||||
| *err = "The maxmemory reserved value should be smaller than maxmemory."; | ||||||||||||||||||||||
| return C_ERR; | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| return C_OK; | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| void calculateKeyEvictionMemory(void) { | ||||||||||||||||||||||
| server.key_eviction_memory = server.maxmemory - server.maxmemory_reserved; | ||||||||||||||||||||||
|
Comment on lines
+6787
to
+6788
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I do not think so. Here, the server.maxmemory and server.maxmemory_reserved are fixed values if nobody update them by config set command. Thus, I think calculating server.key_eviction_memory once and work as a global variable is better. |
||||||||||||||||||||||
| size_t used = zmalloc_used_memory() - freeMemoryGetNotCountedMemory(); | ||||||||||||||||||||||
| if (server.key_eviction_memory < used) { | ||||||||||||||||||||||
| serverLog(LL_WARNING, "WARNING: the difference between memory usage and maxmemory is less than reserved memory. " | ||||||||||||||||||||||
| "This will result in key eviction depending on the maxmemory-policy. But server can still accept new write commands."); | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| int serverSetProcTitle(char *title) { | ||||||||||||||||||||||
| #ifdef USE_SETPROCTITLE | ||||||||||||||||||||||
| if (!title) title = server.exec_argv[0]; | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -1279,6 +1279,13 @@ acllog-max-len 128 | |||||
| # | ||||||
| # maxmemory-policy noeviction | ||||||
|
|
||||||
| # `maxmemory-reserved` specifies a fixed amount of memory set aside from `maxmemory`. | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am good with changing
Suggested change
|
||||||
| # When the available memory, the difference between memory usage and `maxmemory`, falls | ||||||
| # below this threshold, proactive key eviction is triggered. However, this does not immediately | ||||||
| # result in write commands being rejected; only reaching the `maxmemory` limit will do so. | ||||||
| # | ||||||
| # maxmemory-reserved <bytes> | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For most of the configs, there is a real example here, usually with the default value. I think it's better. We can mention in the text above that it's in bytes and that the default is 0 to be extra clear.
Suggested change
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here is aligned with maxmemory |
||||||
|
|
||||||
| # LRU, LFU and minimal TTL algorithms are not precise algorithms but approximated | ||||||
| # algorithms (in order to save memory), so you can tune it for speed or | ||||||
| # accuracy. By default the server will check five keys and pick the one that was | ||||||
|
|
||||||
Uh oh!
There was an error while loading. Please reload this page.