Extend the generic skeleton to opening non-owned shared memory objects. by Rahul-Sutariya · Pull Request #482 · eclipse-score/communication

Rahul-Sutariya · 2026-05-28T13:42:01Z

Issue: #387

crimson11 · 2026-05-29T11:09:32Z

+
+// Inter-VM shared-memory backends can have a short NAME_MAX, use compact 5-digit service ID instead of the default
+// 16-digit padding
+void AppendServiceAndInstanceCompact(std::ostream& out,


You can leave out this "compacting". If we would takes this over from the PoC ... then we would really materialize all the bugs/short-commings, which BlackBerry had in their sample-contribution :) (namely they had a weird restriction in their qvmshm implementation regarding path-name-sizes).

removed compacting, spilt the pr and updated to: #492

crimson11 · 2026-05-29T11:58:00Z

 {
  public:
-    explicit ShmPathBuilder(const std::uint16_t service_id) noexcept : IShmPathBuilder{}, service_id_{service_id} {}
+    explicit ShmPathBuilder(const std::uint16_t service_id, const bool inter_vm_support = false) noexcept


Add a doxygen comment to this ctor.
document/comment the inter_vm_support parameter. Note, that it is an interim solution, to express the intent to have a shm-object, which supports sharing across VMs by encoding it into the path-name. A future solution will be based on a redesigned lib/memory/shared API abstraction.

updated the doxygen comment and spilt the pr and updated to: #492 https://github.com/eclipse-score/communication/pull/492/changes#diff-23baa8b4571febbd6ac770b38d32efd5fdba6b4a52c5ed12e37d03531ef2ef0dR32-R44

crimson11 · 2026-05-29T12:12:28Z

+            gateway_shm_ready_ = false;
+            break;
+
+        case ShmReuseStrategy::kUnknownStrategy:


You should have a "fal through" here:

case ShmReuseStrategy::kUnknownStrategy: default: SCORE_LANGUAGE_FUTURECPP_ASSERT_PRD_MESSAGE(false, "Unknown SHM reuse strategy."); break;

Adapted the changes, thanks

crimson11 · 2026-05-29T14:23:03Z

+                                      "Could not open existing shared memory region for gateway-forwarded service.");
+            }
+            was_old_shm_region_reopened_ = false;
+            gateway_shm_ready_ = true;


I guess, we should rename gateway_shm_ready_ -> use_gateway_forwarded_shm_

midterm we might need some refactoring imho ... because now we have different flags, which are mutual exclusive (was_old_shm_region_reopened_ , use_gateway_forwarded-shm_) which is a bit ugly...

So from a high-level perspective, we have either a "normal" skeleton ... or a "forwarding" skeleton.
So - for now we express the latter only with this flag use_gateway_forwarded_shm_ ... which looks a bit "hacky" as a lot of behaviour is different in this case. So I'm thinking in the direction to introduce a more generic "mode" attribute in our skeleton class, which is either "REGULAR" or "FORWARDING" ... but this is - as I said - for future refactorings.

remanned gateway_shm_ready_ -> use_gateway_forwarded_shm_, thanks

crimson11 · 2026-05-29T14:32:12Z

    // re-open that shared memory in PrepareOffer(). In that case, we should retrieved the EventDataControl and
    // EventDataStorage from the shared memory and attempt to rollback the Skeleton tracing transaction log.
-    if (was_old_shm_region_reopened_)
+    if (gateway_shm_ready_)


We should do an assert here! If we end up in this template-function call AND gateway_shm_ready_ (now - see above use_gateway_forwarded_shm_ ) is true, something went wrong.
Why? Because this specific interVM functionality is only used in our gateway setups, where always GenericSkeleton is used - no "typed skeletons". But in case of a GenericSkeleton this template-func will never be called! Because in this case we only deal with GenericSkeletonEvents, which do not call this template func, but instead the non-template func Skeleton::RegisterGeneric()!

So - make an assert here and document, that use_gateway_forwarded_shm_ == true is only expected in gateway-use-cases, where GenericSkeletons are used ...

updated the changes, Thanks

crimson11 · 2026-05-29T14:43:19Z

+    const auto event_metainfo_it = find_element(storage_->events_metainfo_, element_fq_id);
    const auto event_data_storage_it = find_element(storage_->events_, element_fq_id);

+    if (lola_service_instance_deployment_.inter_vm_support_ && lola_service_instance_deployment_.inter_vm_forwarded_)


See argumentation above! In case we are in the interVM-gateway case, we expect a GenericSkeleton being used. But then, we would NOT end up in this template func as it is only used within "typed skeletons".

Updated the changes. Thanks

crimson11 · 2026-05-29T15:58:15Z

    return quality_type_;
 }

+Result<void> Skeleton::ValidateDataShmObject(const SkeletonEventBindings& events,


I did not see any calls to this method (and also not to ValidateControlShmObject)

Where did you expect the validation calls to happen?
My 1st guess would be directly after opening the shm-objects in PrepareOffer?

But now I had a 2nd look... and I'm not sure, whether this validation you implemented here is "added value".
My thoughts:

in Skeleton::RegisterGeneric() further down you have the code every GenericSkeletonEvent (and later also field) will run through, to register at its parent skeleton, thereby also "checking" the shared memory (CONTROL and DATA). I.e. at least it checks, whether for its ElementFqId the expected control/data artefact exists in shared-memory ... so if in the interVM use case, the shm, which we opened would be a wrong one, Skeleton::RegisterGeneric() would fail.

do you really check much more here, than what is implicitly checked in Skeleton::RegisterGeneric()?

You are even hyper-strict here - as you check, that the shm conains exactly the number of events/fields as you have here with events/ fields params ... if the shm-object would contain additional ones ... that would not even be a real problem/error. The Skeleton::RegisterGeneric() is more "flexible" ... it just checks, that the things at least exist in shm, which we expect ... if there is even more ... no problem.

My proposal:

remove ValidateDataShmObject and ValidateControlShmObject

if you feel that some consistency checks are missing within Skeleton::RegisterGeneric() ... add them there.

yes, you are right i feel Skeleton::RegisterGeneric() is enough to verify, removed both functions, Thanks

crimson11 · 2026-05-29T16:43:24Z

                               const size_t sample_alignment) noexcept -> GenericRegistrationResult
 {
-    if (was_old_shm_region_reopened_)
+    if (gateway_shm_ready_)


This can be optimized:
the if/else branch basically just differ regarding rollback! Only in one case a rollback is done - in the other not.

if (use_gateway_forwarded_shm_ || was_old_shm_region_reopened_) { auto [event_data_control_qm, event_data_control_asil_b] = memory_manager_.RetrieveEventControlsFromOpenedSharedMemory(element_fq_id); auto& event_data_storage = memory_manager_.RetrieveEventDataFromOpenedSharedMemory<std::uint8_t>(element_fq_id); if (!use_gateway_forwarded_shm_) { memory_manager_.RollbackSkeletonTracingTransactions(event_data_control_qm); if (event_data_control_asil_b != nullptr) { memory_manager_.RollbackSkeletonTracingTransactions(*event_data_control_asil_b); } } return {static_cast<void*>(&event_data_storage), event_data_control_qm, event_data_control_asil_b}; }

Optimized the code, Thanks

Issue: eclipse-score#387

Rahul-Sutariya marked this pull request as ready for review May 28, 2026 13:48

Rahul-Sutariya requested review from LittleHuba, bemerybmw, castler, crimson11, hoe-jo and limdor as code owners May 28, 2026 13:48

Rahul-Sutariya self-assigned this May 29, 2026

crimson11 requested changes May 29, 2026

View reviewed changes

Rahul-Sutariya force-pushed the extend_genericSkeleton_to_open_non_owned_shared_mem_obj branch from 84454fd to 22b416d Compare June 1, 2026 10:50

Extend "GenericSkeleton" to open non-owned shared memory objects.

ed155b6

Issue: eclipse-score#387

Rahul-Sutariya force-pushed the extend_genericSkeleton_to_open_non_owned_shared_mem_obj branch from 22b416d to ed155b6 Compare June 2, 2026 05:25

Rahul-Sutariya changed the title ~~Extend the generic skeleton to opening non-owned shared memory objects and add ShmPathBuilder extension for inter-VM shm path construction.~~ Extend the generic skeleton to opening non-owned shared memory objects. Jun 2, 2026

Rahul-Sutariya requested a review from crimson11 June 2, 2026 05:46

Conversation

Rahul-Sutariya commented May 28, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants