INTERNAL: Remove the check for clock_gettime function #789

ing-eoking · 2024-10-23T00:58:21Z

🔗 Related Issue

jam2in/arcus-works#590

⌨️ What I did

기존 코드에서는 clock_gettime 사용을 위해 librt에서 확인을 수행했지만, 링킹 옵션인 -lrt는 따로 지정되지 않았습니다.
확인 후 -lrt옵션을 주도록 추가하도록 변경합니다.

ing-eoking · 2024-10-29T07:48:18Z

configure.ac

@@ -23,8 +23,10 @@ dnl if clock_gettime is available, define USE_CLOCK_GETTIME 1
 dnl else define USE_GETTIMEOFDAY 1
 dnl **********************************************************************
 AC_CHECK_LIB(rt, clock_gettime,
+        CLOCK_GETTIME_LIBS="-lrt"


AC_CHECK_LIB ($LIB, $FUNC, [$ACTION1], [$ACTION2])

$LIB에서 $FUNC라는 함수가 존재하는지 확인하는 매크로입니다.

만약 함수가 존재하면 $ACTION1을 수행하고, 존재하지 않으면 $ACTION2를 수행합니다.

현재 PR에서 $ACTION1에 해당하는 코드는 다음과 같습니다:

CLOCK_GETTIME_LIBS="-lrt" AC_DEFINE(USE_CLOCK_GETTIME, 1, [Defined if clock_gettime is available in libc])

이 코드는 CLOCK_GETTIME_LIBS 변수에 "-lrt" 문자열을 저장하고,
USE_CLOCK_GETTIME이라는 전처리기를 config.h에 정의합니다.

궁금한 점이 있습니다.
EE 서버 코드를 보면 clock_gettime() 함수를 사용하는 곳이 있습니다.
기존에 --enable-zk-integration 없이 컴파일한 경우에 해당 코드들이 정상 동작하나요?
혹은 ZK가 없으면 실행되지 않는 코드라서 문제가 없었던 것인가요?

zk 링킹 여부 관계없이 clock_gettime 또는 gettimeofday 를 쓰도록 되게끔 구현되어 있어야 합니다.
만약 아래 처럼 구현되어 있다면 맞게 구현된 것 입니다.

#ifdef USE_CLOCK_GETTIME clock_gettime(...); #else gettimeofday(...); #endif

clock_gettime() 매뉴얼을 보면, 아래와 같이
glibc 2.7 이전 버전이면 librt와 linking하라고 되어 있는 데요.
glibc 2.7 부터는 glibc 라이브러리에서 clock_gettime() 지원하는가요?

#include <time.h> int clock_gettime(clockid_t clk_id, struct timespec *tp); //Link with -lrt (only for glibc version before 2.17)

참고로, USE_CLOCK_GETTIME 사용하는 코드는 EE 버전에만 있습니다.
그리고, USE_GETTIMEOFDAY는 아예 사용되지 않는 define 입니다.

지금 수정하려는 문제가 clock_gettime() 유무와 관련있는 것이 맞나요?

정확하게는 aio_XXX 만 사용하지 못하는 것만 문제가 아닙니다.

rt 라이브러리에 제공하는 API를 가져다 쓰는 상황에서 -lrt 옵션이 없을 시
발생할 수 있는 문제입니다.

아래 상황에서 -lrt 포함하지 않고 빌드해도 문제가 없나요?

libc 최신 버전 사용하여 -lrt 없이도 clock_gettime() 사용 가능한 경우

문제 없습니다.
반대의 libc 구 버전을 사용 중인 상황에서 rt에서 clock_gettime을 찾은 경우 문제가 발생합니다.

clock_gettime() 발견하지 못하여 USE_GETTIMEOFDAY 설정되는 경우

문제 없습니다.

aio_xxx() 사용하기 위해서는 조건부가 아니라 무조건 -lrt 포함해야 하는 것이 아닌지?

무조건 -lrt 옵션을 주는 것은 좋지 않다고 봅니다. rt 라이브러리가 없는 환경이 있을 수도 있습니다.

ARCUS 캐시 서버는 공식적으로 CentOS를 지원합니다.
MacOS 관련 컴파일 문제를 해결했던 이유는 단지 로컬 개발 편의성을 위함입니다.
따라서 이는 -librt를 aio_xxx() 함수가 없어도 포함하는 이유가 될 수 없습니다.

공식으로 지원하는 OS에 대하여 크게 기준으로 삼은 적은 없어서 이 부분은 좀 더 고려해보긴 하겠습니다.

우선 말씀에 대한 저의 의견은 아래와 같습니다.

MacOS에서 문제가 발생했으니 librt 검사를 통해 aio_xxx를 넣지 않았다는 것은 아닙니다. 모든 시스템에서 해당되지 않을 수도 있어서 librt 검사를 포함하지 않은 이유 중 하나가 된 것입니다.

또한, 공식적으로 CentOS와 Ubuntu에서 사용 가능하다고 명시된 것은 맞지만, 그 시스템만을 지원하기 위해 개발이 진행된 것은 아닌 것으로 알고 있습니다. 만약 CentOS 지원만을 목표로 했다면, 다른 리눅스 시스템과의 호환성을 고려하지 않고 memcached 코드가 구현되었을 것입니다.

이는 본 PR의 의도와는 다른 문제입니다.
이 문제를 해결하기 위한 변경 사항으로 인해 이슈가 해결될 수는 있지만, 이슈를 해결하기 위해 현재 변경사항을 반영한다는 것은 의미상 적합하지 않습니다.

이번 이슈는 rt 라이브러리가 링킹되지 않아 발생한 문제였습니다. 이를 체크하기 위해 aio_xxx와 clock_gettime 중 하나를 선택한 것입니다.

Linux Foundation에서 확인한 바에 따르면, rt 라이브러리에는 clock_gettime이 포함되어 있었습니다. 그래서 이를 기준으로 체크한 후 -lrt를 추가하도록 결정했습니다. (출처: Linux Standard Base)

또한 제가 확인한 범위에서는 rt 라이브러리에 aio_xxx는 존재하지만 clock_gettime이 없는 경우는 없었던 것 같습니다. 그래서 clock_gettime을 체크 기준으로 삼게 된 것입니다.

만약 aio_xxx만 rt에 존재하거나 다른 라이브러리에 존재한다면, 그 경우에도 근본적인 해결책이 되지는 않을 것입니다.

추가로, EE 캐시 서버 코드를 보니 이미 그렇게 구현되어 있습니다.
따라서 두번째 이유도 적합하지 않은 이유입니다.

이 부분은 이해가 되지 않네요. 조금 더 설명해주시면 감사하겠습니다.

모든 시스템에서 해당되지 않을 수도 있어서 librt 검사를 포함하지 않은 것입니다.

MacOS는 libc에 포함되어 있어서 aio_suspend() 문제가 발생하지 않은 것입니다.
librt를 포함 기준을 aio_xxx() 함수로 바꿔도 MacOS에서 문제가 발생하지 않습니다.

https://man7.org/linux/man-pages/man3/aio_suspend.3.html
리눅스 메뉴얼 페이지에서 aio_xxx() 함수는 librt에 포함되어 있습니다.
librt를 포함 기준을 aio_xxx() 함수로 바꿔도 CentOS를 제외한 다른 리눅스 시스템에서 문제가 발생하지 않습니다.

이번 이슈는 rt 라이브러리가 링킹되지 않아 발생한 문제였습니다.

기존 로직에서 librt 내에서 clock_gettime을 탐색하여 존재를 확인했음에도 불구하고 -lrt 옵션을 아예 주지 않고 있었던 문제와 aio_xxx() 함수를 사용하지 못하는 문제가 서로 다른 이슈라는 의미입니다.
두 이슈의 해결 방법은 공통적으로 librt를 링킹한다는 것이지만, aio_xxx() 함수를 사용하지 못하는 문제를 해결하기 위해 clock_gettime() 함수의 존재 여부를 확인하는 것은 의미상 부적합한 것입니다.

이 부분은 이해가 되지 않네요. 조금 더 설명해주시면 감사하겠습니다.

EE 코드는 이미 아래와 같이 구현되어 있습니다. 그런데도 문제가 있나요?

#ifdef USE_CLOCK_GETTIME clock_gettime(...); #else gettimeofday(...); #endif

https://man7.org/linux/man-pages/man3/aio_suspend.3.html
리눅스 메뉴얼 페이지에서 aio_xxx() 함수는 librt에 포함되어 있습니다.
librt를 포함 기준을 aio_xxx() 함수로 바꿔도 CentOS를 제외한 다른 리눅스 시스템에서 문제가 발생하지 않습니다.

주신 링크를 살펴본 결과, clock_gettime과 aio_xxx 함수가 같은 시기에 추가된 것으로 보이며
clock_gettime과 aio_xxx가 항상 librt에 존재함이 보장되는 것 같습니다.

EE 코드는 이미 아래와 같이 구현되어 있습니다. 그런데도 문제가 있나요?

지금 수정하고 있는 기준으로 말씀하시는 거라면 문제가 없을 것입니다.

기존 코드로 말씀하시는 거라면 문제가 존재합니다. 기존에는 낮은 버전의 Libc 사용시 -lrt 옵션을 주지 않아
rt가 링킹되지 않기에 zk가 링킹하지 않고 위의 코드가 구현되어 있다면 낮은 버전의 libc에서 문제가 발생할 것입니다.

기존 코드로 말씀하시는 거라면 문제가 존재합니다. 기존에는 낮은 버전의 Libc 사용시 -lrt 옵션을 주지 않아
rt가 링킹되지 않기에 zk가 링킹하지 않고 위의 코드가 구현되어 있다면 낮은 버전의 libc에서 문제가 발생할 것입니다.

그래서 아래와 같이 질문했던 것입니다.

기존에 --enable-zk-integration 없이 컴파일한 경우에 해당 코드들이 정상 동작하나요?
혹은 ZK가 없으면 실행되지 않는 코드라서 문제가 없었던 것인가요?

정말로 libc 버전도 낮고 librt가 링킹되지 않으면 문제가 발생하는 코드임을 확인했나요?

ing-eoking · 2024-10-29T07:50:00Z

Makefile.am

@@ -95,7 +95,7 @@ memcached_SOURCES = \
 memcached_LDFLAGS =-R '$(libdir)'
 memcached_CFLAGS = @PROFILER_FLAGS@ ${AM_CFLAGS}
 memcached_DEPENDENCIES = libmcd_util.la
-memcached_LDADD = @PROFILER_LDFLAGS@ libmcd_util.la -levent $(APPLICATION_LIBS)
+memcached_LDADD = @CLOCK_GETTIME_LIBS@ @PROFILER_LDFLAGS@ libmcd_util.la -levent $(APPLICATION_LIBS)


@{변수명}@

configure.ac에서 정의된 변수를 사용합니다.

ing-eoking · 2024-10-29T07:54:25Z

configure.ac

        AC_DEFINE(USE_CLOCK_GETTIME, 1, [Defined if clock_gettime is available in libc]),
        AC_DEFINE(USE_GETTIMEOFDAY, 1, [Defined if clock_gettime is not available in libc]))
+AC_SUBST(CLOCK_GETTIME_LIBS)


AC_SUBST($VAR)

configure.ac 외부에서 사용하기 위한 변수를 지정합니다.
이렇게 하면 CLOCK_GETTIME_LIBS를 Makefile에서 활용할 수 있게 됩니다.

ing-eoking · 2024-10-30T08:17:36Z

@jhpark816 @namsic

clock_gettime, aio_suspend를 둘 다 확인하고 둘 중 하나라도 있으면 링킹하는 것으로 변경하기로 했습니다.

다른 의견 있을 시 주시면 감사하겠습니다.

namsic · 2024-10-30T08:49:07Z

clock_gettime, aio_suspend를 둘 다 확인하고 둘 중 하나라도 있으면 링킹하는 것으로 변경하기로 했습니다.

아래 상황에서 -lrt 포함하지 않고 빌드해도 문제가 없나요?
=> 문제 없습니다.

무조건 -lrt 옵션을 주는 것은 좋지 않다고 봅니다. rt 라이브러리가 없는 환경이 있을 수도 있습니다.

aio_xxx 함수가 모든 시스템에서 librt에 포함된다는 보장이 없었습니다. (Squall/Gust 엔진 문제는 CentOS 7에서만 발생했으며, 제 MacOS에서는 문제가 발생하지 않았습니다.)

함수 존재 여부에 따라 링크한다는 것은, 두 함수가 없는 환경에서는 -lrt 설정하지 않고도 aio_xxx() 호출할 수 있다는 것이지요?
(예를 들어, MacOS에서는 librt를 사용하지 않고도 어디선가 aio_xxx()를 가져다가 호출하여 빌드 성공한다는 것이지요?)
아니면, librt 없는 환경에서 빌드하면 aio_xxx() 함수를 사용하지 않는 구현으로 빌드가 된다는 것인가요?

jhpark816 · 2024-10-30T08:51:35Z

@ing-eoking
의견입니다.

clock_gettime 함수
- 현재 EE 버전에서만 사용됨.
- configure 검사가 CE 버전에서는 필요 없고 EE 버전에서만 필요.
- 따라서 조건적으로 librt와 링킹 필요
- 참고, replication & migraiton 로직에서만 clock_gettime 사용하고 있을 것 같음.
aio_suspend 함수
- 현재 EE 버전에서만 사용됨.
- 무조건 librt 필요. (MacOS에서는 다른가요?)
- configure 검사가 필요 없고, 무조건 librt와 링킹 설정
- squall or gust 엔진에서만 사용하므로, 그 엔진의 library 빌드 시에 링킹되면 좋겠음.

ing-eoking · 2024-10-31T00:56:16Z

@namsic

함수 존재 여부에 따라 링크한다는 것은, 두 함수가 없는 환경에서는 -lrt 설정하지 않고도 aio_xxx() 호출할 수 있다는 것이지요?

네. 맞습니다.

@jhpark816

그렇다면, CE 버전이 아닌 EE 버전만 수정하면 될까요?

무조건 librt 필요. (MacOS에서는 다른가요?)

네. MacOS에서는 -lrt를 주지 않아도 됩니다.

jhpark816 · 2024-10-31T01:40:17Z

그렇다면, CE 버전이 아닌 EE 버전만 수정하면 될까요?

수정은 둘 다 필요하지 않나요?

CE 버전에서는 해당 configure 사항을 제거
EE 버전에서는 해당 configure 사항을 적합하게 추가

네. MacOS에서는 -lrt를 주지 않아도 됩니다.

MacOS에서는 aio_xxxx() 함수들이 어떤 library 통해 제공되나요?
그 library는 linking 된 상태인가요?

jhpark816 · 2024-10-31T01:41:38Z

@ing-eoking
어떻게 수정한다는 사항을 먼저 정리해 주고, PR을 수정해 주세요.

uhm0311 · 2024-10-31T01:42:40Z

MacOS에서는 aio_xxxx() 함수들이 어떤 library 통해 제공되나요?
그 library는 linking 된 상태인가요?

libc를 통해 제공됩니다.

https://developer.apple.com/library/archive/documentation/System/Conceptual/ManPages_iPhoneOS/man2/aio_suspend.2.html

ing-eoking · 2024-10-31T01:57:11Z

@jhpark816

어떻게 수정한다는 사항을 먼저 정리해 주고, PR을 수정해 주세요.

현재 PR은 CE 버전에서는 해당 configure 사항을 제거 로 변경하겠습니다.

이후 EE 버전에서는 해당 configure 사항을 적합하게 추가를 EE 쪽 PR로 올려 진행하겠니다.

jhpark816 · 2024-10-31T03:45:34Z

본 PR을 merge하여 EE에 반영하면 문제가 될 수 있으므로,
본 PR은 그대로 두고 EE을 먼저 수정하는 것이 좋겠습니다.

jhpark816 · 2024-11-11T07:03:38Z

@ing-eoking
본 PR을 merge하면 되죠?

ing-eoking · 2024-11-11T07:13:08Z

@jhpark816

네 머지해도 될 것 같습니다.

ing-eoking marked this pull request as draft October 23, 2024 01:02

ing-eoking marked this pull request as ready for review October 23, 2024 01:43

ing-eoking requested review from jhpark816, namsic and uhm0311 October 23, 2024 01:43

jhpark816 removed their request for review October 23, 2024 02:07

ing-eoking commented Oct 29, 2024

View reviewed changes

INTERNAL: Remove the check for clock_gettime function

6d4dbeb

ing-eoking force-pushed the lrt branch from 0a1e1c6 to 6d4dbeb Compare October 31, 2024 02:26

ing-eoking changed the title ~~FIX: Link a librt if it exists~~ INTERNAL: Remove the check for clock_gettime function Oct 31, 2024

ing-eoking marked this pull request as draft October 31, 2024 02:27

ing-eoking marked this pull request as ready for review October 31, 2024 02:42

uhm0311 approved these changes Oct 31, 2024

View reviewed changes

jhpark816 merged commit bf52896 into naver:develop Nov 11, 2024
1 check passed

ing-eoking deleted the lrt branch November 11, 2024 07:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

INTERNAL: Remove the check for clock_gettime function #789

INTERNAL: Remove the check for clock_gettime function #789

ing-eoking commented Oct 23, 2024

ing-eoking Oct 29, 2024

uhm0311 Oct 29, 2024

ing-eoking Oct 29, 2024 •

edited

Loading

jhpark816 Oct 30, 2024

jhpark816 Oct 30, 2024

ing-eoking Oct 30, 2024

ing-eoking Oct 30, 2024 •

edited

Loading

uhm0311 Oct 30, 2024

ing-eoking Oct 30, 2024 •

edited

Loading

uhm0311 Oct 30, 2024

ing-eoking Oct 29, 2024

ing-eoking Oct 29, 2024

ing-eoking commented Oct 30, 2024 •

edited by uhm0311

Loading

namsic commented Oct 30, 2024 •

edited

Loading

jhpark816 commented Oct 30, 2024

ing-eoking commented Oct 31, 2024 •

edited

Loading

jhpark816 commented Oct 31, 2024 •

edited

Loading

jhpark816 commented Oct 31, 2024

uhm0311 commented Oct 31, 2024 •

edited

Loading

ing-eoking commented Oct 31, 2024

jhpark816 commented Oct 31, 2024

jhpark816 commented Nov 11, 2024

ing-eoking commented Nov 11, 2024

INTERNAL: Remove the check for clock_gettime function #789

INTERNAL: Remove the check for clock_gettime function #789

Conversation

ing-eoking commented Oct 23, 2024

🔗 Related Issue

⌨️ What I did

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ing-eoking Oct 29, 2024 • edited Loading

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 Oct 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ing-eoking Oct 30, 2024 • edited Loading

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 Oct 30, 2024 • edited by uhm0311 Loading

namsic commented Oct 30, 2024 • edited Loading

jhpark816 commented Oct 30, 2024

ing-eoking commented Oct 31, 2024 • edited Loading

jhpark816 commented Oct 31, 2024 • edited Loading

jhpark816 commented Oct 31, 2024

uhm0311 commented Oct 31, 2024 • edited Loading

ing-eoking commented Oct 31, 2024

jhpark816 commented Oct 31, 2024

jhpark816 commented Nov 11, 2024

ing-eoking commented Nov 11, 2024

ing-eoking Oct 29, 2024 •

edited

Loading

ing-eoking Oct 30, 2024 •

edited

Loading

ing-eoking Oct 30, 2024 •

edited

Loading

ing-eoking commented Oct 30, 2024 •

edited by uhm0311

Loading

namsic commented Oct 30, 2024 •

edited

Loading

ing-eoking commented Oct 31, 2024 •

edited

Loading

jhpark816 commented Oct 31, 2024 •

edited

Loading

uhm0311 commented Oct 31, 2024 •

edited

Loading