diff mbox

[1/3] api: buffer_pool: Correct buf_size pool param documentation

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

Commit Message

Savolainen, Petri (NSN - FI/Espoo) Dec. 22, 2014, 11:56 a.m. UTC 
From: ext Bill Fischofer [mailto:bill.fischofer@linaro.org]

Sent: Friday, December 19, 2014 6:09 PM
To: Petri Savolainen
Cc: lng-odp-forward
Subject: Re: [lng-odp] [PATCH 1/3] api: buffer_pool: Correct buf_size pool param documentation



On Fri, Dec 19, 2014 at 9:28 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     | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)


Beyond this, as we discussed yesterday, a portable application really has no idea what segment sizes are being used by the underlying implementation, nor should it care.  The only application-observable impact of segmentation is on packet addressability.  As written, this spec precludes an SoC that has fixed-sized HW-managed segments from being ODP compliant.  You've argued that an implementation can restrict acceptable values for this parameter but then the application is just echoing back the ODP_CONFIG variables here, so what is the point of this variable?

Most SoCs do not have tight limitations for segment size. When limits are defined with standard config defines, application can adapt to those limits and  optimize segmentation for its use cases when limitations are loose (in the common case).


The purpose of buf_size and num_bufs is to allow the implementation to calculate the amount of storage it needs to reserve to support the buffer pool.  So if an application says "I want N buffers of maximum size S" that makes sense. Saying "I want N things that will be divided into some unspecified number of segments of size S" is ambiguous because N is no longer determinate (this was one of the design issues with the original buffer pool implementation). So if num_bufs is no longer determinate, what is it's purpose?

It needs more documentation, but for packets it’s total number of segments (for raw buffers and timers it’s number of those things). At the same time, it’s the total number of packets (of single segment).

If it would be the total number of any size packets, segmentation would not improve memory usage (which it should do) since implementations would need to reserve memory for the worst case (all packets being the max size) anyway.

-Petri

+       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 */
        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 4da5f84..27e816d 100644
--- a/platform/linux-generic/include/api/odp_buffer_pool.h
+++ b/platform/linux-generic/include/api/odp_buffer_pool.h
@@ -35,18 +35,19 @@  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 buf_size;  /**< Minimum buffer length in bytes. For packets,
+                                this is the minimum segment buffer length. The
+                                length includes head-/tailroom bytes. Use 0 for
+                                default length. */

This is confusing.  Presumably for buffers of type ODP_BUFFER_TYPE_PACKET this is a synonym for ODP_CONFIG_PACKET_BUF_LEN_MIN.  However, how is a 0 value to be interpreted for other buffer types?  Need to specify that here.  In the current code it is legitimate to specify a buf_size of zero as that means that buffers will consist only of metadata.  Buffers of type ODP_BUFFER_TYPE_TIMEOUT fall into that category, for example. Since this is a buffer structure, not a packet structure, the documentation should reflect that.

User should use zero when

-        the buffer type has only meta-data

-        user is going to use the default segment size for packets (== PACKET_BUF_LEN_MIN)