[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Minios-devel] [UNIKRAFT PATCH v4 04/11] lib/uknetdev: Netbuf refcounting and releasing



Hello Simon,

Please find the comments inline.

On 10/10/2018 02:00 PM, Simon Kuenzer wrote:
Introduce reference counting and buffer releasing with Netbufs.

Signed-off-by: Simon Kuenzer <simon.kuenzer@xxxxxxxxx>
---
  lib/uknetdev/exportsyms.uk       |  3 ++
  lib/uknetdev/include/uk/netbuf.h | 60 ++++++++++++++++++++++++++++++++++++++++
  lib/uknetdev/netbuf.c            | 55 ++++++++++++++++++++++++++++++++++++
  3 files changed, 118 insertions(+)

diff --git a/lib/uknetdev/exportsyms.uk b/lib/uknetdev/exportsyms.uk
index 3153a5d..591d46c 100644
--- a/lib/uknetdev/exportsyms.uk
+++ b/lib/uknetdev/exportsyms.uk
@@ -2,6 +2,9 @@ uk_netbuf_init_indir
  uk_netbuf_alloc_indir
  uk_netbuf_alloc_buf
  uk_netbuf_prepare_buf
+uk_netbuf_ref
+uk_netbuf_free_single
+uk_netbuf_free
  uk_netbuf_disconnect
  uk_netbuf_connect
  uk_netbuf_append
diff --git a/lib/uknetdev/include/uk/netbuf.h b/lib/uknetdev/include/uk/netbuf.h
index c448c6a..5b0907c 100644
--- a/lib/uknetdev/include/uk/netbuf.h
+++ b/lib/uknetdev/include/uk/netbuf.h
@@ -283,6 +283,43 @@ struct uk_netbuf *uk_netbuf_prepare_buf(void *mem, size_t 
size,
                                        size_t privlen, uk_netbuf_dtor_t dtor);


The function uk_netbuf_ref can also be defined as inline function.
 /**
+ * Increase reference count of a netbuf including each successive netbuf
+ * element of a chain. Note, preceding chain elements are not visited.
+ * @param m
+ *   head of the uk_netbuf chain to increase refcount of
+ * @returns
+ *   Reference to m
+ */
+struct uk_netbuf *uk_netbuf_ref(struct uk_netbuf *m);
+
+/**
+ * Increase reference count of a single netbuf (irrespective of a chain)
+ * @param m
+ *   uk_netbuf to increase refcount of
+ * @returns
+ *   Reference to m
+ */
+static inline struct uk_netbuf *uk_netbuf_ref_single(struct uk_netbuf *m)
+{
+       UK_ASSERT(m);
+
+       uk_refcount_acquire(&m->refcount);
+       return m;
+}
+

Maybe rename uk_netbuf_refcount_single with  uk_netbuf_refcount_single_get
+/**
+ * Returns the current reference count of a single netbuf
+ * @param m
+ *   uk_netbuf to return the reference count of
+ */
+static inline uint32_t uk_netbuf_refcount_single(struct uk_netbuf *m)
+{
+       UK_ASSERT(m);
+
+       return uk_refcount_read(&m->refcount);
+}
+
+/**
   * Connects two netbuf chains
   * @param headtail
   *   Last netbuf element of chain that should come first.
@@ -317,6 +354,29 @@ void uk_netbuf_append(struct uk_netbuf *head,
   */
  struct uk_netbuf *uk_netbuf_disconnect(struct uk_netbuf *m);
+/**
+ * Decreases the reference count of each element of the chain.
+ * If a refcount becomes 0, the netbuf is disconnected from its chain,
+ * its destructor is called and the memory is free'd according to its
+ * allocation.
+ * @param m
+ *   head of uk_netbuf chain to release
+ * @returns
+ *   Reference to m
+ */
+void uk_netbuf_free(struct uk_netbuf *m);
+
+/**
+ * Decreases the reference count of a single netbuf. If refcount becomes 0,
+ * the netbuf is disconnected from its chain, its destructor is called and
+ * the memory is free'd according to its allocation.
+ * @param m
+ *   uk_netbuf to release
+ * @returns
+ *   Reference to m
+ */
+void uk_netbuf_free_single(struct uk_netbuf *m);
+
  /*
   * Iterator helpers for netbuf chains
   */
diff --git a/lib/uknetdev/netbuf.c b/lib/uknetdev/netbuf.c
index 5bc2312..b624364 100644
--- a/lib/uknetdev/netbuf.c
+++ b/lib/uknetdev/netbuf.c
@@ -189,6 +189,18 @@ struct uk_netbuf *uk_netbuf_prepare_buf(void *mem, size_t 
size,
        return m;
  }
+struct uk_netbuf *uk_netbuf_ref(struct uk_netbuf *m)
+{
+       struct uk_netbuf *iter;
+
+       UK_ASSERT(m);
+
+       UK_NETBUF_CHAIN_FOREACH(iter, m)
+               uk_netbuf_ref_single(iter);
+
+       return m;
+}
+
  struct uk_netbuf *uk_netbuf_disconnect(struct uk_netbuf *m)
  {
        struct uk_netbuf *remhead = NULL;
@@ -243,3 +255,46 @@ void uk_netbuf_append(struct uk_netbuf *head,
        headtail->next = tail;
        tail->prev = headtail;
  }
+
+void uk_netbuf_free_single(struct uk_netbuf *m)
+{
+       struct uk_alloc *a;
+
+       UK_ASSERT(m);
+
+       /* Decrease refcount and call destructor and free up memory
+        * when last reference was released.
+        */
+       if (uk_refcount_release(&m->refcount) == 1) {
+               /* Disconnect this netbuf from the chain. */

Additional note: We need to check in the uk_netbuf_disconnect for refcount to be one. We should avoid disconnecting buffer with multi-user reference. In the free function it done correctly but the uk_netbuf_disconnect is exposed to the user directly.

Probably it wise to add a note on
* uk_netbuf_disconnect
* uk_netbuf_connect
* uk_netbuf_append
regarding the refcount and reference to uk_netbuf

+               uk_netbuf_disconnect(m);
+
+               /* Copy the reference of the allocator in case
+                * the destructor is free'ing up our memory
+                * (e.g., uk_netbuf_init_indir() used).
+                * In such a case `a` should be (NULL), however
+                * we need to access it for a  check after we have
+                * called the destructor.
+                */
+               a = m->_a;
+
+               if (m->dtor)
+                       m->dtor(m);
+               if (a)
+                       uk_free(a, m);
+       }
+}
+
+void uk_netbuf_free(struct uk_netbuf *m)
+{
+       struct uk_netbuf *n;
+
+       UK_ASSERT(m);
+       UK_ASSERT(!m->prev);
+
+       while (m != NULL) {

While freeing the buffers in the chain, there maybe buffers with higher reference count. We need to atleast put define a way to inform the user that we have not freed some buffers in the chain.
+               n = m->next;
+               uk_netbuf_free_single(m);
+               m = n;
+       }
+}


Thanks & Regards
Sharan


_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.