add psmx test to check whether shared memory can be enabled and the l… #1939

mvandenhoek · 2024-02-29T16:04:33Z

…ocator can be set

firuze-cagliyan

Review ok.

dpotman

A few things that need a fix or can be improved, see review comments below.

dpotman · 2024-03-06T13:07:01Z

src/core/ddsc/tests/psmx.c

+        cfg.psmx_instances->cfg.priority.isdefault = 1;
+        cfg.psmx_instances->cfg.priority.value = 1000000;
+        cfg.psmx_instances->next = NULL;
+        domain = dds_create_domain_with_rawconfig(domainId, &cfg);


Seems to me that using the create_participant helper function would avoid some code duplication in this test. If you'd use domain Id 5 and add an entry with a specific locator to the global psmx_locator variable, that would work I think.

dpotman · 2024-03-06T14:10:22Z

src/core/ddsc/tests/psmx.c

+  bool using_psmx_iox = (strcmp(CDDS_PSMX_NAME, "iox") == 0);
+  ddsrt_free(CDDS_PSMX_NAME);
+
+  if ( using_psmx_iox ) {


Currently it's iox or cdds, but there could be more PSMX implementation, so I'd prefer not to assume here that we only have these 2. I think you can make the test case generic by using dds_psmx_supported_features to check for shared memory support of the plugin, and do the dds_is_shared_memory_available checks on the endpoints accordingly

I checked it, but dds_is_shared_memory_available() internally uses dds_psmx_supported_features(). In other words, if I call dds_psmx_supported_features() first, it can't help but give the same result for dds_is_shared_memory_available(). So it seems a bit pointless to me to do it that way.

What I meant is that the else block (from line 1636) can be completely removed (and so the if on line 1376) if you parameterize the "is shared memory available". That can be done by either hard-coding that the iox implementation uses shared memory, and cdds not, or by requesting the features from the plugin. The latter will also work if we decide to add more implementations to run this test on, so I think that's the way to go. Calling dds_is_shared_memory_available() on the endpoints could be used to check if the returned value is consistent with the features the PSMX instance claims to support.

dpotman · 2024-03-06T14:17:46Z

src/core/ddsc/tests/psmx.c

+        config_psmx_elm_free(cfg.psmx_instances);
+      }
+      CU_ASSERT_FATAL(domain > 0);
+      CU_ASSERT_FATAL(domain_has_psmx_instance_name(domain, "CycloneDDS-IOX-PSMX"));


You can pass an option to the iox plugin to set the SERVICE_NAME, I think it's better to use this and check for this value, so that the test does not rely on this plugin internal constant.

The SERVICE_NAME ends up in _service_name in the struct iox_psmx, which inherits from dds_psmx_t.
The iox_create_psmx() returns the struct dds_psmx, which doesn't contain the _service_name (it is in the derived class, not the base class). I couldn't find any getter functions for this. Perhaps it should be added to dds_psmx_ops_t ?

src/core/ddsc/tests/psmx.c

dpotman · 2024-03-06T15:29:15Z

src/core/ddsc/tests/psmx.c

+        CU_ASSERT_FATAL(dds_is_shared_memory_available(reader2));
+      }
+      {
+        // Write and read samples


I wouldn't include writing and reading samples in this test, because it's about testing the is_shared_memory flag. Maybe move this to a different test case?

I thought as 'setting the flag' only is not enough and we should also see that it works as it should be so that we can read/write samples when shared memory is enabled. Putting in another test case is also an option but it means that we should repeat some beginning steps of this test case in that one, so I think keeping these steps here is easier for us.

dpotman · 2024-03-06T15:29:58Z

src/core/ddsc/tests/psmx.c

+      dds_delete(domain);
+    }
+    {
+      // Test case using a config without specifying a locator.


Same as above: use the helper function and avoid assumptions on plugin internals.

dpotman · 2024-03-06T15:41:29Z

src/core/ddsc/tests/psmx.c

+      dds_entity_t participant = dds_create_participant(domainId, NULL, NULL);
+      CU_ASSERT_FATAL(participant > 0);
+      {
+        // Check that the second half of the locator is all zeros.


This assumption is based on details in the iox plugin implementation; the value assigned to the locator is the return value of the get_node_id call on the PSMX interface that returns a dds_psmx_node_identifier_t, which is 16 bytes. This happens to be an 64 bit value with trailing zero's, but could in theory be anything.

src/core/ddsc/tests/psmx.c

mvandenhoek · 2024-03-19T15:02:47Z

I think this addresses the comments, but I bumped into a problem with the cdds_psmx. When you run the same test twice in a loop (after cleaning up all entities of course, by means of dds_delete(domain);), it runs into an assert when the test code calls dds_create_topic() on the second iteration of the test.
psmx_cdds_impl.c:143: cdds_psmx_create_topic: Assertion `g_domain >= 0' failed.

The value of g_domain is -4, which stands for returncode DDS_RETCODE_PRECONDITION_NOT_MET.

The assert happens here:

cyclonedds/src/core/ddsc/tests/psmx_cdds_impl.c

Lines 135 to 145 in f724b4f

    
           static struct dds_psmx_topic * cdds_psmx_create_topic (struct dds_psmx * psmx, 
        
               const char * topic_name, const char * type_name, dds_data_type_properties_t data_type_props) 
        
           { 
        
             struct cdds_psmx *cpsmx = (struct cdds_psmx *) psmx; 
        
             if (g_domain == -1) 
        
             { 
        
               char *conf = ddsrt_expand_envvars (DDS_CONFIG, DDS_DOMAINID); 
        
               g_domain = dds_create_domain (DDS_DOMAINID, conf); 
        
               assert (g_domain >= 0); 
        
               ddsrt_free (conf); 
        
             }

The source of the returncode:

cyclonedds/src/core/ddsc/src/dds_domain.c

Lines 224 to 241 in f724b4f

    
           static dds_entity_t dds_domain_create_internal_xml_or_raw (dds_domain **domain_out, dds_domainid_t id, bool implicit, const struct config_source *config) 
        
           { 
        
             struct dds_domain *dom; 
        
             dds_entity_t domh = DDS_RETCODE_ERROR; 
        
             /* FIXME: should perhaps lock parent object just like everywhere */ 
        
             ddsrt_mutex_lock (&dds_global.m_mutex); 
        
            retry: 
        
             if (id != DDS_DOMAIN_DEFAULT) 
        
               dom = dds_domain_find_locked (id); 
        
             else 
        
               dom = ddsrt_avl_find_min (&dds_domaintree_def, &dds_global.m_domains); 
        
             if (dom) 
        
             { 
        
               if (!implicit) 
        
                 domh = DDS_RETCODE_PRECONDITION_NOT_MET; 
        
               else

It executes dom = dds_domain_find_locked (id);, then goes to domh = DDS_RETCODE_PRECONDITION_NOT_MET;.

mvandenhoek · 2024-04-03T11:42:08Z

Added a dummy psmx interface, which is now used by the shared memory test.

eboasson

Thank you for this @mvandenhoek but I'm afraid I have some different ideas on how to go about testing whether the functions on the PSMX interface get invoked (and the results used) correctly by the core library. I've tried to sketch these ideas in the comments.

I won't say it has to be done the way I sketched, but on the whole I think it makes more sense the approach you chose, specifically also because it makes fewer assumptions and keeps the different tests a bit cleaner.

eboasson · 2024-04-10T13:12:41Z

src/core/ddsc/tests/CMakeLists.txt

+if(BUILD_SHARED_LIBS)
+  target_link_libraries(psmx_dummy PRIVATE ddsc)
+endif()
+install(


We shouldn't install this, it is strictly for testing. I now see we also install the Cyclone-based one, that's also wrong.

It is somehow necessary to install it in a static build because otherwise CMake doesn't accept the reference to the psmx_dummy library when constructing libc.a, but at the same time it appears installing an OBJECT library doesn't actually install anything. In short, it seems one can get away with that.

When a shared library, installing it really copies a file, but there is also no need to install it. So I think it should be:

if(BUILD_SHARED_LIBS) target_link_libraries(psmx_dummy PRIVATE ddsc) else() install(...) endif()

eboasson · 2024-04-10T13:14:16Z

src/core/ddsc/tests/psmx.c

@@ -29,6 +30,7 @@

 #include "config_env.h"
 #include "test_common.h"
+#include "psmx_dummy_public.h"


This include makes me think it would be better to move these new tests to a separate file. There is quite a difference between tests operating independently from the specific plugin, and tests requiring detailed knowledge of the plugin. If they are all in the same file, it takes effort to figure out the dependencies; if they are instead put in a different file, you avoid that problem.

eboasson · 2024-04-10T13:16:27Z

src/core/ddsc/tests/psmx.c

@@ -62,17 +64,35 @@ static void fail_too_much_data (void) { fail (); }
 #define TRACE_CATEGORY "discovery"
 #endif

-static dds_entity_t create_participant (dds_domainid_t int_dom)
+/**
+ * @brief Create a participant object


So the only thing this documentation comment says that somewhat useful is NULL if not used. Clearly the function does quite a bit and so I think a documentation comment should provide a more detailed explanation of its purpose than this, or there simply shouldn't be a documentation comment at all.

I also see that the value passed in for locator is psmx_locators[...].a in all the "old" cases, and only exists for passing NULL in once and passing in some other array once. The way this wiggles its way into the function is quite ugly (for example, the -1 to handle the psmx%d bit ...)

It really is handling just three cases:

The old (complicated) arrangement with a plugin name taken from the environment, etc. etc.

The dummy-based test with no locator

The dummy-based test with a locator

There is a misunderstanding in what the locator string does, and as a consequence I would say distinguishing between the two latter cases is not even necessary for the purposes of this test. But that is something I'll comment upon at the new test code.

It continues to always take the plugin name from the environment (which makes some sense for when you want to control it from the outside). For the psmx_dummy-based tests, you work around that by using ddsrt_setenv to overrule the setting. That's like taking a so-so construction and making it worse.

If you look at what you actually need for the psmx_dummy plugin, it is next to nothing and so moving the dummy-based tests to a separate file with its own code for setting things up would simpler and arguably be more readable than this parameterised thing.

This was the least intrusive way I could think of to reuse this function. That is, without changing the interface too much, and without changing the behavior in a way that affects the other testcases. But if the dummy psmx testcase goes into another file as suggested in your earlier comment, then there is no reason to use the "ducttape approach" with ddsrt_setenv(), so the problem will solve itself.

eboasson · 2024-04-10T13:38:44Z

src/core/ddsc/tests/psmx.c

@@ -98,6 +118,39 @@ static dds_entity_t create_participant (dds_domainid_t int_dom)
  return pp;
 }

+static bool domain_pin(dds_entity_t domain, dds_domain** dom_out)


I don't think it is worth the effort to create this function: it is used exactly twice, once for pinning the domain and checking the number of PSMX plugins, and once for getting the locator. If the second one were to operate on a dds_domain pointer it could easily be done together with the check for the number of plugins and there'd only be a single call-site.

It is also totally reasonable to require the operation to succeed. Sanity checking it is always good, but a simple:

rc = dds_entity_pin(domain, &x); assert(rc == 0 && dds_entity_kind(x) == DDS_KIND_DOMAIN);

suffices, with the assert only being nice-to-have.

eboasson · 2024-04-10T13:39:11Z

src/core/ddsc/tests/psmx.c

+  dds_entity_unpin((dds_entity*)dom);
+}
+
+static bool domain_get_psmx_locator(dds_entity_t domain, ddsi_locator_t* l_out)


See comment at domain_pin

eboasson · 2024-04-10T14:34:50Z

src/core/ddsc/tests/psmx_dummy_impl.c

+
+//### Helper functions ###
+
+static char* get_config_option_value (const char* conf, const char* option_name)


I'd leave this out. No need to parse the configuration, there is only a need to show that the configuration string is passed in correctly.

Everything else can easily be handled by global variables (see comments above, on accessing the statistics).

eboasson · 2024-04-10T14:35:47Z

src/core/ddsc/tests/psmx_dummy_impl.c

+
+//### Dynamic library functions ###
+
+static dummy_mockstats_t* g_mockstats;


So I'd simplify this by having not a global pointer to a dummy_mockstats_t, but just a global dummy_mockstats_t. (The owned one can then be deleted.)

eboasson · 2024-04-10T14:36:25Z

src/core/ddsc/tests/psmx_dummy_impl.c

+static dummy_mockstats_t* g_mockstats;
+static bool g_mockstats_owned;
+
+static bool dummy_psmx_type_qos_supported(


Stylistically, I think it makes more sense to define the structs with function pointers at the end, that saves a ton of function prototypes here.

eboasson · 2024-04-10T14:38:50Z

src/core/ddsc/tests/psmx_dummy_impl.c

+  dds_data_type_properties_t data_type_props
+) {
+  (void)data_type_props;
+  struct dds_psmx_topic* topic = dds_alloc(sizeof(struct dds_psmx_topic));


I think it is worth considering creating two topics in the test, and mapping these to two global variables for dds_psmx_topic. That way you can extend the test application to inform the library which topic should show up in the next operation, and then verify that it does.

Am I interpreting this correctly that the test code should prepare some data in the global variable (which can be accessed by both the test code and the plugin), before calling dds_create_topic(), and then let the plugin verify from inside dummy_psmx_create_topic(), that it gets a request for the correct topic?

eboasson · 2024-04-10T14:40:35Z

src/core/ddsc/tests/psmx_dummy_impl.c

+  dds_psmx_endpoint_type_t endpoint_type
+) {
+  (void)qos;
+  struct dds_psmx_endpoint* endp = dds_alloc(sizeof(struct dds_psmx_endpoint));


Same idea as dummy_psmx_create_topic, though perhaps you'd want four of these if there are two topics.

mvandenhoek · 2024-04-16T16:07:17Z

Thank you for this @mvandenhoek but I'm afraid I have some different ideas on how to go about testing whether the functions on the PSMX interface get invoked (and the results used) correctly by the core library. I've tried to sketch these ideas in the comments.

I won't say it has to be done the way I sketched, but on the whole I think it makes more sense the approach you chose, specifically also because it makes fewer assumptions and keeps the different tests a bit cleaner.

I believe these changes address your comments.

eboasson · 2024-04-30T13:57:27Z

src/core/ddsc/tests/psmx_dummy_impl.c

+
+static dummy_mockstats_t g_mockstats;
+
+static dummy_mockstats_t* dummy_mockstats_get_ptr()


Why not export this one in the same way as the two alloc functions?

eboasson · 2024-04-30T13:57:42Z

src/core/ddsc/tests/psmx_dummy_public.h

+  dynamic_array_t endpoints;
+}dummy_mockstats_t;
+
+typedef struct dummy_psmx {


This one should be private.

eboasson · 2024-04-30T13:58:54Z

src/core/ddsc/tests/psmxif.c

+
+static void free_strings(uint32_t len, char** strings)
+{
+    if (len != 0 && strings != NULL) {


I generally don't care all that much about 2 vs 4 space (vs tab ...) indentation, but I do find it a bit annoying if it changes unnecessarily in one file. I would appreciate it if you can make this one use 2-space indentation as well.

eboasson · 2024-04-30T14:00:05Z

src/core/ddsc/tests/psmxif.c

+{
+  char *configstr;
+  char locator_str[74];
+  if ( locator == NULL ) {


These complications with locators are no longer used in any meaningful way, they should be removed. A single, fixed, configuration string suffices for the purpose of the test.

eboasson · 2024-04-30T14:01:05Z

src/core/ddsc/tests/psmxif.c

+</General>\
+<Discovery>\
+  <Tag>${CYCLONEDDS_PID}</Tag>\
+  <ExternalDomainId>0</ExternalDomainId>\


This just runs a single instance of Cyclone, no need for the tricks that allow you to pretend there's a network in between when everything is in a single process.

eboasson · 2024-04-30T14:18:37Z

src/core/ddsc/tests/psmxif.c

+      CU_ASSERT_FATAL(reader1 > 0);
+      {
+        // Check the dummy psmx instance_name.
+        dds_qos_t* qos = dds_create_qos();


Why check this here, but not for the writer?

From a test structure perspective, it would also make more sense to check these in a separate test. With appropriately factored out entity creation code it takes only a few lines extra to turn it into a separate test case.

This is now in a separate test.

eboasson · 2024-04-30T14:21:08Z

src/core/ddsc/tests/psmxif.c

+        dds_return_t rc = dds_entity_pin(reader1, &x);
+        CU_ASSERT_FATAL(rc == DDS_RETCODE_OK && dds_entity_kind(x) == DDS_KIND_READER);
+        struct dds_psmx_endpoints_set* endpt_set = &((dds_reader*)x)->m_endpoint.psmx_endpoints;
+        CU_ASSERT_FATAL(endpt_set->length == 1 && endpt_set->endpoints[0] == psmx_endpt_expected);


I think the only practical way to verify this in my preferred implementation is to check in the plug-in on deleting the reader. But that needs to be done anyway.

eboasson · 2024-04-30T14:21:30Z

src/core/ddsc/tests/psmxif.c

+        dds_entity_unpin(x);
+      }
+
+      create_unique_topic_name("shared_memory", topicname, sizeof(topicname));


All things topic2: more of the same.

eboasson · 2024-04-30T14:23:22Z

src/core/ddsc/tests/psmxif.c

+      // Check that shared memory is enabled when it should, and not enabled when it shouldn't.
+      bool psmx_enabled;
+      psmx_enabled = endpoint_has_psmx_enabled(writer1);
+      CU_ASSERT_FATAL(psmx_enabled);


This is covered already: the preceding tests are stricter than this one. (I would like those preceding ones done differently, but the net effect would still be good enough to say that this test is superfluous.)

eboasson · 2024-04-30T14:29:46Z

src/core/ddsc/tests/psmxif.c

+      bool psmx_enabled;
+      psmx_enabled = endpoint_has_psmx_enabled(writer1);
+      CU_ASSERT_FATAL(psmx_enabled);
+      CU_ASSERT_FATAL(dds_is_shared_memory_available(writer1) == supports_shared_memory_expected);


supports_shared_memory_expected is semantically a variable, but it is assigned only once, at declaration and its address is never taken. So it is a constant. I would prefer CU_ASSERT_FATAL(dds_is_shared_memory_available(writer1)) in such a case.

However, here, you're trying to test dds_is_shared_memory_available but with the current set-up, an implementation of dds_is_shared_memory_available that simply returns true will pass. It is therefore not a good test.

A much better way would be to:

Create a reader/writer that does not involve PSMX, verify that it returns false. (This would best be done in a different test.)

Run this test once with the plugin claiming to support shared memory and verifying it returns true. (This is the current case.)

Run this test once with the plugin claiming to not support shared memory and verifying it returns false. (This case is quite obviously missing.)

With the "direct" access to the dummy plugin, it is trivial to make the plugin claim one or the other.

…ocator can be set Signed-off-by: Michel van den Hoek <michel.vandenhoek@zettascale.tech>

Signed-off-by: Michel van den Hoek <michel.vandenhoek@zettascale.tech>

mvandenhoek force-pushed the ddsc_shared_memory_test_branch branch 3 times, most recently from 74d8d92 to 2f44e5d Compare March 1, 2024 09:03

firuze-cagliyan approved these changes Mar 5, 2024

View reviewed changes

dpotman reviewed Mar 6, 2024

View reviewed changes

mvandenhoek force-pushed the ddsc_shared_memory_test_branch branch from c6e507e to 0627d86 Compare April 3, 2024 11:39

mvandenhoek force-pushed the ddsc_shared_memory_test_branch branch 12 times, most recently from c19c331 to 00d376b Compare April 4, 2024 13:24

mvandenhoek requested review from dpotman and firuze-cagliyan April 8, 2024 14:45

eboasson reviewed Apr 10, 2024

View reviewed changes

mvandenhoek force-pushed the ddsc_shared_memory_test_branch branch from 00d376b to 99a7b81 Compare April 16, 2024 16:04

mvandenhoek force-pushed the ddsc_shared_memory_test_branch branch 2 times, most recently from c4d55b0 to 8148bae Compare April 16, 2024 18:21

eboasson reviewed Apr 30, 2024

View reviewed changes

mvandenhoek added 2 commits May 3, 2024 17:15

add psmx test to check whether shared memory can be enabled and the l…

608328d

…ocator can be set Signed-off-by: Michel van den Hoek <michel.vandenhoek@zettascale.tech>

process review of 'add psmx test...'

6ee0071

Signed-off-by: Michel van den Hoek <michel.vandenhoek@zettascale.tech>

mvandenhoek added 3 commits May 3, 2024 17:15

add dummy psmx interface

4901b3d

Signed-off-by: Michel van den Hoek <michel.vandenhoek@zettascale.tech>

process review of 'add dummy psmx interface'

d1dc466

Signed-off-by: Michel van den Hoek <michel.vandenhoek@zettascale.tech>

process review2 of 'add dummy psmx interface'

22f2a7e

Signed-off-by: Michel van den Hoek <michel.vandenhoek@zettascale.tech>

mvandenhoek force-pushed the ddsc_shared_memory_test_branch branch from 8148bae to 22f2a7e Compare May 3, 2024 15:16


		//### Helper functions ###

		static char* get_config_option_value (const char* conf, const char* option_name)


		//### Dynamic library functions ###

		static dummy_mockstats_t* g_mockstats;


		static dummy_mockstats_t g_mockstats;

		static dummy_mockstats_t* dummy_mockstats_get_ptr()

add psmx test to check whether shared memory can be enabled and the l… #1939

Are you sure you want to change the base?

add psmx test to check whether shared memory can be enabled and the l… #1939

Conversation

mvandenhoek commented Feb 29, 2024

firuze-cagliyan left a comment

Choose a reason for hiding this comment

dpotman left a comment

Choose a reason for hiding this comment

dpotman Mar 6, 2024 • edited

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

Choose a reason for hiding this comment

mvandenhoek commented Mar 19, 2024

mvandenhoek commented Apr 3, 2024

eboasson left a 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

mvandenhoek Apr 10, 2024 • edited

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

mvandenhoek commented Apr 16, 2024

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dpotman Mar 6, 2024 •

edited

mvandenhoek Apr 10, 2024 •

edited