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

Fix/allocate shm only when needed #325

Merged
merged 12 commits into from
Jan 21, 2025
Merged

Conversation

Koichi98
Copy link
Contributor

@Koichi98 Koichi98 commented Jan 20, 2025

Description

現状のAgnocastでは、agnocastを適用したプロセスのヒープ領域は全て共有メモリ (MAP_SHARED) 上に割り当てられるデザインになっているが、これはメモリ効率の観点で問題点がある:一度ヒープとして確保した共有メモリ物理ページが、今後再利用されなかったとしてもずっと解放されず、カーネルが再利用することができない。

madvise(MADV_REMOVE)を利用して、明示的にユーザレイヤから当該物理ページを破棄するように要求することは可能だが、
ミドルウェア (agnocast heaphook) から見える情報だけで MADV_REVOVE 発行タイミングの決定をするのは難しい。アプリケーションのコード上から、アプリケーションの知識に基づき MADV_REVOVE を発行する必要があるが、アプリケーションのプログラマにこのような低レイヤの操作を強いることはできない。

以上を踏まえ、そもそも共有メモリの使用を最低限に抑えるという方針を取ることとなった。これはborrow_loaned_message()からpublish()までの間で確保されるヒープ領域のみを共有メモリ上におくことで実現している。

本実装によって今後のAgnocast適用時には以下の2点に注意する必要がある。

  • ipc_shared_ptrで送信するMessageT型を持つメッセージについて、そのMessage型の持つメンバ変数へのmoveによる代入。(共有メモリ上に存在しないオブジェクトの送信を防ぐため)
  • borrow_loaned_message()からpublish()間で行う処理を必要最低限にする。(必要以上に共有メモリ上にオブジェクトを置かないようにするため。)

Related links

TIER IV Internal Doc

How was this PR tested?

  • sample application (required)
  • Autoware (required)

上記に加えて、Eveにおけるmap_loader -> pose_initializer, ad_service_state_monitor, ndt_scan_matcherへの適用。

Notes for reviewers

borrow_loaned_messageからpublishするまでのみを共有メモリに配置するようになったことで、agnocast_heaphookにおけるTLSF変数の初期化が初回borrow時になるが、initialize_agnocastが呼ばれていないのでこれではcreate_publisherやcreate_subscriptionにおけるioctlすら呼べない。それゆえ、TLSFの初期化はプロセスのmain関数の実行よりも前に行う必要があり、__libc_start_main時に行うこととした。初回mallocではなく__libc_start_mainとした理由は以下。

  • 単に初期化をしておきたいだけであって、初回malloc時にTLSFを利用して共有メモリ上に配置するわけではないのが可読性の観点でミスリーディング。
  • 初回のみTLSFのロックを取得して初期化をさせるようなコードは、グローバル変数を必要とするため、実装面でバグが生じやすく細心の注意が必要。
  • __libc_start_mainはUNIXにおいてユーザプロセスのmain関数実行前に必ず一回呼び出されるので初期化関数として最適なものの一つ。

Signed-off-by: Koichi Imai <koichi.imai.2@tier4.jp>
Signed-off-by: Koichi Imai <koichi.imai.2@tier4.jp>
Signed-off-by: Koichi Imai <koichi.imai.2@tier4.jp>
Signed-off-by: Koichi Imai <koichi.imai.2@tier4.jp>
Signed-off-by: Koichi Imai <koichi.imai.2@tier4.jp>
Signed-off-by: Koichi Imai <koichi.imai.2@tier4.jp>
Signed-off-by: Koichi Imai <koichi.imai.2@tier4.jp>
@sykwer sykwer self-requested a review January 20, 2025 04:45
Signed-off-by: Koichi Imai <koichi.imai.2@tier4.jp>
sykwer
sykwer previously approved these changes Jan 20, 2025
@sykwer
Copy link
Member

sykwer commented Jan 20, 2025

全てのコメントを反映したらLGTM

@Koichi98 Koichi98 requested review from veqcc and atsushi421 January 21, 2025 04:20
atsushi421
atsushi421 previously approved these changes Jan 21, 2025
Copy link
Contributor

@atsushi421 atsushi421 left a comment

Choose a reason for hiding this comment

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

LGTM!

@atsushi421 atsushi421 added the run-build-test Run build-test in CI label Jan 21, 2025
@veqcc
Copy link
Contributor

veqcc commented Jan 21, 2025

@Koichi98
clang-tidy が失敗してるので、対応お願いします 👍

Signed-off-by: Koichi Imai <koichi.imai.2@tier4.jp>
Signed-off-by: Koichi Imai <koichi.imai.2@tier4.jp>
@Koichi98 Koichi98 dismissed stale reviews from atsushi421 and sykwer via b74e83e January 21, 2025 06:52
veqcc
veqcc previously approved these changes Jan 21, 2025
@Koichi98 Koichi98 added run-build-test Run build-test in CI and removed run-build-test Run build-test in CI labels Jan 21, 2025
Copy link

------------------------------------------------------------------------------
                           GCC Code Coverage Report
Directory: .
------------------------------------------------------------------------------
File                                       Lines    Exec  Cover   Missing
------------------------------------------------------------------------------
src/agnocastlib/include/agnocast.hpp           2       2   100%   
src/agnocastlib/include/agnocast_executor.hpp
                                               1       0     0%   38
src/agnocastlib/include/agnocast_publisher.hpp
                                              35      27    77%   62,85-86,95-97,101-102
src/agnocastlib/include/agnocast_smart_pointer.hpp
                                              53      50    94%   77,80-81
src/agnocastlib/include/agnocast_topic_info.hpp
                                              13      13   100%   
src/agnocastlib/src/agnocast.cpp              82      15    18%   22,24,27,30,32-34,36,39,42,44-50,54-55,58-62,66-67,71-74,77,80,82-85,88,91,93-94,96-97,102,104-106,109-112,115,117-123,125,138-139,150-151,154-156,161
src/agnocastlib/src/agnocast_component_container.cpp
                                              12       0     0%   5,7-8,11-13,16,18-20,22-23
src/agnocastlib/src/agnocast_component_container_mt.cpp
                                              28       0     0%   7,11-12,14-16,18-20,22-24,26-28,30,32-33,35-37,40,42,44-46,48-49
src/agnocastlib/src/agnocast_executor.cpp
                                              86       0     0%   10,12,14-16,18-20,24,26,29,31-32,35-39,42-45,47-48,51-53,55-58,61-62,67,70,73,75-79,82,86-87,90-91,94,96-100,103,106,110-115,118,121-124,126-129,132,135-137,140,142,145,149-150,153-156,159,162,164-165
src/agnocastlib/src/agnocast_multi_threaded_executor.cpp
                                              51       0     0%   9,13,15-17,19-20,25-26,29-33,36,38-41,44,48,51,53-55,58-60,64,66-67,71,74,77,79-80,82-83,87-88,91,96,100,103-104,107,114-117,120
src/agnocastlib/src/agnocast_publisher.cpp
                                              84       7     8%   7,9,20,23,28,30,32-38,42-43,50,55,57-62,65-72,77,81-88,91-92,94-97,99-102,104,107-108,112-113,119,123-132,135-136,139,142,144-149,152
src/agnocastlib/src/agnocast_single_threaded_executor.cpp
                                              19       0     0%   10-12,16,18-21,24,28-30,33,35-37,40-42
src/agnocastlib/src/agnocast_smart_pointer.cpp
                                              20       0     0%   5,7-15,19,21-29
src/agnocastlib/src/agnocast_subscription.cpp
                                              92       9     9%   10,12-15,17,20,24,27-29,31,33,35,37-42,44-49,51,54-61,64-67,69,71,74,77,81-84,86-92,95-96,103,108,110-113,116,119,123-128,130-135,137,139,142,146-147,149-150
src/agnocastlib/src/agnocast_topic_info.cpp
                                              16       0     0%   12,16-17,20-24,28-31,34-37
src/agnocastlib/src/agnocast_utils.cpp        25      23    92%   48,50
------------------------------------------------------------------------------
TOTAL                                        619     146    23%
------------------------------------------------------------------------------

Signed-off-by: Koichi Imai <koichi.imai.2@tier4.jp>
@Koichi98 Koichi98 added run-build-test Run build-test in CI and removed run-build-test Run build-test in CI labels Jan 21, 2025
Copy link

------------------------------------------------------------------------------
                           GCC Code Coverage Report
Directory: .
------------------------------------------------------------------------------
File                                       Lines    Exec  Cover   Missing
------------------------------------------------------------------------------
src/agnocastlib/include/agnocast.hpp           2       2   100%   
src/agnocastlib/include/agnocast_executor.hpp
                                               1       0     0%   38
src/agnocastlib/include/agnocast_publisher.hpp
                                              35      27    77%   62,85-86,95-97,101-102
src/agnocastlib/include/agnocast_smart_pointer.hpp
                                              53      50    94%   77,80-81
src/agnocastlib/include/agnocast_topic_info.hpp
                                              13      13   100%   
src/agnocastlib/src/agnocast.cpp              82      15    18%   22,24,27,30,32-34,36,39,42,44-50,54-55,58-62,66-67,71-74,77,80,82-85,88,91,93-94,96-97,102,104-106,109-112,115,117-123,125,138-139,150-151,154-156,161
src/agnocastlib/src/agnocast_component_container.cpp
                                              12       0     0%   5,7-8,11-13,16,18-20,22-23
src/agnocastlib/src/agnocast_component_container_mt.cpp
                                              28       0     0%   7,11-12,14-16,18-20,22-24,26-28,30,32-33,35-37,40,42,44-46,48-49
src/agnocastlib/src/agnocast_executor.cpp
                                              86       0     0%   10,12,14-16,18-20,24,26,29,31-32,35-39,42-45,47-48,51-53,55-58,61-62,67,70,73,75-79,82,86-87,90-91,94,96-100,103,106,110-115,118,121-124,126-129,132,135-137,140,142,145,149-150,153-156,159,162,164-165
src/agnocastlib/src/agnocast_multi_threaded_executor.cpp
                                              51       0     0%   9,13,15-17,19-20,25-26,29-33,36,38-41,44,48,51,53-55,58-60,64,66-67,71,74,77,79-80,82-83,87-88,91,96,100,103-104,107,114-117,120
src/agnocastlib/src/agnocast_publisher.cpp
                                              84       7     8%   7,9,20,23,28,30,32-38,42-43,50,55,57-62,65-72,77,81-88,91-92,94-97,99-102,104,107-108,112-113,119,123-132,135-136,139,142,144-149,152
src/agnocastlib/src/agnocast_single_threaded_executor.cpp
                                              19       0     0%   10-12,16,18-21,24,28-30,33,35-37,40-42
src/agnocastlib/src/agnocast_smart_pointer.cpp
                                              20       0     0%   5,7-15,19,21-29
src/agnocastlib/src/agnocast_subscription.cpp
                                              92       9     9%   10,12-15,17,20,24,27-29,31,33,35,37-42,44-49,51,54-61,64-67,69,71,74,77,81-84,86-92,95-96,103,108,110-113,116,119,123-128,130-135,137,139,142,146-147,149-150
src/agnocastlib/src/agnocast_topic_info.cpp
                                              16       0     0%   12,16-17,20-24,28-31,34-37
src/agnocastlib/src/agnocast_utils.cpp        25      23    92%   48,50
------------------------------------------------------------------------------
TOTAL                                        619     146    23%
------------------------------------------------------------------------------

Signed-off-by: Koichi Imai <koichi.imai.2@tier4.jp>
@Koichi98 Koichi98 added run-build-test Run build-test in CI and removed run-build-test Run build-test in CI labels Jan 21, 2025
Copy link

------------------------------------------------------------------------------
                           GCC Code Coverage Report
Directory: .
------------------------------------------------------------------------------
File                                       Lines    Exec  Cover   Missing
------------------------------------------------------------------------------
src/agnocastlib/include/agnocast.hpp           2       2   100%   
src/agnocastlib/include/agnocast_executor.hpp
                                               1       0     0%   38
src/agnocastlib/include/agnocast_publisher.hpp
                                              35      27    77%   62,85-86,95-97,101-102
src/agnocastlib/include/agnocast_smart_pointer.hpp
                                              53      50    94%   77,80-81
src/agnocastlib/include/agnocast_topic_info.hpp
                                              13      13   100%   
src/agnocastlib/src/agnocast.cpp              82      15    18%   22,24,27,30,32-34,36,39,42,44-50,54-55,58-62,66-67,71-74,77,80,82-85,88,91,93-94,96-97,102,104-106,109-112,115,117-123,125,138-139,150-151,154-156,161
src/agnocastlib/src/agnocast_component_container.cpp
                                              12       0     0%   5,7-8,11-13,16,18-20,22-23
src/agnocastlib/src/agnocast_component_container_mt.cpp
                                              28       0     0%   7,11-12,14-16,18-20,22-24,26-28,30,32-33,35-37,40,42,44-46,48-49
src/agnocastlib/src/agnocast_executor.cpp
                                              86       0     0%   10,12,14-16,18-20,24,26,29,31-32,35-39,42-45,47-48,51-53,55-58,61-62,67,70,73,75-79,82,86-87,90-91,94,96-100,103,106,110-115,118,121-124,126-129,132,135-137,140,142,145,149-150,153-156,159,162,164-165
src/agnocastlib/src/agnocast_multi_threaded_executor.cpp
                                              51       0     0%   9,13,15-17,19-20,25-26,29-33,36,38-41,44,48,51,53-55,58-60,64,66-67,71,74,77,79-80,82-83,87-88,91,96,100,103-104,107,114-117,120
src/agnocastlib/src/agnocast_publisher.cpp
                                              84       7     8%   7,9,20,23,28,30,32-38,42-43,50,55,57-62,65-72,77,81-88,91-92,94-97,99-102,104,107-108,112-113,119,123-132,135-136,139,142,144-149,152
src/agnocastlib/src/agnocast_single_threaded_executor.cpp
                                              19       0     0%   10-12,16,18-21,24,28-30,33,35-37,40-42
src/agnocastlib/src/agnocast_smart_pointer.cpp
                                              20       0     0%   5,7-15,19,21-29
src/agnocastlib/src/agnocast_subscription.cpp
                                              92       9     9%   10,12-15,17,20,24,27-29,31,33,35,37-42,44-49,51,54-61,64-67,69,71,74,77,81-84,86-92,95-96,103,108,110-113,116,119,123-128,130-135,137,139,142,146-147,149-150
src/agnocastlib/src/agnocast_topic_info.cpp
                                              16       0     0%   12,16-17,20-24,28-31,34-37
src/agnocastlib/src/agnocast_utils.cpp        25      23    92%   48,50
------------------------------------------------------------------------------
TOTAL                                        619     146    23%
------------------------------------------------------------------------------

@atsushi421 atsushi421 self-requested a review January 21, 2025 08:31
@Koichi98 Koichi98 merged commit 67c082f into main Jan 21, 2025
4 checks passed
@Koichi98 Koichi98 deleted the fix/allocate_shm_only_when_needed branch January 21, 2025 08:51
atsushi421 pushed a commit that referenced this pull request Jan 23, 2025
* only allocate shared memory between borrow_loaned_message to publish

Signed-off-by: Koichi Imai <koichi.imai.2@tier4.jp>

* run_listener works but now it sometimes fails with PUBLISHER_ADD_CMD

Signed-off-by: Koichi Imai <koichi.imai.2@tier4.jp>

* above commit was correct, thus refactoring

Signed-off-by: Koichi Imai <koichi.imai.2@tier4.jp>

* add error handling for decrement_publisher_num_borrowed

Signed-off-by: Koichi Imai <koichi.imai.2@tier4.jp>

* refactoring

Signed-off-by: Koichi Imai <koichi.imai.2@tier4.jp>

* apply clang-format

Signed-off-by: Koichi Imai <koichi.imai.2@tier4.jp>

* Add comment

Signed-off-by: Koichi Imai <koichi.imai.2@tier4.jp>

* rename to borrowed_publisher_num & refactor

Signed-off-by: Koichi Imai <koichi.imai.2@tier4.jp>

* fix for clang-tidy

Signed-off-by: Koichi Imai <koichi.imai.2@tier4.jp>

* fix

Signed-off-by: Koichi Imai <koichi.imai.2@tier4.jp>

* apply cargo fmt

Signed-off-by: Koichi Imai <koichi.imai.2@tier4.jp>

* apply clippy

Signed-off-by: Koichi Imai <koichi.imai.2@tier4.jp>

---------

Signed-off-by: Koichi Imai <koichi.imai.2@tier4.jp>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
run-build-test Run build-test in CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants