INTERNAL: Fix the invalid allowable range of run options #732

ing-eoking · 2024-01-25T03:02:17Z

memcached 의 몇몇 구동옵션에서 허용되지 않는 수(음수)를 줄 수 있는 현상을 수정 또는 관련 로그 메시지를 수정합니다.

추가적으로 Memory Limit(maxbytes)옵션인 -m 옵션에서 0을 허용하고 있어,
slabs 모듈 확인 결과 할당 과정에서 아래의 로직을 확인할 수 있었습니다.

if ((slabsp->mem_limit && slabsp->mem_malloced + len > slabsp->mem_limit && p->slabs >= p->rsvd_slabs) ||
    (grow_slab_list(id) == 0) ||
    ((ptr = memory_allocate((size_t)len)) == 0)) {

    MEMCACHED_SLABS_SLABCLASS_ALLOCATE_FAILED(id);
    return 0;
}

mem_limit(maxbytes)를 0으로 설정한 경우는 메모리에 대한 제한을 두지 않기 위한 용도인가요?

uhm0311 · 2024-01-25T03:05:54Z

memcached.c

-                    " greater than 0 and less than memlimit.\n");
+                    "The value of sticky(gummed) memory limit"
+                    " cannot be less than 0"
+                    " and must be greater than memlimit.\n");


The value of memory limit ... must be greater than sticky_limit.
The value of sticky(gummed) memory limit ... must be greater than memlimit.

둘 다 서로 자기가 더 높은 값이어야 한다고 되어 있는데요. 어느 쪽이 더 높은 값이어야 하나요?

jhpark816 · 2024-01-25T03:20:20Z

@ing-eoking
memlimit == 0은 허용하지 않아야 할 것 같습니다. 이를 unlimited 의미로 구현하지 않았기 때문입니다.

jhpark816

리뷰 완료

jhpark816 · 2024-01-25T03:27:10Z

memcached.c

@@ -15182,8 +15182,9 @@ int main (int argc, char **argv)
            settings.maxbytes = (size_t)cache_memory_limit * 1024 * 1024;
            if (cache_memory_limit < 0 || settings.maxbytes < settings.sticky_limit) {
                mc_logger->log(EXTENSION_LOG_WARNING, NULL,
-                    "The value of memory limit must be"
-                    " greater than 0 and sticky_limit.\n");


cache_memory_limit <= 0 조건으로 비교하고,
cache_memory_limit와 sticky_memory_limit 간의 validataion check는
옵션 파싱이 끝난 다음에 수행해야 할 것 같습니다.

이럴게 하자는 의미입니다.

if (cache_memory_limit <= 0) { mc_logger->log(EXTENSION_LOG_WARNING, NULL, "The value of memory limit must be greater than 0.\n"); return 1; }

jhpark816 · 2024-01-25T03:28:34Z

memcached.c

@@ -15199,8 +15200,9 @@ int main (int argc, char **argv)
            settings.sticky_limit = (size_t)sticky_memory_limit * 1024 * 1024;
            if (sticky_memory_limit < 0 || settings.sticky_limit > settings.maxbytes) {
                mc_logger->log(EXTENSION_LOG_WARNING, NULL,
-                    "The value of sticky(gummed) memory limit must be"
-                    " greater than 0 and less than memlimit.\n");


여기서도 sticky_memory_limit <= 0 이어야 할 것 같습니다.
0은 default value로 sticky 허용하지 않음을 의미합니다.
0으로 설정하는 것도 의미가 없기 때문에 <= 0 조건을 사용하는 것이 좋겠습니다.

여기도 마찬가지입니다.

ing-eoking · 2024-01-25T04:12:36Z

수정되었습니다.

uhm0311 · 2024-01-25T04:17:38Z

memcached.c

+        settings.maxbytes < settings.sticky_limit) {
+        mc_logger->log(EXTENSION_LOG_WARNING, NULL,
+            "The value of memory limit"
+            " and must be greater than 0 and sticky_limit.\n");


The value of memory limit and must be greater than 0 and sticky_limit.
문장이 이상해 보입니다.

ing-eoking · 2024-01-25T04:25:31Z

  if (settings.maxbytes <= 0 || settings.sticky_limit <= 0 ||
         settings.maxbytes < settings.sticky_limit)

3 조건을 포함하고 있는 메시지로 변경해야할 것 같습니다.
maxbytes도 0보다 크고, sticky_limit도 0보다 크고 maxbytes는 sticky_limit 보다 크다.

이러면 문장이 조금 길어지는데, 수식으로 표현할까요?

jhpark816

리뷰 완료

jhpark816 · 2024-01-25T04:26:18Z

memcached.c

@@ -15182,8 +15182,9 @@ int main (int argc, char **argv)
            settings.maxbytes = (size_t)cache_memory_limit * 1024 * 1024;
            if (cache_memory_limit < 0 || settings.maxbytes < settings.sticky_limit) {
                mc_logger->log(EXTENSION_LOG_WARNING, NULL,
-                    "The value of memory limit must be"
-                    " greater than 0 and sticky_limit.\n");


이럴게 하자는 의미입니다.

if (cache_memory_limit <= 0) { mc_logger->log(EXTENSION_LOG_WARNING, NULL, "The value of memory limit must be greater than 0.\n"); return 1; }

jhpark816 · 2024-01-25T04:26:37Z

memcached.c

@@ -15199,8 +15200,9 @@ int main (int argc, char **argv)
            settings.sticky_limit = (size_t)sticky_memory_limit * 1024 * 1024;
            if (sticky_memory_limit < 0 || settings.sticky_limit > settings.maxbytes) {
                mc_logger->log(EXTENSION_LOG_WARNING, NULL,
-                    "The value of sticky(gummed) memory limit must be"
-                    " greater than 0 and less than memlimit.\n");


여기도 마찬가지입니다.

jhpark816 · 2024-01-25T04:29:16Z

memcached.c

+            "The value of memory limit must be"
+            " greater than 0 and sticky_limit.\n");
+        return 1;
+    }


여기는 아래와 같이 수정하자는 얘기입니다.

if (settings.maxbytes < settings.sticky_limit) { mc_logger->log(EXTENSION_LOG_WARNING, NULL, "The value of memory limit cannot be smaller than sticky_limit.\n"); return 1; }

settings.sticky_limit == 0은 가능하며, 정상적인 값입니다.

기존 memcached는 memlimit = 0 값을 허용하였는 지를 확인 바랍니다.

memcached에서 아래와 같이 구동했을 때, 간단한 K/V에서 문제없었습니다.

./memcached -E .libs/default_engine.so -p 13579 -m 0

settings.sticky_limit == 0은 가능하며, 정상적인 값입니다.

CI 테스트 실패함을 확인했습니다. 위의 방식으로 수정하겠습니다.

jhpark816 · 2024-01-25T05:28:57Z

commit message도 아래와 같이 수정하시죠.

INTERNAL: Fix the invalid allowable range of run options

ing-eoking · 2024-01-25T06:01:23Z

오픈소스 memcached의 memlimit 허용 범위에 대하여

오픈소스 memcached에 memlimit을 0으로 설정하여 구동할 경우 아래와 같은 에러가 출력됩니다.

./memcached -m 0
Cannot set item size limit higher than 1/2 of memory max.

오픈소스 memcached 에서는 memlimit의 크기가 Max Item Size 보다 2배 이상 설정하게끔 구현되어 있습니다.
즉, memlimit을 최소 2048 bytes 이상의 크기를 지정해야 합니다.

if (settings.item_size_max < ITEM_SIZE_MAX_LOWER_LIMIT) { // ITEM_SIZE_MAX_LOWER_LIMIT = 1024
    fprintf(stderr, "Item max size cannot be less than 1024 bytes.\n");
    exit(EX_USAGE);
}
if (settings.item_size_max > (settings.maxbytes / 2)) {
    fprintf(stderr, "Cannot set item size limit higher than 1/2 of memory max.\n");
    exit(EX_USAGE);
}

ing-eoking requested review from jhpark816, namsic and uhm0311 January 25, 2024 03:02

uhm0311 reviewed Jan 25, 2024

View reviewed changes

jhpark816 reviewed Jan 25, 2024

View reviewed changes

ing-eoking force-pushed the option branch from 3d98c3f to de1bc85 Compare January 25, 2024 04:10

uhm0311 reviewed Jan 25, 2024

View reviewed changes

ing-eoking force-pushed the option branch 2 times, most recently from 4686452 to 40c27b7 Compare January 25, 2024 04:21

jhpark816 reviewed Jan 25, 2024

View reviewed changes

ing-eoking force-pushed the option branch 4 times, most recently from 8ee948b to dee614f Compare January 25, 2024 05:08

INTERNAL: Fix the invalid allowable range of run options

82a689d

ing-eoking force-pushed the option branch from dee614f to 82a689d Compare January 25, 2024 06:02

ing-eoking changed the title ~~INTERNAL: Fix the wrong premitted range of run options~~ INTERNAL: Fix the invalid allowable range of run options Jan 25, 2024

jhpark816 merged commit dddc8e4 into naver:develop Jan 25, 2024
1 check passed

ing-eoking deleted the option branch January 25, 2024 09:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

INTERNAL: Fix the invalid allowable range of run options #732

INTERNAL: Fix the invalid allowable range of run options #732

ing-eoking commented Jan 25, 2024

uhm0311 Jan 25, 2024 •

edited

Loading

jhpark816 commented Jan 25, 2024

jhpark816 left a comment

jhpark816 Jan 25, 2024

jhpark816 Jan 25, 2024

jhpark816 Jan 25, 2024

jhpark816 Jan 25, 2024

ing-eoking commented Jan 25, 2024

uhm0311 Jan 25, 2024

ing-eoking commented Jan 25, 2024 •

edited

Loading

jhpark816 left a comment

jhpark816 Jan 25, 2024

jhpark816 Jan 25, 2024

jhpark816 Jan 25, 2024

jhpark816 Jan 25, 2024

jhpark816 Jan 25, 2024

ing-eoking Jan 25, 2024 •

edited

Loading

jhpark816 commented Jan 25, 2024

ing-eoking commented Jan 25, 2024 •

edited

Loading

INTERNAL: Fix the invalid allowable range of run options #732

INTERNAL: Fix the invalid allowable range of run options #732

Conversation

ing-eoking commented Jan 25, 2024

uhm0311 Jan 25, 2024 • edited Loading

Choose a reason for hiding this comment

jhpark816 commented Jan 25, 2024

jhpark816 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ing-eoking commented Jan 25, 2024

Choose a reason for hiding this comment

ing-eoking commented Jan 25, 2024 • edited Loading

jhpark816 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ing-eoking Jan 25, 2024 • edited Loading

Choose a reason for hiding this comment

jhpark816 commented Jan 25, 2024

ing-eoking commented Jan 25, 2024 • edited Loading

오픈소스 memcached의 memlimit 허용 범위에 대하여

uhm0311 Jan 25, 2024 •

edited

Loading

ing-eoking commented Jan 25, 2024 •

edited

Loading

ing-eoking Jan 25, 2024 •

edited

Loading

ing-eoking commented Jan 25, 2024 •

edited

Loading