(#507) Rework of platform_buffer_create()/destroy() to init/deinit() …

…interfaces.

This commit reworks the buffer_handle{} interfaces to now become
platform_buffer_init() and platform_buffer_deinit().

Structures that need a buffer_handle{}, declare a nested sub-struct,
which will go through this init / deinit interface to allocate / free
memory using existing mmap() interfaces. This removes the need for
an input 'heap_id / heap_handle' arg to allocate and free memory.
This change does not functionally change anything in these methods.
Added small unit-test, platform_apis_test, to exercise these changes.
Cleanup structures and fns that used to take 'heap_handle *' which
now become unused with this rework. Tighten up backout / error handling
in clockcache_init() and deinit() code-flow.

Co-authored by Rob Johnson, who reworked the entire interfaces as
implemented above, to remove the dependency on 'hid' argument.

Makefile

@@ -443,6 +443,9 @@ $(BINDIR)/$(UNITDIR)/task_system_test: $(UTIL_SYS)
                                        $(OBJDIR)/$(FUNCTIONAL_TESTSDIR)/test_async.o \
                                        $(LIBDIR)/libsplinterdb.so
 
+$(BINDIR)/$(UNITDIR)/platform_apis_test: $(UTIL_SYS)       \
+                                         $(PLATFORM_SYS)
+
 ########################################
 # Convenience mini unit-test targets
 unit/util_test:                    $(BINDIR)/$(UNITDIR)/util_test

src/clockcache.c

@@ -46,7 +46,7 @@
  *
  *      clockcache_log, etc. are used to write an output of cache operations to
  *      a log file for debugging purposes. If CC_LOG is set, then all output is
- *      written, if ADDR_TRACING is set, then only operations which affect
+ *      written. If ADDR_TRACING is set, then only operations which affect
  *      entries with either entry_number TRACE_ENTRY or address TRACE_ADDR are
  *      written.
  *
@@ -1761,14 +1761,13 @@ clockcache_config_init(clockcache_config *cache_cfg,
 }
 
 platform_status
-clockcache_init(clockcache          *cc,   // OUT
-                clockcache_config   *cfg,  // IN
-                io_handle           *io,   // IN
-                allocator           *al,   // IN
-                char                *name, // IN
-                platform_heap_handle hh,   // IN
-                platform_heap_id     hid,  // IN
-                platform_module_id   mid)    // IN
+clockcache_init(clockcache        *cc,   // OUT
+                clockcache_config *cfg,  // IN
+                io_handle         *io,   // IN
+                allocator         *al,   // IN
+                char              *name, // IN
+                platform_heap_id   hid,  // IN
+                platform_module_id mid)  // IN
 {
    int      i;
    threadid thr_i;
@@ -1803,10 +1802,9 @@ clockcache_init(clockcache          *cc,   // OUT
    clockcache_log(
       0, 0, "init: capacity %lu name %s\n", cc->cfg->capacity, name);
 
-   cc->al          = al;
-   cc->io          = io;
-   cc->heap_handle = hh;
-   cc->heap_id     = hid;
+   cc->al      = al;
+   cc->io      = io;
+   cc->heap_id = hid;
 
    /* lookup maps addrs to entries, entry contains the entries themselves */
    cc->lookup =
@@ -1824,12 +1822,14 @@ clockcache_init(clockcache          *cc,   // OUT
       goto alloc_error;
    }
 
+   platform_status rc = STATUS_NO_MEMORY;
+
    /* data must be aligned because of O_DIRECT */
-   cc->bh = platform_buffer_create(cc->cfg->capacity, cc->heap_handle, mid);
-   if (!cc->bh) {
+   rc = platform_buffer_init(&cc->bh, cc->cfg->capacity);
+   if (!SUCCESS(rc)) {
       goto alloc_error;
    }
-   cc->data = platform_buffer_getaddr(cc->bh);
+   cc->data = platform_buffer_getaddr(&cc->bh);
 
    /* Set up the entries */
    for (i = 0; i < cc->cfg->page_capacity; i++) {
@@ -1841,14 +1841,19 @@ clockcache_init(clockcache          *cc,   // OUT
 
    /* Entry per-thread ref counts */
    size_t refcount_size = cc->cfg->page_capacity * CC_RC_WIDTH * sizeof(uint8);
-   cc->rc_bh = platform_buffer_create(refcount_size, cc->heap_handle, mid);
-   if (!cc->rc_bh) {
+
+   rc = platform_buffer_init(&cc->rc_bh, refcount_size);
+   if (!SUCCESS(rc)) {
       goto alloc_error;
    }
-   cc->refcount = platform_buffer_getaddr(cc->rc_bh);
+   cc->refcount = platform_buffer_getaddr(&cc->rc_bh);
+
    /* Separate ref counts for pins */
    cc->pincount =
       TYPED_ARRAY_ZALLOC(cc->heap_id, cc->pincount, cc->cfg->page_capacity);
+   if (!cc->pincount) {
+      goto alloc_error;
+   }
 
    /* The hands and associated page */
    cc->free_hand  = 0;
@@ -1872,36 +1877,53 @@ clockcache_init(clockcache          *cc,   // OUT
    return STATUS_NO_MEMORY;
 }
 
+/*
+ * De-init the resources allocated to initialize a clockcache.
+ * This function may be called to deal with error situations, or a failed
+ * clockcache_init(). So check for non-NULL handles before trying to release
+ * resources.
+ */
 void
 clockcache_deinit(clockcache *cc) // IN/OUT
 {
    platform_assert(cc != NULL);
 
-   /*
-    * Check for non-null cause this is also used to clean up a failed
-    * clockcache_init
-    */
    if (cc->logfile) {
       clockcache_log(0, 0, "deinit %s\n", "");
 #if defined(CC_LOG) || defined(ADDR_TRACING)
       platform_close_log_file(cc->logfile);
 #endif
    }
 
-   if (cc->rc_bh) {
-      platform_buffer_destroy(cc->rc_bh);
+   if (cc->lookup) {
+      platform_free(cc->heap_id, cc->lookup);
    }
+   if (cc->entry) {
+      platform_free(cc->heap_id, cc->entry);
+   }
+
+   debug_only platform_status rc = STATUS_TEST_FAILED;
+   if (cc->data) {
+      rc = platform_buffer_deinit(&cc->bh);
 
-   platform_free(cc->heap_id, cc->entry);
-   platform_free(cc->heap_id, cc->lookup);
-   if (cc->bh) {
-      platform_buffer_destroy(cc->bh);
+      // We expect above to succeed. Anyway, we are in the process of
+      // dismantling the clockcache, hence, for now, can't do much by way
+      // of reporting errors further upstream.
+      debug_assert(SUCCESS(rc), "rc=%s", platform_status_to_string(rc));
+      cc->data = NULL;
    }
-   cc->data = NULL;
-   platform_free_volatile(cc->heap_id, cc->batch_busy);
+   if (cc->refcount) {
+      rc = platform_buffer_deinit(&cc->rc_bh);
+      debug_assert(SUCCESS(rc), "rc=%s", platform_status_to_string(rc));
+      cc->refcount = NULL;
+   }
+
    if (cc->pincount) {
       platform_free_volatile(cc->heap_id, cc->pincount);
    }
+   if (cc->batch_busy) {
+      platform_free_volatile(cc->heap_id, cc->batch_busy);
+   }
 }
 
 /*

src/clockcache.h

@@ -116,14 +116,13 @@ struct clockcache {
 
    uint32              *lookup;
    clockcache_entry    *entry;
-   buffer_handle       *bh;   // actual memory for pages
+   buffer_handle        bh;   // actual memory for pages
    char                *data; // convenience pointer for bh
    platform_log_handle *logfile;
-   platform_heap_handle heap_handle;
    platform_heap_id     heap_id;
 
    // Distributed locks (the write bit is in the status uint32 of the entry)
-   buffer_handle  *rc_bh;
+   buffer_handle   rc_bh;
    volatile uint8 *refcount;
    volatile uint8 *pincount;
 
@@ -157,14 +156,13 @@ clockcache_config_init(clockcache_config *cache_config,
                        uint64             use_stats);
 
 platform_status
-clockcache_init(clockcache          *cc,   // OUT
-                clockcache_config   *cfg,  // IN
-                io_handle           *io,   // IN
-                allocator           *al,   // IN
-                char                *name, // IN
-                platform_heap_handle hh,   // IN
-                platform_heap_id     hid,  // IN
-                platform_module_id   mid);   // IN
+clockcache_init(clockcache        *cc,   // OUT
+                clockcache_config *cfg,  // IN
+                io_handle         *io,   // IN
+                allocator         *al,   // IN
+                char              *name, // IN
+                platform_heap_id   hid,  // IN
+                platform_module_id mid); // IN
 
 void
 clockcache_deinit(clockcache *cc); // IN

src/platform_linux/platform.c

@@ -56,45 +56,49 @@ void
 platform_heap_destroy(platform_heap_handle UNUSED_PARAM(*heap_handle))
 {}
 
-buffer_handle *
-platform_buffer_create(size_t               length,
-                       platform_heap_handle UNUSED_PARAM(heap_handle),
-                       platform_module_id   UNUSED_PARAM(module_id))
+/*
+ * platform_buffer_init() - Initialize an input buffer_handle, bh.
+ *
+ * Certain modules, e.g. the buffer cache, need a very large buffer which
+ * may not be serviceable by the heap. Create the requested buffer using
+ * mmap() and initialize the input 'bh' to track this memory allocation.
+ */
+platform_status
+platform_buffer_init(buffer_handle *bh, size_t length)
 {
-   buffer_handle *bh = TYPED_MALLOC(platform_get_heap_id(), bh);
+   platform_status rc = STATUS_NO_MEMORY;
 
-   if (bh != NULL) {
-      int prot  = PROT_READ | PROT_WRITE;
-      int flags = MAP_PRIVATE | MAP_ANONYMOUS | MAP_NORESERVE;
-      if (platform_use_hugetlb) {
-         flags |= MAP_HUGETLB;
-      }
+   int prot  = PROT_READ | PROT_WRITE;
+   int flags = MAP_PRIVATE | MAP_ANONYMOUS | MAP_NORESERVE;
+   if (platform_use_hugetlb) {
+      flags |= MAP_HUGETLB;
+   }
 
-      bh->addr = mmap(NULL, length, prot, flags, -1, 0);
-      if (bh->addr == MAP_FAILED) {
-         platform_error_log(
-            "mmap (%lu) failed with error: %s\n", length, strerror(errno));
-         goto error;
-      }
+   bh->addr = mmap(NULL, length, prot, flags, -1, 0);
+   if (bh->addr == MAP_FAILED) {
+      platform_error_log(
+         "mmap (%lu bytes) failed with error: %s\n", length, strerror(errno));
+      goto error;
+   }
 
-      if (platform_use_mlock) {
-         int rc = mlock(bh->addr, length);
-         if (rc != 0) {
-            platform_error_log(
-               "mlock (%lu) failed with error: %s\n", length, strerror(errno));
-            munmap(bh->addr, length);
-            goto error;
-         }
+   if (platform_use_mlock) {
+      int mlock_rv = mlock(bh->addr, length);
+      if (mlock_rv != 0) {
+         platform_error_log("mlock (%lu bytes) failed with error: %s\n",
+                            length,
+                            strerror(errno));
+         munmap(bh->addr, length);
+         rc = CONST_STATUS(errno);
+         goto error;
       }
    }
-
    bh->length = length;
-   return bh;
+   return STATUS_OK;
 
 error:
-   platform_free(platform_get_heap_id(), bh);
-   bh = NULL;
-   return bh;
+   // Reset, in case mmap() or mlock() failed.
+   bh->addr = NULL;
+   return rc;
 }
 
 void *
@@ -103,17 +107,23 @@ platform_buffer_getaddr(const buffer_handle *bh)
    return bh->addr;
 }
 
+/*
+ * platform_buffer_deinit() - Deinit the buffer handle, which involves
+ * unmapping the memory for the large buffer created using mmap().
+ * This is expected to be called on a 'bh' that has been successfully
+ * initialized, thru a prior platform_buffer_init() call.
+ */
 platform_status
-platform_buffer_destroy(buffer_handle *bh)
+platform_buffer_deinit(buffer_handle *bh)
 {
    int ret;
    ret = munmap(bh->addr, bh->length);
    if (ret) {
       return CONST_STATUS(errno);
    }
 
-   platform_free(platform_get_heap_id(), bh);
-
+   bh->addr   = NULL;
+   bh->length = 0;
    return STATUS_OK;
 }
 

src/platform_linux/platform.h

@@ -626,16 +626,14 @@ platform_heap_create(platform_module_id    module_id,
 void
 platform_heap_destroy(platform_heap_handle *heap_handle);
 
-buffer_handle *
-platform_buffer_create(size_t               length,
-                       platform_heap_handle heap_handle,
-                       platform_module_id   module_id);
+platform_status
+platform_buffer_init(buffer_handle *bh, size_t length);
 
 void *
 platform_buffer_getaddr(const buffer_handle *bh);
 
 platform_status
-platform_buffer_destroy(buffer_handle *bh);
+platform_buffer_deinit(buffer_handle *bh);
 
 platform_status
 platform_mutex_init(platform_mutex    *mu,