Skip to content
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

issue-1444: Use kernel delay accounting to calculate cpu wait #1630

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

antonmyagkov
Copy link
Collaborator

@antonmyagkov antonmyagkov commented Jul 17, 2024

issue: #1444

  1. Add stats fetcher type to configuration
  2. Rename ICGroupStatsFetcher to IStatsFetcher
  3. Implement TTaskStatsFetcher

taskstats doc:
https://www.kernel.org/doc/Documentation/accounting/taskstats.txt
https://www.kernel.org/doc/Documentation/accounting/delay-accounting.rst

@antonmyagkov antonmyagkov force-pushed the users/myagkov/issue-1444-use-delayacct branch from 9416d9c to d491e70 Compare July 17, 2024 21:37
Copy link
Contributor

Note

This is an automated comment that will be appended during run.

🔴 linux-x86_64-relwithdebinfo: some tests FAILED for commit d491e70.

TESTS PASSED ERRORS FAILED SKIPPED MUTED?
5458 5454 0 1 1 2

@antonmyagkov antonmyagkov force-pushed the users/myagkov/issue-1444-use-delayacct branch 5 times, most recently from 2044242 to 487a76d Compare July 23, 2024 15:19
Copy link
Contributor

Note

This is an automated comment that will be appended during run.

🔴 linux-x86_64-relwithdebinfo: some tests FAILED for commit 487a76d.

TESTS PASSED ERRORS FAILED SKIPPED MUTED?
5492 5491 0 1 0 0

@antonmyagkov antonmyagkov force-pushed the users/myagkov/issue-1444-use-delayacct branch from 487a76d to 3bc2042 Compare August 27, 2024 15:41
Copy link
Contributor

Note

This is an automated comment that will be appended during run.

🔴 linux-x86_64-relwithdebinfo: some tests FAILED for commit 3bc2042.

TESTS PASSED ERRORS FAILED SKIPPED MUTED?
5687 5686 0 1 0 0

@antonmyagkov antonmyagkov force-pushed the users/myagkov/issue-1444-use-delayacct branch 3 times, most recently from 4701cd8 to 52f2bd0 Compare November 1, 2024 20:38
Copy link
Contributor

github-actions bot commented Nov 1, 2024

Note

This is an automated comment that will be appended during run.

🔴 linux-x86_64-relwithdebinfo: some tests FAILED for commit 52f2bd0.

TESTS PASSED ERRORS FAILED SKIPPED MUTED?
6061 6060 0 1 0 0

@antonmyagkov antonmyagkov force-pushed the users/myagkov/issue-1444-use-delayacct branch 2 times, most recently from a08f79b to 497d987 Compare November 1, 2024 23:06
Copy link
Contributor

github-actions bot commented Nov 2, 2024

Note

This is an automated comment that will be appended during run.

🟢 linux-x86_64-relwithdebinfo: all tests PASSED for commit 497d987.

TESTS PASSED ERRORS FAILED SKIPPED MUTED?
6077 6077 0 0 0 0

Copy link
Contributor

github-actions bot commented Nov 2, 2024

Note

This is an automated comment that will be appended during run.

🟢 linux-x86_64-relwithdebinfo: all tests PASSED for commit 8001b7a.

TESTS PASSED ERRORS FAILED SKIPPED MUTED?
6077 6077 0 0 0 0

@antonmyagkov antonmyagkov force-pushed the users/myagkov/issue-1444-use-delayacct branch from f5b8b26 to 68aa05f Compare November 3, 2024 22:09
Copy link
Contributor

github-actions bot commented Nov 3, 2024

Note

This is an automated comment that will be appended during run.

🟢 linux-x86_64-relwithdebinfo: all tests PASSED for commit 68aa05f.

TESTS PASSED ERRORS FAILED SKIPPED MUTED?
6077 6077 0 0 0 0

Copy link
Contributor

github-actions bot commented Nov 4, 2024

Note

This is an automated comment that will be appended during run.

🟢 linux-x86_64-relwithdebinfo: all tests PASSED for commit 39c756e.

TESTS PASSED ERRORS FAILED SKIPPED MUTED?
6077 6077 0 0 0 0

@antonmyagkov antonmyagkov force-pushed the users/myagkov/issue-1444-use-delayacct branch from b8b085c to be4fec9 Compare November 8, 2024 22:30
Copy link
Contributor

github-actions bot commented Nov 9, 2024

Note

This is an automated comment that will be appended during run.

🟢 linux-x86_64-relwithdebinfo: all tests PASSED for commit a516eec.

TESTS PASSED ERRORS FAILED SKIPPED MUTED?
6098 6098 0 0 0 0

@antonmyagkov antonmyagkov force-pushed the users/myagkov/issue-1444-use-delayacct branch 4 times, most recently from f58ff78 to 50f14dd Compare November 15, 2024 12:25
tpashkin
tpashkin previously approved these changes Nov 19, 2024
Copy link
Contributor

Note

This is an automated comment that will be appended during run.

🟢 linux-x86_64-relwithdebinfo: all tests PASSED for commit 57a7799.

TESTS PASSED ERRORS FAILED SKIPPED MUTED?
6119 6119 0 0 0 0

Copy link
Contributor

github-actions bot commented Dec 3, 2024

Note

This is an automated comment that will be appended during run.

🔴 linux-x86_64-relwithdebinfo: some tests FAILED for commit 5d3fedc.

TESTS PASSED ERRORS FAILED SKIPPED MUTED?
6158 6157 0 1 0 0

@antonmyagkov antonmyagkov force-pushed the users/myagkov/issue-1444-use-delayacct branch from 8cbd414 to 7c3c9b6 Compare December 4, 2024 12:46
@antonmyagkov antonmyagkov force-pushed the users/myagkov/issue-1444-use-delayacct branch from 7c3c9b6 to 6ae006d Compare December 4, 2024 12:48
Copy link
Contributor

github-actions bot commented Dec 4, 2024

Note

This is an automated comment that will be appended during run.

🟢 linux-x86_64-relwithdebinfo: all tests PASSED for commit 6ae006d.

TESTS PASSED ERRORS FAILED SKIPPED MUTED?
6167 6167 0 0 0 0

@antonmyagkov antonmyagkov requested a review from tpashkin December 4, 2024 17:23

////////////////////////////////////////////////////////////////////////////////

struct TTaskStatsFetcher final: public IStatsFetcher
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TTaskStatsFetcher - основное изменение в этом пул реквесте

@antonmyagkov antonmyagkov added the rebase Add this label if you want to rebase your PR for test run label Dec 24, 2024
Copy link
Contributor

Note

This is an automated comment that will be appended during run.

🟢 linux-x86_64-relwithdebinfo: all tests PASSED for commit 6ae006d.

TESTS PASSED ERRORS FAILED SKIPPED MUTED?
6314 6314 0 0 0 0

tpashkin
tpashkin previously approved these changes Dec 27, 2024

TResultOrError<TDuration> GetCpuWait() override
{
try {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

может, использовать function try блок, чтобы не было двойной табуляции?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Прикольно. Я не знал про такую фичу.

Я бы оставил как есть для единообразия кода(По крайней мере я не нашел примеров использования function try в нашем коде).

auto cpuDelay = NThreading::NewPromise<TResultOrError<TDuration>>();
netlinkSocket.SetCallback(
NL_CB_VALID,
[this, &cpuDelay](nl_msg* nlmsg) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

чем гарантируется что this не протухнет к моменту вызова колбэка?

return NL_OK;
});
netlinkSocket.Send(message);
auto cpuWait = cpuDelay.GetFuture().GetValue(NetlinkSocketTimeout);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

аа, получается мы тут повиснем на ожидании коллбэка

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

мы блокируемся в Send на самом деле. Там синхронная реализация https://github.com/ydb-platform/nbs/blob/main/cloud/storage/core/libs/netlink/socket.cpp#L45

Copy link
Collaborator

@tpashkin tpashkin Jan 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

в целом ничего не мешает сделать его асинхронным, но тогда нужно будет аккуратно проставить error колбэки и пошарить контекст, например, через shared_from_this как сделано в nbd

cloud/storage/core/libs/diagnostics/stats_fetcher.cpp Outdated Show resolved Hide resolved

////////////////////////////////////////////////////////////////////////////////

struct TCgroupStatsFetcher final
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

лучше в разных файлах оставить разные реализации, тогда будет видно что код просто перенесли.


////////////////////////////////////////////////////////////////////////////////

ICgroupStatsFetcherPtr CreateCgroupStatsFetcher(
IStatsFetcherPtr CreateCgroupStatsFetcher(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

а чем CreateCgroupStatsFetcher/CreateTaskStatsFetcher отличается от BuildStatsFetcher не достаточно одной функции?

Copy link
Collaborator Author

@antonmyagkov antonmyagkov Jan 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Теперь точно нужны, так как я разнес реализации CGroupStatsFetcher и TaskStatsFetcher по разным cpp файлам

cloud/storage/core/protos

library/cpp/lwtrace/mon

contrib/restricted/libnl/lib/nl-3
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

от contrib/restricted/libnl начинает зависеть еще один ya.make нам снова прописывать исключения (((

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Есть re-implementation of libnl-2 с лицензий apache 2.0 https://android.googlesource.com/platform/system/core/+/29d238d/libnl_2/README

@tpashkin Может мы можем на нее перейти?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tpashkin Может мы можем на нее перейти?

Почему бы и нет, нам нужен довольно базовый набор функций

Copy link
Collaborator Author

@antonmyagkov antonmyagkov Jan 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Я создал отдельный тикет: #2869
Попробуем перейти на другую реализацию

Copy link
Collaborator

@tpashkin tpashkin Jan 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

от contrib/restricted/libnl начинает зависеть еще один ya.make нам снова прописывать исключения (((

@drbasic а напомни, плиз, что за исключения. вы сейчас не линкуетесь с libnl в аркадии?

@antonmyagkov antonmyagkov force-pushed the users/myagkov/issue-1444-use-delayacct branch from 3b4deb3 to c3a8efa Compare January 16, 2025 16:48
Copy link
Contributor

Note

This is an automated comment that will be appended during run.

🔴 linux-x86_64-relwithdebinfo: some tests FAILED for commit c3a8efa.

TESTS PASSED ERRORS FAILED SKIPPED MUTED?
6335 6332 0 3 0 0

Copy link
Contributor

Note

This is an automated comment that will be appended during run.

🟢 linux-x86_64-relwithdebinfo: all tests PASSED for commit c39eff4.

TESTS PASSED ERRORS FAILED SKIPPED MUTED?
6334 6334 0 0 0 0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rebase Add this label if you want to rebase your PR for test run
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants