(#507) Change platform_buffer_create()/destroy() interfaces to platform_buffer_init() / deinit() methods. #508

gapisback · 2022-12-22T00:48:29Z

This commit reworks the interfaces of platform_buffer_create() and platform_buffer_destroy() to now become platform_buffer_init() and platform_buffer_deinit() interfaces.

This removes a dependency on platform_heap_id or platform_heap_handle arg.

This change does not functionally change anything in these methods. Also, this (indirectly) fixes a minor bug in the create function to deal with the (very rare) case of failing to allocate memory from the heap for buffer_handle *bh . Added small unit-test, platform_apis_test, to exercise these changes.

NOTE: This is some common infra-code built in a prototype dev branch for supporting different memory handles.

I'm pulling this piece out on its own for integration to /main, so that this ground work can be laid for integrating future dev-efforts (w/ little less pain).

netlify · 2022-12-22T00:48:33Z

✅ Deploy Preview for splinterdb canceled.

Name	Link
🔨 Latest commit	`3f57698`
🔍 Latest deploy log	https://app.netlify.com/sites/splinterdb/deploys/63be05d639bc1f0008bd100f

gapisback · 2022-12-22T00:49:07Z

Makefile

@@ -428,6 +428,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)
+


Minor new unit-test to exercise the changed APIs.

gapisback · 2022-12-22T00:49:40Z

src/platform_linux/platform.c

-                       platform_heap_handle UNUSED_PARAM(heap_handle),
-                       platform_module_id   UNUSED_PARAM(module_id))
+platform_buffer_create(size_t             length,
+                       platform_heap_id   heap_id,


This arg sets the stage to allow diff heap-ID handles in future.

(As discussed previously, for the in-flight prototype dev code to support use of shared memory for allocation, I am reusing this heap_id as the handle to select the shared-segment ID to allocate from. The change I'm doing here sets up the stage to move to that model, if we ever decide to productize that implementation.

Even if we choose to not go that route, this change by itself is not problematic, and should continue to work correctly.
)

gapisback · 2022-12-22T00:50:03Z

src/platform_linux/platform.c

-   buffer_handle *bh = TYPED_MALLOC(platform_get_heap_id(), bh);
+   buffer_handle *bh = TYPED_MALLOC(heap_id, bh);
+   if (bh == NULL) {
+      return bh;


Minor bug-fix; otherwise old L99 will seg-fault.

gapisback · 2022-12-22T00:51:50Z

src/platform_linux/platform.c

   ret = munmap(bh->addr, bh->length);
   if (ret) {
      return CONST_STATUS(errno);
   }

-   platform_free(platform_get_heap_id(), bh);
+   platform_free(heap_id, *bh_in);


Interface changed (for the same reasons as for the create() method) so that we can use the right heap_id while freeing memory. Otherwise, default platform_get_heap_id() may be NULL, and we may end up not freeing the memory with the proper heap-ID.

gapisback · 2023-01-03T14:55:55Z

src/platform_linux/platform.c

-      if (platform_use_hugetlb) {
-         flags |= MAP_HUGETLB;
-      }
+   int prot  = PROT_READ | PROT_WRITE;


Blocks of code are indented to the left now as we've lost L 66 if() condition. Function will return early on L73 if bh == NULL.

gapisback · 2023-01-03T14:56:59Z

src/platform_linux/platform.c

 platform_status
-platform_buffer_destroy(buffer_handle *bh)
+platform_buffer_destroy(platform_heap_id heap_id, buffer_handle **bh_in)


Changed the interface to receive address of buffer_handle *, so that it can be NULL'ed out on exit.
(This is a safer way to avoid callers accidentally accessing bh_in after this call.

gapisback · 2023-01-03T14:58:10Z

src/rc_allocator.c

@@ -388,7 +388,7 @@ rc_allocator_init(rc_allocator        *al,
 void
 rc_allocator_deinit(rc_allocator *al)
 {
-   platform_buffer_destroy(al->bh);
+   platform_buffer_destroy(al->heap_id, &al->bh);


example of this usage. al->bh will be NULL'ed out upon return.

ajhconway

LGTM

rtjohnso

I would like to propose a different approach:

Change these from create and destroy functions to init and deinit functions.

Then change all users to declare buffer_handles instead of buffer_handle * fields in their structures.

I believe this will solve the problem you are trying to solve.

I discussed this with Alex and he also thinks this would actually be better than the existing code.

src/platform_linux/platform.h

gapisback · 2023-01-07T16:29:49Z

HI @rtjohnso - Based on your suggestion to rework this approach, I made the required changes. Uploaded a new commit, here.

(I will squash the two commits and will clean up the commit messages once you approve this work, and before I integrate this to /main.)

It has certainly simplified the interface and code and solves the dependency on the heap_id argument entirely. This works well to pre-stage this change to absorb use of different memory streams in the future. So, thanks!

rtjohnso

Awesome.

There are two main things I'm requesting:

Clean up the error handling in clockcache_init (see comments).
Audit the code you've changed to see if any additional heap_id or heap_handle parameters or structure fields can be eliminated. I think I flagged them all in comments, but it would be good to double-check.

Thank you!

src/platform_linux/platform.c

rtjohnso · 2023-01-07T21:16:03Z

src/clockcache.c

-   if (!cc->rc_bh) {
+
+   rc = platform_buffer_init(&cc->rc_bh, refcount_size);
+   if (!SUCCESS(rc)) {


The error handling needs to be tightened up.

If either platform_buffer_init fails, then we goto alloc_error, which does clockcache_deinit. And clockcache_deinit unconditionally calls platform_buffer_deinit on both buffers. And platform_buffer_deinit unconditionally calls munmap on the buffer_handle's addr. But, depending on where we fail, none, one, or both of the buffers may have not been inited, so calling munmap could cause an error.

There are two ways to fix this, I think:

Modify clockcache_deinit to check whether the corresponding pointer is NULL before it calls platform_buffer_deinit. So, it would check cc->data before calling platform_buffer_deinit(&cc->bh) and it would check cc->refcount before calling platform_buffer_deinit(&cc->rc_bh).

Modify clockcache_init to have different error handling targets and do the cleanup on its own, i.e. have goto alloc_failure1;, goto alloc_failure2;, etc.

My preference is Option 1, but either is fine.

@rtjohnso - There are quite a few callers to platform_buffer_deinit(), and technically, one has to comb through all instances to make sure that we are not calling this wrongly.

A 3rd approach would be to push the checks to inside of platform_buffer_deinit().

It will try to do the munmap() only if bh->addr is non-NULL. This will solve the error handling issue from clockcache_deinit() and also protect all other instances where platform_buffer_deinit() is being called.

What are your thoughts on this approach (before I implement it).

(And, ooops, ... sorry, I introduced this error handling issue when implementing the change to introduce init() / deinit() interface, without looking too hard at error handling. Sorry!)

@rtjohnso : Re: your comment that "Before the buffer_handle is inited, addr may be garbage, so you cannot safely deinit based on the value of addr."

In the two places we are talking about, both rc_allocator_init() and clockcache_init() do a ZERO_CONTENTS(al) and ZERO_CONTENTS(cc).

So, certainly for the error path going through clockcache_deinit(), we should be sure that cc->bh->addrwill be NULL even beforeinit()` happened.

Unless you can spot something else I am overlooking, seems like pushing the NULL-check to inside platform_buffer_deinit() should work.

Discussed this in PR meeting. Rework this as suggested by @robj. clockcache_deinit() will only call platform_buffer_deinit() only if corresponding pointers are non-NULL.

Remove the fix as applied in the latter function.

src/clockcache.c

src/rc_allocator.c

tests/functional/test_functionality.c

tests/functional/test_splinter_shadow.c

src/clockcache.h

src/rc_allocator.h

rtjohnso · 2023-01-09T04:58:10Z

Before the buffer_handle is inited, addr may be garbage, so you cannot safely deinit based on the value of addr. Best, Rob Message ID: ***@***.***>

…

rtjohnso · 2023-01-09T05:10:00Z

Actually I think it was broken in different ways before, so no worries! Thank you fixing it all up.

…

On Sun, Jan 8, 2023 at 9:01 PM gapisback ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/clockcache.c <#508 (comment)>: > @@ -1841,11 +1843,12 @@ 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)) { (And, ooops, ... sorry, I introduced this error handling issue when implementing the change to introduce init() / deinit() interface, without looking too hard at error handling. Sorry!) — Reply to this email directly, view it on GitHub <#508 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA45FRK6LX3MSDTAVNGS4YTWROLT7ANCNFSM6AAAAAATGFT2QU> . You are receiving this because you were mentioned.Message ID: ***@***.***>

rtjohnso · 2023-01-09T05:46:04Z

That's pretty un-idiomatic of how `_init` functions usually work and makes some weird usage rules. What if I want to use a buffer_handle somewhere else? What if I want one on the stack? Now I have to know that I need to zero it before I can init it? Very error prone.

…

On Sun, Jan 8, 2023 at 9:44 PM gapisback ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/clockcache.c <#508 (comment)>: > @@ -1841,11 +1843,12 @@ 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)) { @rtjohnso <https://github.com/rtjohnso> : Re: your comment that "*Before the buffer_handle is inited, addr may be garbage, so you cannot safely deinit based on the value of addr.*" In the two places we are talking about, both rc_allocator_init() and clockcache_init() do a ZERO_CONTENTS(al) and ZERO_CONTENTS(cc). So, certainly for the error path going through clockcache_deinit(), we should be sure that cc->bh->addrwill be NULL even beforeinit()` happened. Unless you can spot something else I am overlooking, seems like pushing the NULL-check to inside platform_buffer_deinit() should work. — Reply to this email directly, view it on GitHub <#508 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA45FRLHOW4SBR2QRAI4NGLWROQR5ANCNFSM6AAAAAATGFT2QU> . You are receiving this because you were mentioned.Message ID: ***@***.***>

gapisback · 2023-01-09T05:49:33Z

@rtjohnso - I implemented much of the cleanup to rip out heap_handle * args from many places, but chasing other not-needed hh and hid will take me some time ( and a big screen ) ... Will follow-up tomorrow.

gapisback · 2023-01-09T05:58:22Z

This part, Rob - "What if I want one on the stack? Now I have to know that I need to zero it before I can initit? Very error prone." .... I'm not so sure, it's that error prone.

I have been trained and have gotten used to a conventional way of coding where every on-stack structure is always MEMZERO()-ed; i.e. in our code base ZERO_CONTENTS(). For precisely the reason that you want to make sure that all fields are zero'ed out before anything -- including init -- is done with the structure.

I think requiring that on-stack variables, structures are always zero'ed out before use is a stronger discipline to have / enforce.

I can implement it in the ways you are suggesting, but I think then there is more work to chase down all other places where platform_buffer_init() is called and ensure that error handling is done right.

We should probably try to talk this through to resolve the right approach to adopt.

src/clockcache.c

gapisback · 2023-01-09T19:06:57Z

src/clockcache.c

   if (cc->logfile) {
      clockcache_log(0, 0, "deinit %s\n", "");
 #if defined(CC_LOG) || defined(ADDR_TRACING)
      platform_close_log_file(cc->logfile);
 #endif
   }

-   platform_buffer_deinit(&cc->rc_bh);
+   if (cc->lookup) {


I did not bother to arrange these backout codes in reverse order of how they are done in init() method as each backout fragment is protected under an if() check.

So, although, not done in "pure backout form", this code ordering should work.

gapisback · 2023-01-09T19:08:10Z

src/clockcache.c

   platform_buffer_deinit(&cc->bh);
+   platform_buffer_deinit(&cc->rc_bh);


I did not bracket this with a corresponding if (!NULL) check on the member field as I am relying on the bh struct being zero'ed out on declaration.

Current code always conforms to this ... Let's talk this point through ...

gapisback · 2023-01-09T19:12:30Z

Hi, @rtjohnso & @ajhconway -- I am in the middle of amending the code to correctly handle backout error issues, and to purge heap_handle * args, and fn-params that are no longer needed.

The error handling corrections are partially implemented; see this commit.

I paused as this needs some unit-testing and I have provided a sample of such a unit-test here. We need better errno-type error reporting back from individual sub-systems [ here, clockcache ], so the unit-test can verify that the backout was triggered at the expected error location. See, e.g., the logic implemented in new test case test_clockcache_init_fails_to_allocate_lookup_array().

I think there are few open items with this work that are best discussed in-person. Let's try to cover these items on Tue's PR meeting.

gapisback · 2023-01-10T17:36:01Z

src/rc_allocator.c

@@ -351,7 +351,7 @@ rc_allocator_init(rc_allocator      *al,
      return rc;
   }
   // To ensure alignment always allocate in multiples of page size.
-   uint32 buffer_size = cfg->extent_capacity * sizeof(uint8);
+   uint64 buffer_size = cfg->extent_capacity * sizeof(uint8);


Potential bugfix. buffer_size declared as uint32 could overflow, to 0, for very large extent_capacity. (Discovered this while trying to build negative test cases in newly added, incomplete, clockcache_test.c unit test.

gapisback · 2023-01-10T18:58:39Z

src/platform_linux/platform.c

@@ -82,12 +82,13 @@ platform_buffer_init(buffer_handle *bh, size_t length)
   }

   if (platform_use_mlock) {
-      int rc = mlock(bh->addr, length);
-      if (rc != 0) {
+      int mlock_rv = mlock(bh->addr, length);


Rename existing rc which shadows the new instance added at the top of this function (in an earlier commit of this PR cycle).

gapisback · 2023-01-10T18:59:07Z

src/platform_linux/platform.c

         platform_error_log("mlock (%lu bytes) failed with error: %s\n",
                            length,
                            strerror(errno));
         munmap(bh->addr, length);
+         rc = CONST_STATUS(errno);


As suggested by Rob ... this is is better.

gapisback · 2023-01-10T18:59:56Z

tests/unit/btree_stress_test.c

@@ -174,6 +172,7 @@ CTEST_TEARDOWN(btree_stress)
   clockcache_deinit(&data->cc);
   rc_allocator_deinit(&data->al);
   task_system_destroy(data->hid, &data->ts);
+   platform_heap_destroy(&data->hh);


Missing in initial code; not an issue right now ... (Not sure why ASAN-builds are not tripping up ...)

gapisback · 2023-01-10T19:01:19Z

src/platform_linux/platform.c

-   if (ret) {
-      return CONST_STATUS(errno);
-   }
+   if (bh->addr) {


Discuss with @rtjohnso -- is this acceptable to you now?

gapisback · 2023-01-10T19:01:58Z

src/platform_linux/platform.c

-   bh->addr = NULL;
+      bh->addr = NULL;
+   }
+   bh->length = 0;


This and L124 are added so that we can validate the state of resulting output bh in unit-tests.

gapisback · 2023-01-10T19:02:32Z

tests/functional/test_functionality.c

-                   uint64               num_inserts,
-                   uint64               correctness_check_frequency,
-                   task_system         *state,
-                   platform_heap_handle hh,


Deleting this arg caused clang-format to throw a fit.

gapisback · 2023-01-10T19:03:40Z

tests/unit/platform_apis_test.c

+      FILE *dev_null = fopen("/dev/null", "w");
+      ASSERT_NOT_NULL(dev_null);
+      platform_set_log_streams(dev_null, dev_null);
+   }


This block will be simplified with just call to one function when PR #514 checks-in to /main.

gapisback · 2023-01-10T19:04:41Z

tests/unit/platform_apis_test.c

+   size_t length = (1024 * KiB * GiB);
+
+   // This is expected to fail as length is very large
+   rc = platform_buffer_init(&bh, length);


From code inspection and manual testing, the length set on L90 is sufficiently big-enough to cause mmap() to fail w/ an error.

Should this approach be tightened further? (I think this is sufficient.)

gapisback · 2023-01-10T23:08:01Z

@rtjohnso - As per our discussion in the PR review meeting, I reworked the flow in clockcache_deinit() and adjusted the logic accordingly in platform_buffer_deinit().

This latest commit (squashing 2 prev related-commits) should address the last remaining issue.

rtjohnso

Thank you! That cleaned up a lot of stuff.

src/clockcache.c

gapisback · 2023-01-11T00:12:26Z

@rtjohnso - About your very last comment, I'm only adding the extra check that in case we are unable to allocate memory to cc->pincount() via TYPED_ARRAY_ZALLOC(), that we bail, and go to error-backout target. (This is not test code; this is mainline library code.)

This was also (already) being done in /main here, further below, and also in other places in this clockcache_init() function which is allocation memory using TYPED_ARRAY_ZALLOC() or variants.

1860    cc->batch_busy =
1861       TYPED_ARRAY_ZALLOC(cc->heap_id,
1862                          cc->batch_busy,
1863                          cc->cfg->page_capacity / CC_ENTRIES_PER_BATCH);
1864    if (!cc->batch_busy) {
1865       goto alloc_error;
1866    }

…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.

vmwclabot added the cla-not-required label Dec 22, 2022

gapisback linked an issue Dec 22, 2022 that may be closed by this pull request

Rework platform_buffer_create() to take platform_heap_id hid as arg. Fix (minor) latent bug in this routine. #507

Closed

gapisback commented Dec 22, 2022

View reviewed changes

gapisback requested review from ajhconway and rtjohnso December 22, 2022 02:03

gapisback force-pushed the agurajada/507-pf_buffer_create branch from 305839a to 4de46da Compare December 25, 2022 17:32

gapisback commented Jan 3, 2023

View reviewed changes

ajhconway approved these changes Jan 5, 2023

View reviewed changes

gapisback force-pushed the agurajada/507-pf_buffer_create branch from 4de46da to c23b30b Compare January 6, 2023 06:14

rtjohnso requested changes Jan 6, 2023

View reviewed changes

src/platform_linux/platform.h Outdated Show resolved Hide resolved

gapisback force-pushed the agurajada/507-pf_buffer_create branch from c23b30b to b7a2b22 Compare January 7, 2023 16:22

rtjohnso requested changes Jan 7, 2023

View reviewed changes

gapisback commented Jan 9, 2023

View reviewed changes

src/clockcache.c Show resolved Hide resolved

gapisback commented Jan 9, 2023

View reviewed changes

gapisback commented Jan 10, 2023

View reviewed changes

gapisback force-pushed the agurajada/507-pf_buffer_create branch from 4289306 to b588554 Compare January 10, 2023 22:51

gapisback changed the title ~~(#507) Minor rework of platform_buffer_create()/destroy() interfaces.~~ (#507) Minor rework of platform_buffer_create()/destroy() interfaces. Change them to platform_buffer_init() / deinit() methods. Jan 10, 2023

gapisback changed the title ~~(#507) Minor rework of platform_buffer_create()/destroy() interfaces. Change them to platform_buffer_init() / deinit() methods.~~ (#507) Change platform_buffer_create()/destroy() interfaces to platform_buffer_init() / deinit() methods. Jan 10, 2023

gapisback force-pushed the agurajada/507-pf_buffer_create branch from b588554 to f422a16 Compare January 10, 2023 23:05

rtjohnso approved these changes Jan 10, 2023

View reviewed changes

src/clockcache.c Show resolved Hide resolved

gapisback mentioned this pull request Jan 10, 2023

platform_buffer_init() [ was: platform_buffer_create() ] fails silently #26

Open

gapisback force-pushed the agurajada/507-pf_buffer_create branch 2 times, most recently from 1e928ed to bbe8063 Compare January 11, 2023 00:28

gapisback enabled auto-merge (rebase) January 11, 2023 00:31

gapisback force-pushed the agurajada/507-pf_buffer_create branch from bbe8063 to 3f57698 Compare January 11, 2023 00:41

gapisback merged commit 60c7910 into main Jan 11, 2023

gapisback deleted the agurajada/507-pf_buffer_create branch January 11, 2023 01:32

		platform_buffer_deinit(&cc->bh);
		platform_buffer_deinit(&cc->rc_bh);

(#507) Change platform_buffer_create()/destroy() interfaces to platform_buffer_init() / deinit() methods. #508

(#507) Change platform_buffer_create()/destroy() interfaces to platform_buffer_init() / deinit() methods. #508

Conversation

gapisback commented Dec 22, 2022 • edited Loading

netlify bot commented Dec 22, 2022 • edited Loading

✅ Deploy Preview for splinterdb canceled.

Choose a reason for hiding this comment

gapisback Dec 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gapisback Jan 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajhconway left a comment

Choose a reason for hiding this comment

rtjohnso left a comment

Choose a reason for hiding this comment

gapisback commented Jan 7, 2023

rtjohnso 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rtjohnso commented Jan 9, 2023 via email

rtjohnso commented Jan 9, 2023 via email

rtjohnso commented Jan 9, 2023 via email

gapisback commented Jan 9, 2023

gapisback commented Jan 9, 2023

gapisback Jan 9, 2023 • edited Loading

Choose a reason for hiding this comment

gapisback Jan 9, 2023 • edited Loading

Choose a reason for hiding this comment

gapisback commented Jan 9, 2023

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

gapisback commented Jan 10, 2023

rtjohnso left a comment

Choose a reason for hiding this comment

gapisback commented Jan 11, 2023

gapisback commented Dec 22, 2022 •

edited

Loading

netlify bot commented Dec 22, 2022 •

edited

Loading

gapisback Dec 22, 2022 •

edited

Loading

gapisback Jan 3, 2023 •

edited

Loading

gapisback Jan 9, 2023 •

edited

Loading

gapisback Jan 9, 2023 •

edited

Loading