Add onlyForTopics and forbiddenTopics config options to enable/disables PSMXes on a topic-by-topic basis #2030

tobiasstarkwayve · 2024-06-10T15:47:42Z

Currently, enabling the iceoryx transport is an all-or-nothing matter. Either all topics use the iceoryx transport or none of them does.

In a larger system, this can be unnecessarily burdensome. A ROS system running on Cyclone, for example, may contain countless small publishers related to built-in ROS topics, services, or just small application-specific ROS topics. For those topics, iceoryx transport doesn't really have a large benefit. It is therefore needlessly onerous to conform to the various iceoryx restrictions (such as pre-allocated memory pools, peak number of outstanding loans, ...).

This PR adds a configuration mechanism to restrict use of iceoryx (or, more precisely, any PSMX mechanism) to individual named topics or to all but a few named topics. It thereby enables Cyclone users to enable iceoryx transport for those topics where shared-memory transport makes a real difference or to disable iceoryx for those topics where its restrictions are particularly onerous.

As a side-benefit, this PR also helps debugging bugs related to violations of the iceoryx preconditions. If, say, random segfaults are encountered in a project, topics can disable iceoryx one by one until the culprit is found.

The PR as contained right now is in working state, but a few polishes could still be applied (e.g., better testing). I'd like to agree on the general approach first before putting effort into tests, though.

eboasson

Thank you @tobiasstarkwayve! I think this is a very sensible improvement indeed. It looks also like the approach is practical and I don't see any obvious problems with it. The only real comment I have is that I think it would make sense to lean a bit more heavily on the existing infrastructure for "network partitions" and "ignored partitions".

I've given some comments to this effect and gave some links to bits that might be of use. I am sure those links are not really needed, but maybe it saves you some time.

Also, since you're mentioning ROS 2 and are working on Cyclone master ... perhaps you have sorted out the build issues for the RMW layer already, or perhaps you were simply planning to look at that later. I did some work and you're welcome to use it: https://github.com/eboasson/rmw_cyclonedds/pull/new/cdds-master. There are still some test obligations remaining, but the basics seem to work. Perhaps it is useful to you.

Let's make this happen. ☺️

P.S. You probably already figured out how best to deal with the conflicts on the generated configuration stuff. If not ...

git reset ../docs/manual/config/config_file_reference.rst ../docs/manual/options.md ../etc/cyclonedds.rnc ../etc/cyclonedds.xsd ../src/core/ddsi/defconfig.c && git checkout ../docs/manual/config/config_file_reference.rst ../docs/manual/options.md ../etc/cyclonedds.rnc ../etc/cyclonedds.xsd ../src/core/ddsi/defconfig.c
cmake .
make

usually solves all the issues.

eboasson · 2024-06-17T18:32:13Z

src/core/ddsc/src/dds_psmx.c

@@ -249,10 +266,54 @@ static dds_return_t psmx_instance_load (const struct ddsi_domaingv *gv, struct d
    goto err_init;
  }
  psmx_instance->priority = config->priority.value;
+
+  if (config->only_for_topics.size > 0 && config->forbidden_topics.size > 0) {


I tend to think that having both allow and deny lists makes sense, then "it" (in this case, Iceoryx or PSMX, but it applies more generally) only gets used if allowed and not disallowed. In short, I think it would not consider this conflicting options.

@eboasson The problem with having both options at the same time is how this interacts with the default state. If you have a topic that is on neither list, what do you do?
If the default is to use the PSMX then the allowlist makes no sense. If the default is not to use the PSMX then the denylist makes no sense.

So what this PR does is to set the default based on which list is used. If an allowlist is provided then the default is "no". If a denylist is provided then the default is "yes". And that's why these two options are conflicting.

eboasson · 2024-06-17T18:36:22Z

src/core/ddsc/src/dds_topic.c

+    bool allowed_by_config = !psmx->only_for_topics[0];
+    // Then any topic in only_for_topics is allowed
+    for (char **topic = psmx->only_for_topics; *topic; topic++) {
+      if (strcmp(ktp->name, *topic) == 0) {


I would also use pattern matching (perhaps even regex matching — but those are not as portable), not just exact matches. It is only used outside the "hot" path, so performance is not that big of an issue.

There is

cyclonedds/src/core/ddsi/src/ddsi_misc.c

Line 39 in 00c63c3

int ddsi_patmatch (const char *pat, const char *str)

, and there is also

cyclonedds/src/core/ddsi/src/ddsi_nwpart.c

Line 92 in 00c63c3

static int ddsi_is_ignored_nwpart_one (const struct ddsi_config *cfg, const char *partition, const char *topic)

which at the very least has a passing resemblance in functionality to the lists here.

@eboasson That's a good idea. I'll check it out.

@eboasson Done. There was a slight complication in that ddsi_misc was not available from the place where I needed it, so I added a new header ddsi/ddsi_misc.h to export it.

eboasson · 2024-06-17T18:38:02Z

src/core/ddsi/src/ddsi__cfgelems.h

@@ -103,6 +103,21 @@ static struct cfgelem psmx_attributes[] = {
      "This has no meaning in CycloneDDS itself, and its parsing is deferred to the"
      "PSMX implementation.</p>"
    )),
+  STRING("forbiddenTopics", NULL, 1, "",
+    MEMBEROF(ddsi_config_psmx_listelem, cfg.forbidden_topics),
+    FUNCTIONS(if_topic_array, uf_topic_array, ff_topic_array, pf_topic_array),


I would go for exactly the same pattern as what is used in the "network partitions" (and the "ignored partitions", so

cyclonedds/src/core/ddsi/src/ddsi__cfgelems.h

Line 739 in 00c63c3

MEMBEROF(ddsi_config_ignoredpartition_listelem, DCPSPartitionTopic),

). Not because it is necessarily better, but simply because it means more code can be reused.

Doing it using multiple XML elements also means you don't have to worry (as much) about what characters are allowed in topic names 😀

@eboasson Thank you! I had the feeling that this problem must have come up in some other option already, I didn't think of the "ignored partitions" setting.

@eboasson Moved the patterns into separate XML elements. I had some trouble figuring out how to work with XML text content, so the config now looks something like this

<PubSubMessageExchange name="iox" library="psmx_iox" config="LOG_LEVEL=INFO;"> <forbiddenTopics> <Pattern value="Round?rip"/>  <Pattern value="*Hello*"/> </forbiddenTopics> </PubSubMessageExchange>

If you'd prefer the pattern to be the text content of the Pattern element I'd need some pointers on how to do that :-).

eboasson · 2024-06-17T18:41:19Z

src/core/ddsi/src/ddsi_config.c

@@ -680,9 +688,7 @@ static int if_psmx(struct ddsi_cfgst *cfgst, void *parent, struct cfgelem const
  struct ddsi_config_psmx_listelem *new = if_common (cfgst, parent, cfgelem, sizeof(*new));
  if (new == NULL)
    return -1;
-  new->cfg.name = NULL;


if_common at some point started doing a memset — it is quite amazing it avoided doing it for as many years as it did! — so rather than adding a memset here, we should just delete setting the fields to null pointers.

See

cyclonedds/src/core/ddsi/src/ddsi_config.c

Line 649 in 00c63c3

static void *if_common (UNUSED_ARG (struct ddsi_cfgst *cfgst), void *parent, struct cfgelem const * const cfgelem, unsigned size)

, or commit e6fc9a9.

I added a commit removing some of the redundant initialisations, at least all the ones I could find with a quick search.

eboasson reviewed Jun 17, 2024

View reviewed changes

tobiasstarkwayve added 3 commits June 19, 2024 10:13

Add onlyForTopics and forbiddenTopics config option

b636313

Change config format of topic allow/denylists to XML elements

1024475

Interpret only-for-topics/forbidden topics as patterns

faa4b07

tobiasstarkwayve force-pushed the tobiasstark/enable-shm-per-topic branch from 4f42a40 to faa4b07 Compare June 19, 2024 15:04

Remove a few redundant zero initialisations

1bc4083

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add onlyForTopics and forbiddenTopics config options to enable/disables PSMXes on a topic-by-topic basis #2030

Add onlyForTopics and forbiddenTopics config options to enable/disables PSMXes on a topic-by-topic basis #2030

tobiasstarkwayve commented Jun 10, 2024

eboasson left a comment

eboasson Jun 17, 2024

tobiasstarkwayve Jun 19, 2024 •

edited

Loading

eboasson Jun 17, 2024

tobiasstarkwayve Jun 19, 2024

tobiasstarkwayve Jun 19, 2024

eboasson Jun 17, 2024

tobiasstarkwayve Jun 19, 2024

tobiasstarkwayve Jun 19, 2024

eboasson Jun 17, 2024

tobiasstarkwayve Jun 19, 2024

Add onlyForTopics and forbiddenTopics config options to enable/disables PSMXes on a topic-by-topic basis #2030

Are you sure you want to change the base?

Add onlyForTopics and forbiddenTopics config options to enable/disables PSMXes on a topic-by-topic basis #2030

Conversation

tobiasstarkwayve commented Jun 10, 2024

eboasson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tobiasstarkwayve Jun 19, 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

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

tobiasstarkwayve Jun 19, 2024 •

edited

Loading