First step to clean up pbuf implementation: add pbuf_alloc_reference() to allocate pbufs referencing external payload; move member initialization to common function; simplify PBUF_POOL chain allocator

src/core/pbuf.c

@@ -151,6 +151,20 @@ void eth_rx_irq()
 volatile u8_t pbuf_free_ooseq_pending;
 #define PBUF_POOL_IS_EMPTY() pbuf_pool_is_empty()
 
+/* Initialize members of struct pbuf after allocation */
+static void
+pbuf_init_alloced_pbuf(struct pbuf *p, void* payload, u16_t tot_len, u16_t len, u8_t type)
+{
+  p->next = NULL;
+  p->payload = payload;
+  p->tot_len = tot_len;
+  p->len = len;
+  p->type = type;
+  p->flags = 0;
+  p->ref = 1;
+  p->if_idx = NETIF_NO_INDEX;
+}
+
 /**
  * Attempt to reclaim some memory from queued out-of-sequence TCP segments
  * if we run out of pool pbufs. It's better to give priority to new packets
@@ -248,11 +262,14 @@ pbuf_pool_is_empty(void)
 struct pbuf *
 pbuf_alloc(pbuf_layer layer, u16_t length, pbuf_type type)
 {
-  struct pbuf *p, *q, *r;
+  struct pbuf *p;
   u16_t offset;
-  s32_t rem_len; /* remaining length */
   LWIP_DEBUGF(PBUF_DEBUG | LWIP_DBG_TRACE, ("pbuf_alloc(length=%"U16_F")\n", length));
 
+  if ((type == PBUF_REF) || (type == PBUF_ROM)) {
+    return pbuf_alloc_reference(NULL, length, type);
+  }
+
   /* determine header offset */
   switch (layer) {
   case PBUF_TRANSPORT:
@@ -282,75 +299,42 @@ pbuf_alloc(pbuf_layer layer, u16_t length, pbuf_type type)
 
   switch (type) {
   case PBUF_POOL:
-    /* allocate head of pbuf chain into p */
+    {
-    p = (struct pbuf *)memp_malloc(MEMP_PBUF_POOL);
+      struct pbuf *q, *last;
-    LWIP_DEBUGF(PBUF_DEBUG | LWIP_DBG_TRACE, ("pbuf_alloc: allocated pbuf %p\n", (void *)p));
+      u16_t rem_len; /* remaining length */
-    if (p == NULL) {
+      p = NULL;
-      PBUF_POOL_IS_EMPTY();
+      last = NULL;
-      return NULL;
+      rem_len = length;
-    }
+      while (rem_len > 0) {
-    p->type = type;
+        q = (struct pbuf *)memp_malloc(MEMP_PBUF_POOL);
-    p->next = NULL;
+        if (q == NULL) {
-    p->if_idx = NETIF_NO_INDEX;
+          PBUF_POOL_IS_EMPTY();
-
+          /* free chain so far allocated */
-    /* make the payload pointer point 'offset' bytes into pbuf data memory */
+          if (p) {
-    p->payload = LWIP_MEM_ALIGN((void *)((u8_t *)p + (SIZEOF_STRUCT_PBUF + offset)));
+            pbuf_free(p);
-    LWIP_ASSERT("pbuf_alloc: pbuf p->payload properly aligned",
+          }
-            ((mem_ptr_t)p->payload % MEM_ALIGNMENT) == 0);
+          /* bail out unsuccessfully */
-    /* the total length of the pbuf chain is the requested size */
+          return NULL;
-    p->tot_len = length;
+        }
-    /* set the length of the first pbuf in the chain */
+        pbuf_init_alloced_pbuf(q, LWIP_MEM_ALIGN((void *)((u8_t *)q + SIZEOF_STRUCT_PBUF + offset)),
-    p->len = LWIP_MIN(length, PBUF_POOL_BUFSIZE_ALIGNED - LWIP_MEM_ALIGN_SIZE(offset));
+          rem_len, LWIP_MIN(rem_len, PBUF_POOL_BUFSIZE_ALIGNED - LWIP_MEM_ALIGN_SIZE(offset)), type);
-    LWIP_ASSERT("check p->payload + p->len does not overflow pbuf",
+        LWIP_ASSERT("pbuf_alloc: pbuf q->payload properly aligned",
-                ((u8_t*)p->payload + p->len <=
+                ((mem_ptr_t)q->payload % MEM_ALIGNMENT) == 0);
-                 (u8_t*)p + SIZEOF_STRUCT_PBUF + PBUF_POOL_BUFSIZE_ALIGNED));
+        LWIP_ASSERT("PBUF_POOL_BUFSIZE must be bigger than MEM_ALIGNMENT",
-    LWIP_ASSERT("PBUF_POOL_BUFSIZE must be bigger than MEM_ALIGNMENT",
+          (PBUF_POOL_BUFSIZE_ALIGNED - LWIP_MEM_ALIGN_SIZE(offset)) > 0 );
-      (PBUF_POOL_BUFSIZE_ALIGNED - LWIP_MEM_ALIGN_SIZE(offset)) > 0 );
+        if (p == NULL) {
-    /* set reference count (needed here in case we fail) */
+          /* allocated head of pbuf chain (into p) */
-    p->ref = 1;
+          p = q;
-
+        } else {
-    /* now allocate the tail of the pbuf chain */
+          /* make previous pbuf point to this pbuf */
-
+          last->next = q;
-    /* remember first pbuf for linkage in next iteration */
+        }
-    r = p;
+        last = q;
-    /* remaining length to be allocated */
+        rem_len -= q->len;
-    rem_len = length - p->len;
+        offset = 0;
-    /* any remaining pbufs to be allocated? */
-    while (rem_len > 0) {
-      q = (struct pbuf *)memp_malloc(MEMP_PBUF_POOL);
-      if (q == NULL) {
-        PBUF_POOL_IS_EMPTY();
-        /* free chain so far allocated */
-        pbuf_free(p);
-        /* bail out unsuccessfully */
-        return NULL;
       }
-      q->type = type;
+      break;
-      q->flags = 0;
-      q->next = NULL;
-      /* make previous pbuf point to this pbuf */
-      r->next = q;
-      /* set total length of this pbuf and next in chain */
-      LWIP_ASSERT("rem_len < max_u16_t", rem_len < 0xffff);
-      q->tot_len = (u16_t)rem_len;
-      /* this pbuf length is pool size, unless smaller sized tail */
-      q->len = LWIP_MIN((u16_t)rem_len, PBUF_POOL_BUFSIZE_ALIGNED);
-      q->payload = (void *)((u8_t *)q + SIZEOF_STRUCT_PBUF);
-      LWIP_ASSERT("pbuf_alloc: pbuf q->payload properly aligned",
-              ((mem_ptr_t)q->payload % MEM_ALIGNMENT) == 0);
-      LWIP_ASSERT("check p->payload + p->len does not overflow pbuf",
-                  ((u8_t*)p->payload + p->len <=
-                   (u8_t*)p + SIZEOF_STRUCT_PBUF + PBUF_POOL_BUFSIZE_ALIGNED));
-      q->ref = 1;
-      /* calculate remaining length to be allocated */
-      rem_len -= q->len;
-      /* remember this pbuf for linkage in next iteration */
-      r = q;
     }
-    /* end of chain */
-    /*r->next = NULL;*/
-
-    break;
   case PBUF_RAM:
     {
       mem_size_t alloc_len = LWIP_MEM_ALIGN_SIZE(SIZEOF_STRUCT_PBUF + offset) + LWIP_MEM_ALIGN_SIZE(length);
@@ -362,50 +346,65 @@ pbuf_alloc(pbuf_layer layer, u16_t length, pbuf_type type)
     
       /* If pbuf is to be allocated in RAM, allocate memory for it. */
       p = (struct pbuf*)mem_malloc(alloc_len);
+      if (p == NULL) {
+        return NULL;
+      }
+      pbuf_init_alloced_pbuf(p, LWIP_MEM_ALIGN((void *)((u8_t *)p + SIZEOF_STRUCT_PBUF + offset)),
+        length, length, type);
+      LWIP_ASSERT("pbuf_alloc: pbuf->payload properly aligned",
+             ((mem_ptr_t)p->payload % MEM_ALIGNMENT) == 0);
+      break;
     }
-
-    if (p == NULL) {
-      return NULL;
-    }
-    /* Set up internal structure of the pbuf. */
-    p->payload = LWIP_MEM_ALIGN((void *)((u8_t *)p + SIZEOF_STRUCT_PBUF + offset));
-    p->len = p->tot_len = length;
-    p->next = NULL;
-    p->type = type;
-
-    LWIP_ASSERT("pbuf_alloc: pbuf->payload properly aligned",
-           ((mem_ptr_t)p->payload % MEM_ALIGNMENT) == 0);
-    break;
-  /* pbuf references existing (non-volatile static constant) ROM payload? */
-  case PBUF_ROM:
-  /* pbuf references existing (externally allocated) RAM payload? */
-  case PBUF_REF:
-    /* only allocate memory for the pbuf structure */
-    p = (struct pbuf *)memp_malloc(MEMP_PBUF);
-    if (p == NULL) {
-      LWIP_DEBUGF(PBUF_DEBUG | LWIP_DBG_LEVEL_SERIOUS,
-                  ("pbuf_alloc: Could not allocate MEMP_PBUF for PBUF_%s.\n",
-                  (type == PBUF_ROM) ? "ROM" : "REF"));
-      return NULL;
-    }
-    /* caller must set this field properly, afterwards */
-    p->payload = NULL;
-    p->len = p->tot_len = length;
-    p->next = NULL;
-    p->type = type;
-    break;
   default:
     LWIP_ASSERT("pbuf_alloc: erroneous type", 0);
     return NULL;
   }
-  /* set reference count */
-  p->ref = 1;
-  /* set flags */
-  p->flags = 0;
   LWIP_DEBUGF(PBUF_DEBUG | LWIP_DBG_TRACE, ("pbuf_alloc(length=%"U16_F") == %p\n", length, (void *)p));
   return p;
 }
 
+/**
+ * @ingroup pbuf
+ * Allocates a pbuf for referenced data.
+ * Referenced data can be volatile (PBUF_REF) or long-lived (PBUF_ROM).
+ *
+ * The actual memory allocated for the pbuf is determined by the
+ * layer at which the pbuf is allocated and the requested size
+ * (from the size parameter).
+ *
+ * @param payload referenced payload
+ * @param length size of the pbuf's payload
+ * @param type this parameter decides how and where the pbuf
+ * should be allocated as follows:
+ *
+ * - PBUF_ROM: It is assumed that the memory used is really
+ *             similar to ROM in that it is immutable and will not be
+ *             changed. Memory which is dynamic should generally not
+ *             be attached to PBUF_ROM pbufs. Use PBUF_REF instead.
+ * - PBUF_REF: It is assumed that the pbuf is only
+ *             being used in a single thread. If the pbuf gets queued,
+ *             then pbuf_take should be called to copy the buffer.
+ *
+ * @return the allocated pbuf.
+ */
+struct pbuf *
+pbuf_alloc_reference(void *payload, u16_t length, pbuf_type type)
+{
+  struct pbuf *p;
+  LWIP_ASSERT("invalid pbuf_type", (type == PBUF_REF) || (type == PBUF_ROM));
+  /* only allocate memory for the pbuf structure */
+  p = (struct pbuf *)memp_malloc(MEMP_PBUF);
+  if (p == NULL) {
+    LWIP_DEBUGF(PBUF_DEBUG | LWIP_DBG_LEVEL_SERIOUS,
+                ("pbuf_alloc_reference: Could not allocate MEMP_PBUF for PBUF_%s.\n",
+                (type == PBUF_ROM) ? "ROM" : "REF"));
+    return NULL;
+  }
+  pbuf_init_alloced_pbuf(p, payload, length, length, type);
+  return p;
+}
+
+
 #if LWIP_SUPPORT_CUSTOM_PBUF
 /**
  * @ingroup pbuf

src/include/lwip/pbuf.h

@@ -240,6 +240,7 @@ void pbuf_free_ooseq(void);
 #define pbuf_init()
 
 struct pbuf *pbuf_alloc(pbuf_layer l, u16_t length, pbuf_type type);
+struct pbuf *pbuf_alloc_reference(void *payload, u16_t length, pbuf_type type);
 #if LWIP_SUPPORT_CUSTOM_PBUF
 struct pbuf *pbuf_alloced_custom(pbuf_layer l, u16_t length, pbuf_type type,
                                  struct pbuf_custom *p, void *payload_mem,