See comments below based on today's discussions.
On Wed, Apr 12, 2017 at 7:58 AM, Matias Elo <matias....@nokia.com> wrote:
> Add fields to odp_pool_info_t for maximum number of packets of any length,
> maximum number of minimum length packets, and maximum number of maximum
> length packets.
>
> odp_pool_param_t documentation is updated to reflect these new fields.
>
> Signed-off-by: Matias Elo <matias....@nokia.com>
> ---
> include/odp/api/spec/pool.h | 43 ++++++++++++++++++++++++++++++
> -------------
> 1 file changed, 30 insertions(+), 13 deletions(-)
>
> diff --git a/include/odp/api/spec/pool.h b/include/odp/api/spec/pool.h
> index c0de195..402c9f2 100644
> --- a/include/odp/api/spec/pool.h
> +++ b/include/odp/api/spec/pool.h
> @@ -181,23 +181,27 @@ typedef struct odp_pool_param_t {
> uint32_t align;
> } buf;
> struct {
> - /** The number of packets that the pool must
> provide
> - that are packet length 'len' bytes or smaller.
> - The maximum value is defined by pool capability
> - pkt.max_num. */
> + /** The exact number of 'len' byte packets that
> the pool
> + must provide. The maximum value is defined by
> pool
> + capability pkt.max_num. Pool is empty after
> + allocating all the 'len' byte packets. Pool
> capacity
> + for other packet lengths may vary. See
> + odp_pool_info_t for details. */
> uint32_t num;
>
The root issue here seems to be to balance two things:
1. The application's desire to control the number of packets that may
reside in a given pool.
2. The implementation's desire to have flexibility in how it organizes the
pool internals given the application's stated needs for pool capacity.
The intent of the original provision of the odp_pool_param_t was to serve
these needs by allowing the application to set the pool capacity in terms
of the number of packets it must be able to hold and a set of hints (len,
max_len) that assist the implementation in allocating sufficient resources
to meet these needs.
The argument now seems to be that asking an ODP implementation to honor a
fixed pkt.num is too onerous, so the question is how best to accommodate
this without giving the implementation complete license to ignore any hints
the application may provide.
The crux of the argument is whether it is reasonable to require an ODP
packet pool to track packet allocation, as opposed to segment allocation
that just happen to be assembled into packets under the covers. I've argued
that this is not an onerous requirement, but others disagree.
>
> - /** Minimum packet length that the pool must
> provide
> - 'num' packets. The number of packets may be
> less
> - than 'num' when packets are larger than 'len'.
> - The maximum value is defined by pool capability
> - pkt.max_len. Use 0 for default. */
> + /** The packet length that the pool must provide
> exactly
> + 'num' packets. The maximum value is defined by
> pool
> + capability pkt.max_len. Pool capacity for other
> + packet lengths may vary. See odp_pool_info_t
> for
> + details. Use 0 for default. */
> uint32_t len;
>
As we discussed today, this wording doesn't quite work. Consider an
implementation that rounds the number of segments physically allocated up
to some higher boundary (required by HW). How is this limit to be enforced
unless the implementation tracks packet allocations rather than segment
allocations (which for the sake of argument we're assuming it's
unreasonable to ask it to do)?
>
> /** Maximum packet length that will be allocated
> from
> the pool. The maximum value is defined by pool
> - capability pkt.max_len. Use 0 for default (the
> - pool maximum). */
> + capability pkt.max_len. Pool capacity for this
> + packet length can be checked from pool info
> + num_max_len (odp_pool_info_t). Use 0 for
> default
>
Why is this additional clause needed?
> + (the pool maximum).*/
> uint32_t max_len;
>
> /** Minimum number of packet data bytes that are
> stored
> @@ -272,8 +276,21 @@ odp_pool_t odp_pool_lookup(const char *name);
> * Used to get information about a pool.
> */
> typedef struct odp_pool_info_t {
> - const char *name; /**< pool name */
> - odp_pool_param_t params; /**< pool parameters */
> + /** Pool name */
> + const char *name;
> + /** Pool parameters */
> + odp_pool_param_t params;
> + /** Packet pool info */
> + struct {
> + /** Maximum number of packets of any length */
> + uint32_t max_num;
> +
>
As we discussed today, applications will probably not be happy if pkt.num
is set to 1000 and max_num or num_min_len returns 1000000. But this is not
prohibited by this specification. So how are we to establish an upper bound
on number of packets without requiring that the implementation track packet
allocation? Of course, if the implementation can track packet allocation to
provide a "reasonable" upper bound, than it can certainly track packet
allocation to set this to pkt.num as the original spec requires.
> + /** Maximum number of minimum length packets */
> + uint32_t num_min_len;
>
Why are both max_num and num_min_len needed? By definition max_num >=
num_min_len, but under what conditions would an implementation return
max_num > num_min_len?
Also, how are packet references handled in this model? We've previously
stated that a reference counts against pkt.num in the pool in which it is
created.
> +
> + /** Maximum number of maximum length packets */
> + uint32_t num_max_len;
> + } pkt;
> } odp_pool_info_t;
>
> /**
> --
> 2.7.4
>
>
