diff mbox

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

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

Commit Message

Petri Savolainen Dec. 23, 2014, 11:29 a.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    | 28 +++++++++++++---------
 1 file changed, 17 insertions(+), 11 deletions(-)

Comments

Bill Fischofer Dec. 23, 2014, 12:58 p.m. UTC | #1
On Tue, Dec 23, 2014 at 5:29 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    | 28
> +++++++++++++---------
>  1 file changed, 17 insertions(+), 11 deletions(-)
>
> 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.



> +       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,
> +                                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.


>         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 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).*/
+	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,
+				 this is the total number of segments and the
+				 maximum number of packets (in case that all
+				 packets have a single segment). */
 	int      buf_type;  /**< Buffer type */
 } odp_buffer_pool_param_t;