Merge pull request #7207 from devreal/remove_shmem_seg_hdr

awlauria · web-flow · commit 127576603797 · 2020-01-28T13:57:55.000-05:00
Remove unused opal_shmem_seg_hdr_t to retain alignment
diff --git a/opal/mca/shmem/mmap/shmem_mmap_module.c b/opal/mca/shmem/mmap/shmem_mmap_module.c
@@ -136,7 +136,7 @@ shmem_ds_reset(opal_shmem_ds_t *ds_buf)
     ds_buf->seg_id = OPAL_SHMEM_DS_ID_INVALID;
     ds_buf->seg_size = 0;
     memset(ds_buf->seg_name, '\0', OPAL_PATH_MAX);
-    ds_buf->seg_base_addr = (unsigned char *)MAP_FAILED;
+    ds_buf->seg_base_addr = MAP_FAILED;
 }
 
 /* ////////////////////////////////////////////////////////////////////////// */
@@ -306,12 +306,7 @@ segment_create(opal_shmem_ds_t *ds_buf,
     pid_t my_pid = getpid();
     bool space_available = false;
     uint64_t amount_space_avail = 0;
-
-    /* the real size of the shared memory segment.  this includes enough space
-     * to store our segment header.
-     */
-    size_t real_size = size + sizeof(opal_shmem_seg_hdr_t);
-    opal_shmem_seg_hdr_t *seg_hdrp = MAP_FAILED;
+    void *segment = MAP_FAILED;
 
     /* init the contents of opal_shmem_ds_t */
     shmem_ds_reset(ds_buf);
@@ -375,8 +370,7 @@ segment_create(opal_shmem_ds_t *ds_buf,
                        real_file_name);
     }
     /* let's make sure we have enough space for the backing file */
-    if (OPAL_SUCCESS != (rc = enough_space(real_file_name,
-                                           real_size,
+    if (OPAL_SUCCESS != (rc = enough_space(real_file_name, size,
                                            &amount_space_avail,
                                            &space_available))) {
         opal_output(0, "shmem: mmap: an error occurred while determining "
@@ -389,7 +383,7 @@ segment_create(opal_shmem_ds_t *ds_buf,
         hn = opal_gethostname();
         rc = OPAL_ERR_OUT_OF_RESOURCE;
         opal_show_help("help-opal-shmem-mmap.txt", "target full", 1,
-                       real_file_name, hn, (unsigned long)real_size,
+                       real_file_name, hn, (unsigned long)size,
                        (unsigned long long)amount_space_avail);
         goto out;
     }
@@ -403,8 +397,8 @@ segment_create(opal_shmem_ds_t *ds_buf,
         rc = OPAL_ERROR;
         goto out;
     }
-    /* size backing file - note the use of real_size here */
-    if (0 != ftruncate(ds_buf->seg_id, real_size)) {
+    /* size backing file */
+    if (0 != ftruncate(ds_buf->seg_id, size)) {
         int err = errno;
         const char *hn;
         hn = opal_gethostname();
@@ -413,8 +407,7 @@ segment_create(opal_shmem_ds_t *ds_buf,
         rc = OPAL_ERROR;
         goto out;
     }
-    if (MAP_FAILED == (seg_hdrp = (opal_shmem_seg_hdr_t *)
-                                  mmap(NULL, real_size,
+    if (MAP_FAILED == (segment = mmap(NULL, size,
                                        PROT_READ | PROT_WRITE, MAP_SHARED,
                                        ds_buf->seg_id, 0))) {
         int err = errno;
@@ -427,20 +420,11 @@ segment_create(opal_shmem_ds_t *ds_buf,
     }
     /* all is well */
     else {
-        /* -- initialize the shared memory segment -- */
-        opal_atomic_rmb();
-
-        /* init segment lock */
-        opal_atomic_lock_init(&seg_hdrp->lock, OPAL_ATOMIC_LOCK_UNLOCKED);
-        /* i was the creator of this segment, so note that fact */
-        seg_hdrp->cpid = my_pid;
-
-        opal_atomic_wmb();
 
         /* -- initialize the contents of opal_shmem_ds_t -- */
         ds_buf->seg_cpid = my_pid;
-        ds_buf->seg_size = real_size;
-        ds_buf->seg_base_addr = (unsigned char *)seg_hdrp;
+        ds_buf->seg_size = size;
+        ds_buf->seg_base_addr = segment;
         (void)opal_string_copy(ds_buf->seg_name, real_file_name, OPAL_PATH_MAX);
 
         /* set "valid" bit because setment creation was successful */
@@ -474,8 +458,8 @@ segment_create(opal_shmem_ds_t *ds_buf,
      }
     /* an error occured, so invalidate the shmem object and munmap if needed */
     if (OPAL_SUCCESS != rc) {
-        if (MAP_FAILED != seg_hdrp) {
-            munmap((void *)seg_hdrp, real_size);
+        if (MAP_FAILED != segment) {
+            munmap(segment, size);
         }
         shmem_ds_reset(ds_buf);
     }
@@ -504,10 +488,9 @@ segment_attach(opal_shmem_ds_t *ds_buf)
                            "open(2)", "", strerror(err), err);
             return NULL;
         }
-        if (MAP_FAILED == (ds_buf->seg_base_addr = (unsigned char *)
-                              mmap(NULL, ds_buf->seg_size,
-                                   PROT_READ | PROT_WRITE, MAP_SHARED,
-                                   ds_buf->seg_id, 0))) {
+        if (MAP_FAILED == (ds_buf->seg_base_addr = mmap(NULL, ds_buf->seg_size,
+                                                        PROT_READ | PROT_WRITE, MAP_SHARED,
+                                                        ds_buf->seg_id, 0))) {
             int err = errno;
             const char *hn;
             hn = opal_gethostname();
@@ -545,7 +528,7 @@ segment_attach(opal_shmem_ds_t *ds_buf)
     );
 
     /* update returned base pointer with an offset that hides our stuff */
-    return (ds_buf->seg_base_addr + sizeof(opal_shmem_seg_hdr_t));
+    return ds_buf->seg_base_addr;
 }
 
 /* ////////////////////////////////////////////////////////////////////////// */
@@ -563,7 +546,7 @@ segment_detach(opal_shmem_ds_t *ds_buf)
          ds_buf->seg_id, (unsigned long)ds_buf->seg_size, ds_buf->seg_name)
     );
 
-    if (0 != munmap((void *)ds_buf->seg_base_addr, ds_buf->seg_size)) {
+    if (0 != munmap(ds_buf->seg_base_addr, ds_buf->seg_size)) {
         int err = errno;
         const char *hn;
         hn = opal_gethostname();
diff --git a/opal/mca/shmem/posix/shmem_posix_module.c b/opal/mca/shmem/posix/shmem_posix_module.c
@@ -126,7 +126,7 @@ shmem_ds_reset(opal_shmem_ds_t *ds_buf)
     ds_buf->seg_id = OPAL_SHMEM_DS_ID_INVALID;
     ds_buf->seg_size = 0;
     memset(ds_buf->seg_name, '\0', OPAL_PATH_MAX);
-    ds_buf->seg_base_addr = (unsigned char *)MAP_FAILED;
+    ds_buf->seg_base_addr = MAP_FAILED;
 }
 
 /* ////////////////////////////////////////////////////////////////////////// */
@@ -177,11 +177,7 @@ segment_create(opal_shmem_ds_t *ds_buf,
 {
     int rc = OPAL_SUCCESS;
     pid_t my_pid = getpid();
-    /* the real size of the shared memory segment.  this includes enough space
-     * to store our segment header.
-     */
-    size_t real_size = size + sizeof(opal_shmem_seg_hdr_t);
-    opal_shmem_seg_hdr_t *seg_hdrp = MAP_FAILED;
+    void *segment = MAP_FAILED;
 
     /* init the contents of opal_shmem_ds_t */
     shmem_ds_reset(ds_buf);
@@ -203,8 +199,8 @@ segment_create(opal_shmem_ds_t *ds_buf,
         rc = OPAL_ERROR;
         goto out;
     }
-    /* size backing file - note the use of real_size here */
-    else if (0 != ftruncate(ds_buf->seg_id, real_size)) {
+    /* size backing file */
+    else if (0 != ftruncate(ds_buf->seg_id, size)) {
         int err = errno;
         const char *hn;
         hn = opal_gethostname();
@@ -213,9 +209,9 @@ segment_create(opal_shmem_ds_t *ds_buf,
         rc = OPAL_ERROR;
         goto out;
     }
-    else if (MAP_FAILED == (seg_hdrp = (opal_shmem_seg_hdr_t*)mmap(NULL, real_size,
-                                                                   PROT_READ | PROT_WRITE, MAP_SHARED,
-                                                                   ds_buf->seg_id, 0))) {
+    else if (MAP_FAILED == (segment = mmap(NULL, size,
+                                           PROT_READ | PROT_WRITE, MAP_SHARED,
+                                           ds_buf->seg_id, 0))) {
         int err = errno;
         const char *hn;
         hn = opal_gethostname();
@@ -226,20 +222,11 @@ segment_create(opal_shmem_ds_t *ds_buf,
     }
     /* all is well */
     else {
-        /* -- initialize the shared memory segment -- */
-        opal_atomic_rmb();
-
-        /* init segment lock */
-        opal_atomic_lock_init(&seg_hdrp->lock, OPAL_ATOMIC_LOCK_UNLOCKED);
-        /* i was the creator of this segment, so note that fact */
-        seg_hdrp->cpid = my_pid;
-
-        opal_atomic_wmb();
 
         /* -- initialize the contents of opal_shmem_ds_t -- */
         ds_buf->seg_cpid = my_pid;
-        ds_buf->seg_size = real_size;
-        ds_buf->seg_base_addr = (unsigned char *)seg_hdrp;
+        ds_buf->seg_size = size;
+        ds_buf->seg_base_addr = segment;
 
         /* notice that we are not setting ds_buf->name here.  at this point,
          * posix_shm_open was successful, so the contents of ds_buf->name are
@@ -286,8 +273,8 @@ segment_create(opal_shmem_ds_t *ds_buf,
         if (-1 != ds_buf->seg_id) {
             shm_unlink(ds_buf->seg_name);
         }
-        if (MAP_FAILED != seg_hdrp) {
-            munmap((void*)seg_hdrp, real_size);
+        if (MAP_FAILED != segment) {
+            munmap((void*)segment, size);
         }
         /* always invalidate in this error path */
         shmem_ds_reset(ds_buf);
@@ -313,10 +300,9 @@ segment_attach(opal_shmem_ds_t *ds_buf)
                            "open(2)", "", strerror(err), err);
             return NULL;
         }
-        else if (MAP_FAILED == (ds_buf->seg_base_addr =
-                                (unsigned char*)mmap(NULL, ds_buf->seg_size,
-                                                     PROT_READ | PROT_WRITE, MAP_SHARED,
-                                                     ds_buf->seg_id, 0))) {
+        else if (MAP_FAILED == (ds_buf->seg_base_addr = mmap(NULL, ds_buf->seg_size,
+                                                             PROT_READ | PROT_WRITE, MAP_SHARED,
+                                                             ds_buf->seg_id, 0))) {
             int err = errno;
             const char *hn;
             hn = opal_gethostname();
@@ -356,7 +342,7 @@ segment_attach(opal_shmem_ds_t *ds_buf)
     );
 
     /* update returned base pointer with an offset that hides our stuff */
-    return (ds_buf->seg_base_addr + sizeof(opal_shmem_seg_hdr_t));
+    return ds_buf->seg_base_addr;
 }
 
 /* ////////////////////////////////////////////////////////////////////////// */
@@ -374,7 +360,7 @@ segment_detach(opal_shmem_ds_t *ds_buf)
          ds_buf->seg_id, (unsigned long)ds_buf->seg_size, ds_buf->seg_name)
     );
 
-    if (0 != munmap((void*)ds_buf->seg_base_addr, ds_buf->seg_size)) {
+    if (0 != munmap(ds_buf->seg_base_addr, ds_buf->seg_size)) {
         int err = errno;
         const char *hn;
         hn = opal_gethostname();
diff --git a/opal/mca/shmem/shmem_types.h b/opal/mca/shmem/shmem_types.h
@@ -32,7 +32,6 @@
 #define OPAL_SHMEM_TYPES_H
 
 #include "opal_config.h"
-#include "opal/align.h"
 
 #include <stddef.h>
 #include <string.h>
@@ -98,15 +97,6 @@ do {                                                                           \
 
 typedef uint8_t opal_shmem_ds_flag_t;
 
-/* shared memory segment header */
-struct opal_shmem_seg_hdr_t {
-    /* segment lock */
-    opal_atomic_lock_t lock;
-    /* pid of the segment creator */
-    pid_t cpid;
-} __opal_attribute_aligned__(OPAL_ALIGN_MIN);
-typedef struct opal_shmem_seg_hdr_t opal_shmem_seg_hdr_t;
-
 struct opal_shmem_ds_t {
     /* pid of the shared memory segment creator */
     pid_t seg_cpid;
@@ -117,7 +107,7 @@ struct opal_shmem_ds_t {
     /* size of shared memory segment */
     size_t seg_size;
     /* base address of shared memory segment */
-    unsigned char *seg_base_addr;
+    void *seg_base_addr;
     /* path to backing store -- last element so we can easily calculate the
      * "real" size of opal_shmem_ds_t. that is, the amount of the struct that
      * is actually being used. for example: if seg_name is something like:
diff --git a/opal/mca/shmem/sysv/shmem_sysv_module.c b/opal/mca/shmem/sysv/shmem_sysv_module.c
@@ -179,11 +179,7 @@ segment_create(opal_shmem_ds_t *ds_buf,
 {
     int rc = OPAL_SUCCESS;
     pid_t my_pid = getpid();
-    /* the real size of the shared memory segment.  this includes enough space
-     * to store our segment header.
-     */
-    size_t real_size = size + sizeof(opal_shmem_seg_hdr_t);
-    opal_shmem_seg_hdr_t *seg_hdrp = MAP_FAILED;
+    void *segment = MAP_FAILED;
 
     /* init the contents of opal_shmem_ds_t */
     shmem_ds_reset(ds_buf);
@@ -192,10 +188,8 @@ segment_create(opal_shmem_ds_t *ds_buf,
      * being located on a network file system... so no check is needed here.
      */
 
-    /* create a new shared memory segment and save the shmid. note the use of
-     * real_size here
-     */
-    if (-1 == (ds_buf->seg_id = shmget(IPC_PRIVATE, real_size,
+    /* create a new shared memory segment and save the shmid. */
+    if (-1 == (ds_buf->seg_id = shmget(IPC_PRIVATE, size,
                                        IPC_CREAT | IPC_EXCL | S_IRWXU))) {
         int err = errno;
         const char *hn;
@@ -206,7 +200,7 @@ segment_create(opal_shmem_ds_t *ds_buf,
         goto out;
     }
     /* attach to the sement */
-    else if ((void *)-1 == (seg_hdrp = shmat(ds_buf->seg_id, NULL, 0))) {
+    else if ((void *)-1 == (segment = shmat(ds_buf->seg_id, NULL, 0))) {
         int err = errno;
         const char *hn;
         hn = opal_gethostname();
@@ -231,20 +225,11 @@ segment_create(opal_shmem_ds_t *ds_buf,
     }
     /* all is well */
     else {
-        /* -- initialize the shared memory segment -- */
-        opal_atomic_rmb();
-
-        /* init segment lock */
-        opal_atomic_lock_init(&seg_hdrp->lock, OPAL_ATOMIC_LOCK_UNLOCKED);
-        /* i was the creator of this segment, so note that fact */
-        seg_hdrp->cpid = my_pid;
-
-        opal_atomic_wmb();
 
         /* -- initialize the contents of opal_shmem_ds_t -- */
         ds_buf->seg_cpid = my_pid;
-        ds_buf->seg_size = real_size;
-        ds_buf->seg_base_addr = (unsigned char *)seg_hdrp;
+        ds_buf->seg_size = size;
+        ds_buf->seg_base_addr = (unsigned char *)segment;
 
         /* notice that we are not setting ds_buf->name here. sysv doesn't use
          * it, so don't worry about it - shmem_ds_reset took care of
@@ -270,8 +255,8 @@ segment_create(opal_shmem_ds_t *ds_buf,
      */
     if (OPAL_SUCCESS != rc) {
         /* best effort to delete the segment. */
-        if ((void *)-1 != seg_hdrp) {
-            shmdt((char*)seg_hdrp);
+        if ((void *)-1 != segment) {
+            shmdt((char*)segment);
         }
         shmctl(ds_buf->seg_id, IPC_RMID, NULL);
 
@@ -316,7 +301,7 @@ segment_attach(opal_shmem_ds_t *ds_buf)
     );
 
     /* update returned base pointer with an offset that hides our stuff */
-    return (ds_buf->seg_base_addr + sizeof(opal_shmem_seg_hdr_t));
+    return ds_buf->seg_base_addr;
 }
 
 /* ////////////////////////////////////////////////////////////////////////// */
