diff mbox

[v2,1/2] api: buffer_pool: Correct buf_size pool param documentation

Message ID A400FC85CF2669428A5A081F01B94F531D9D5B14@DEMUMBX012.nsn-intra.net
State New
Headers show

Commit Message

Savolainen, Petri (NSN - FI/Espoo) Jan. 5, 2015, 12:22 p.m. UTC 
From: ext Bill Fischofer [mailto:bill.fischofer@linaro.org]

Sent: Tuesday, December 23, 2014 2:58 PM
To: Petri Savolainen
Cc: lng-odp-forward
Subject: Re: [lng-odp] [PATCH v2 1/2] api: buffer_pool: Correct buf_size pool param documentation



On Tue, Dec 23, 2014 at 5:29 AM, Petri Savolainen <petri.savolainen@linaro.org<mailto:petri.savolainen@linaro.org>> wrote:
Buf_size parameter defines minimum buffer/segment length.
Use 0 for default length.

Signed-off-by: Petri Savolainen <petri.savolainen@linaro.org<mailto:petri.savolainen@linaro.org>>

---
 .../linux-generic/include/api/odp_buffer_pool.h    | 28 +++++++++++++---------
 1 file changed, 17 insertions(+), 11 deletions(-)

+                                this is the total number of segments and the
+                                maximum number of packets (in case that all
+                                packets have a single segment). */

This is not what the implementation needs to know.  A buffer is not a segment since a buffer is what anchors the metadata associated with the object.  Whether or not we introduce packet segment metadata, packets (single object) have metadata associated with them that are independent of the number of segments they occupy. Conflating these concepts doesn't simplify anything and just confuses the terminology and APIs.

Implementation needs to know this. As it says above, in the worst case all packets in the pool have only single segment and thus the max number of packet metadata objects  is ‘num_bufs’. Number of segments per packet is not a constant, it depends on received packet lengths (and fluctuates with those)

It’s an implementation detail if buffer/packet metadata is stored separately from actual buffer space (or next to it). Typically it’s stored next to the buffer space so that the first cache line of the packet covers both the metadata and the first N bytes of packet data/headroom. When packet consists of multiple segments, the first segment holds per packet meta-data, other segments hold just pointers to the data and next segments. The previous linux-generic implementation was designed like that.


-Petri


        int      buf_type;  /**< Buffer type */
 } odp_buffer_pool_param_t;

--
2.2.1


_______________________________________________
lng-odp mailing list
lng-odp@lists.linaro.org<mailto:lng-odp@lists.linaro.org>
http://lists.linaro.org/mailman/listinfo/lng-odp
diff mbox

Patch

diff --git a/platform/linux-generic/include/api/odp_buffer_pool.h b/platform/linux-generic/include/api/odp_buffer_pool.h
index 8380ac1..9329405 100644
--- a/platform/linux-generic/include/api/odp_buffer_pool.h
+++ b/platform/linux-generic/include/api/odp_buffer_pool.h
@@ -35,19 +35,25 @@  extern "C" {
 /**
  * Buffer pool parameters
  * Used to communicate buffer pool creation options.
+ *
+ * @see ODP_CONFIG_PACKET_BUF_LEN_MIN, ODP_CONFIG_BUFFER_ALIGN_MIN,
+ * ODP_CONFIG_BUFFER_ALIGN_MAX
  */
 typedef struct odp_buffer_pool_param_t {
-       uint32_t buf_size;  /**< Buffer size in bytes.  The maximum
-                              number of bytes application will
-                              store in each buffer. For packets, this
-                              is the maximum packet data length, and
-                              configured headroom and tailroom will be
-                              added to this number */
-       uint32_t buf_align; /**< Minimum buffer alignment in bytes.
-                              Valid values are powers of two.  Use 0
-                              for default alignment.  Default will
-                              always be a multiple of 8. */
-       uint32_t num_bufs;  /**< Number of buffers in the pool */
+       uint32_t buf_size;  /**< Minimum buffer size in bytes. For packets,
+                                this is the minimum segment buffer length,
+                                which includes possible head-/tailroom bytes.
+                                Use 0 for the default size of the buffer type
+                                (e.g. for timeouts or min packet segment
+                                length).*/

I continue to have difficulty with understanding how the implementation is expected to use this interpretation of buf_size to calculate how much storage should be reserved for the buffer pool.  The implementation needs to know this and under the former definition that was straightforward since the application was specifying how large each buffer may be.  Minimums do not do this.

Application specifies the minimum size (e.g. 100 bytes). Implementation can round it up to a sensible value (e.g. 128 bytes). odp_buffer_size() and  odp_packet_seg_buf_len() return the actual buffer length (e.g. 128 bytes). Pool size is num_bufs * (buf_size + round up + internal headers, per buffer overheads, etc).


+       uint32_t buf_align; /**< Minimum buffer alignment in bytes. Valid values
+                                are powers of two.  Use 0 for default
+                                alignment.  Default will always be a multiple
+                                of 8. */
+       uint32_t num_bufs;  /**< Number of buffers in the pool. For packets,