[catnip] Memory Pool Abstraction #84

ppenna · 2022-04-12T14:21:48Z

Description

In this PR, I mostly introduce the enhancement that is described in #83.

Here is a digest of the commits:

In commit 2a31472 I drop bad checks that were used in the pointer arithmetic for allocating/releasing mbufs.
In commit dc73ffe I introduce the MemoryPool abstraction.
In commit c31d48b and 729f091 I refactor the code to rely on the MemoryPool abstraction
In commit 837ed50 I drop the indirect memory pool, because it is not required to provide the shallow copy of mbufs (rte_pktmbuf_clone does the job) and it was causing interdependency between several modules.

BrianZill · 2022-04-12T16:12:04Z

Is the title meant to say "[canip]" or "[catnip]"?

BrianZill · 2022-04-12T16:34:36Z

src/catnip/runtime/memory/mbuf.rs

-        unsafe {
-            rte_pktmbuf_free(self.ptr);
-        }
+        MemoryPool::free_mbuf(self.ptr);
        self.ptr = ptr::null_mut();


This seems very un-Rustian. Now that the unsafe code is contained inside MemoryPool::free_mbuf(), it seems strange to be nullifying self.ptr here. I would expect free_mbuf() to be doing this for us, so we don't have access to a bogus pointer upon return.

Somehow we should make the raw pointer null, because it is not managed by Rust. I did not get what you are suggesting. Could you clarify it further?

I was suggesting that free_mbuf() null out self.ptr itself. To really do this right, we (or rather, the MemoryPool) would hide the "*mut rte_mbuf" inside a Rust type of our own creation. We'd pass that type into free_mbuf(), it would take ownership, etc.

But don't worry about it, this is probably good enough for now.

Oh, I got it and agree. I'm refactoring Mbuf so as to make this interface cleaner too. Thanks.

src/catnip/runtime/memory/mempool.rs

src/catnip/runtime/memory/manager.rs

src/catnip/runtime/memory/mempool.rs

BrianZill

Okay.

ppenna added the enhancement Enhancement Request on an Existing Feature label Apr 12, 2022

ppenna requested a review from BrianZill April 12, 2022 14:21

ppenna self-assigned this Apr 12, 2022