diff mbox

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

Message ID 1419002899-17314-1-git-send-email-petri.savolainen@linaro.org
State New
Headers show

Commit Message

Petri Savolainen Dec. 19, 2014, 3:28 p.m. UTC
Buf_size parameter defines minimum buffer/segment length.
Use 0 for default length.

Signed-off-by: Petri Savolainen <petri.savolainen@linaro.org>
---
 .../linux-generic/include/api/odp_buffer_pool.h     | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

Comments

Bill Fischofer Dec. 19, 2014, 4:09 p.m. UTC | #1
On Fri, Dec 19, 2014 at 9:28 AM, Petri Savolainen <
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>
> ---
>  .../linux-generic/include/api/odp_buffer_pool.h     | 21
> +++++++++++----------
>  1 file changed, 11 insertions(+), 10 deletions(-)
>
> 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.

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?

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?



> +       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
> 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. */
+	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;