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

Re: [Minios-devel] [UNIKRAFT PATCH v4 03/11] lib/uknetdev: Netbuf chaining





On 11.10.18 12:07, Sharan Santhanam wrote:
Hello Simon,

This patch looks good.

There is a minor comment. Please find the comment inline. It is a redundant if condition. I believe we can fix it while upstreaming.

Reviewed-by: Sharan Santhanam <sharan.santhanam@xxxxxxxxx>

On 10/10/2018 02:00 PM, Simon Kuenzer wrote:
Introduce functions for creating and operating netbuf chains. With
this, network packet data that is scattered in memory can be
described.

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

diff --git a/lib/uknetdev/exportsyms.uk b/lib/uknetdev/exportsyms.uk
index 5473bba..3153a5d 100644
--- a/lib/uknetdev/exportsyms.uk
+++ b/lib/uknetdev/exportsyms.uk
@@ -2,3 +2,6 @@ uk_netbuf_init_indir
  uk_netbuf_alloc_indir
  uk_netbuf_alloc_buf
  uk_netbuf_prepare_buf
+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 295e9eb..c448c6a 100644
--- a/lib/uknetdev/include/uk/netbuf.h
+++ b/lib/uknetdev/include/uk/netbuf.h
@@ -282,6 +282,86 @@ struct uk_netbuf *uk_netbuf_prepare_buf(void *mem, size_t size,
                      uint16_t headroom,
                      size_t privlen, uk_netbuf_dtor_t dtor);
+/**
+ * Connects two netbuf chains
+ * @param headtail
+ *   Last netbuf element of chain that should come first.
+ *   It can also be just a single netbuf.
+ * @param tail
+ *   Head element of the second netbuf chain.
+ *   It can also be just a single netbuf.
+ */
+void uk_netbuf_connect(struct uk_netbuf *headtail,
+               struct uk_netbuf *tail);
+
+/**
+ * Connects two netbuf chains
+ * @param head
+ *   Heading netbuf element of chain that should come first.
+ *   It can also be just a single netbuf.
+ * @param tail
+ *   Head element of the second netbuf chain.
+ *   It can also be just a single netbuf.
+ */
+void uk_netbuf_append(struct uk_netbuf *head,
+              struct uk_netbuf *tail);
+
+/**
+ * Disconnects a netbuf from its chain. The chain will remain
+ * without the removed element.
+ * @param m
+ *   uk_netbuf to be removed from its chain
+ * @returns
+ *   - (NULL) Chain consisted only of m
+ *   - Head of the remaining netbuf chain
+ */
+struct uk_netbuf *uk_netbuf_disconnect(struct uk_netbuf *m);
+
+/*
+ * Iterator helpers for netbuf chains
+ */
+/* head -> tail */
+#define UK_NETBUF_CHAIN_FOREACH(var, head)                \
+    for ((var) = (head); (var) != NULL; (var) = (var)->next)
+
+/* tail -> head (reverse) */
+#define UK_NETBUF_CHAIN_FOREACH_R(var, tail)                \
+    for ((var) = (tail); (var) != NULL; (var) = (var)->prev)
+
+/**
+ * Retrieves the last element of a netbuf chain
+ * @param m
+ *   uk_netbuf that is part of a chain
+ * @returns
+ *   Last uk_netbuf of the chain
+ */
+#define uk_netbuf_chain_last(m)                        \
+    ({                                \
+        struct uk_netbuf *__ret = NULL;                \
+        struct uk_netbuf *__iter;                \
+        UK_NETBUF_CHAIN_FOREACH(__iter, (m))            \
+            __ret = __iter;                    \
+                                    \
+        (__ret);                        \
+    })
+
+/**
+ * Retrieves the first element of a netbuf chain
+ * @param m
+ *   uk_netbuf that is part of a chain
+ * @returns
+ *   First uk_netbuf of the chain
+ */
+#define uk_netbuf_chain_first(m)                    \
+    ({                                \
+        struct uk_netbuf *__ret = NULL;                \
+        struct uk_netbuf *__iter;                \
+        UK_NETBUF_CHAIN_FOREACH_R(__iter, (m))            \
+            __ret = __iter;                    \
+                                    \
+        (__ret);                        \
+    })
+
  #ifdef __cplusplus
  }
  #endif
diff --git a/lib/uknetdev/netbuf.c b/lib/uknetdev/netbuf.c
index 72c349b..5bc2312 100644
--- a/lib/uknetdev/netbuf.c
+++ b/lib/uknetdev/netbuf.c
@@ -188,3 +188,58 @@ struct uk_netbuf *uk_netbuf_prepare_buf(void *mem, size_t size,
                   dtor);
      return m;
  }
+
+struct uk_netbuf *uk_netbuf_disconnect(struct uk_netbuf *m)
+{
+    struct uk_netbuf *remhead = NULL;
+
+    UK_ASSERT(m);
+
+    /* It is possible that m was the first element of the chain.
+     * In such a case we want to return the next element of m as the
+     * remaining head of the chain. If there is no next element
+     * there was no chain.
+     */

Why are we doing this check?
+    if (!remhead)
+        remhead = m->next;

Left-over from some previous code pieces. Sorry, yes it is redundant. I will remove the check.

+
+    /* Reconnect the chains before and after m. */
+    if (m->prev)
+        m->prev->next = m->next;
+    if (m->next)
+        m->next->prev = m->prev;
+
+    /* Disconnect m. */
+    m->prev = NULL;
+    m->next = NULL;
+
+    return remhead;
+}
+
+
+void uk_netbuf_connect(struct uk_netbuf *headtail,
+               struct uk_netbuf *tail)
+{
+    UK_ASSERT(headtail);
+    UK_ASSERT(!headtail->next);
+    UK_ASSERT(tail);
+    UK_ASSERT(!tail->prev);
+
+    headtail->next = tail;
+    tail->prev = headtail;
+}
+
+void uk_netbuf_append(struct uk_netbuf *head,
+              struct uk_netbuf *tail)
+{
+    struct uk_netbuf *headtail;
+
+    UK_ASSERT(head);
+    UK_ASSERT(!head->prev);
+    UK_ASSERT(tail);
+    UK_ASSERT(!tail->prev);
+
+    headtail = uk_netbuf_chain_last(head);
+    headtail->next = tail;
+    tail->prev = headtail;
+}


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