On Wed, Jul 19, 2017 at 9:37 PM, Balasubramanian Manoharan <
bala.manoha...@linaro.org> wrote:
> Enable packet hashing per CoS to be able to distribute incoming packets to
> multiple queues linked with a CoS.
>
> Signed-off-by: Balasubramanian Manoharan <bala.manoha...@linaro.org>
> ---
> v4: incorporates review comments from Petri
>
> include/odp/api/spec/classification.h | 83 ++++++++++++++++++++++++++++++
> ++---
> 1 file changed, 78 insertions(+), 5 deletions(-)
>
> diff --git a/include/odp/api/spec/classification.h b/include/odp/api/spec/
> classification.h
> index 39831b2..315afaa 100644
> --- a/include/odp/api/spec/classification.h
> +++ b/include/odp/api/spec/classification.h
> @@ -4,7 +4,6 @@
> * SPDX-License-Identifier: BSD-3-Clause
> */
>
> -
> /**
> * @file
> *
> @@ -19,12 +18,13 @@
> extern "C" {
> #endif
>
> +#include <odp/api/packet_io.h>
> +#include <odp/api/support.h>
> /** @defgroup odp_classification ODP CLASSIFICATION
> * Classification operations.
> * @{
> */
>
> -
> /**
> * @typedef odp_cos_t
> * ODP Class of service handle
> @@ -126,6 +126,13 @@ typedef struct odp_cls_capability_t {
> /** Maximum number of CoS supported */
> unsigned max_cos;
>
> + /** Maximun number of queue supported per CoS
> + * if the value is 1, then hashing is not supported*/
> + unsigned max_hash_queues;
> +
> + /** Protocol header combination supported for Hashing */
> + odp_pktin_hash_proto_t hash_protocols;
> +
> /** A Boolean to denote support of PMR range */
> odp_bool_t pmr_range_supported;
> } odp_cls_capability_t;
> @@ -164,9 +171,33 @@ typedef enum {
> * Used to communicate class of service creation options
> */
> typedef struct odp_cls_cos_param {
>
Not part of this patch, but shouldn't this be struct odp_cls_cos_param_t
for consistency with other typedef'd structs?
> - odp_queue_t queue; /**< Queue associated with CoS */
> - odp_pool_t pool; /**< Pool associated with CoS */
> - odp_cls_drop_t drop_policy; /**< Drop policy associated with
> CoS */
> + /* Number of queues to be linked to this CoS.
> + * If the number is greater than 1 then hashing has to be
> + * configured. If number is equal to 1 then hashing is disabled
> + * and queue has to be configured by the application.
> + * When hashing is enabled the queues are created by the
> implementation
> + * then application need not configure any queue to the class of
> service
> + * Depening on the implementation this number might be rounded-off
> to
> + * nearest supported value (e.g power of 2)
> + * */
> + uint32_t num_queue;
> +
> + /** Queue parameters */
> + odp_queue_param_t queue_param;
> +
> + /* Protocol header fields which are included in packet input
> + * hash calculation */
> + odp_pktin_hash_proto_t hash_proto;
> +
> + /* If hashing is disabled, then application has to configure
> + * this queue and packets are delivered to this queue */
>
If hashing is enabled, is this field ignored? Doc should say so if yes.
> + odp_queue_t queue;
>
Alternately, if queue is only meaningful when num_queue == 1, should this
be a union? For example:
/** Number of queues associated with this CoS.
* Controls variant parameter mappings needed for optional hashing. */
uint32_t num_queue;
/** Variant mappings based on num_queue value */
union {
/** Mapping used when num_queue == 1 */
odp_queue_t queue; /** The single queue associated with this CoS */
/** Mapping used when num_queue > 1 */
struct {
odp_queue_param_t queue_param; /** Parameters used for
queues created for this CoS */
odp_pktin_hash_proto_t hash_proto; /** How packets are
hashed into queues for this CoS */
};
};
> +
> + /* Pool associated with CoS */
> + odp_pool_t pool;
> +
> + /* Drop policy associated with CoS */
> + odp_cls_drop_t drop_policy;
> } odp_cls_cos_param_t;
>
> /**
> @@ -209,6 +240,23 @@ int odp_cls_capability(odp_cls_capability_t
> *capability);
> odp_cos_t odp_cls_cos_create(const char *name, odp_cls_cos_param_t
> *param);
>
> /**
> + * Queue hash result
> + * Returns the queue within a CoS in which a particular packet will be
> enqueued
> + * based on the packet parameters and hash protocol field configured with
> the
> + * class of service.
> + *
> + * @param cos class of service
> + * @param packet Packet handle
> + *
> + * @retval Returns the queue handle on which this packet will
> be
> + * enqueued.
> + * @retval ODP_QUEUE_INVALID for error case
> + *
> + * @note The packet has to be updated with valid header pointers L2, L3
> and L4.
> + */
> +odp_queue_t odp_queue_hash_result(odp_cos_t cos, odp_packet_t packet);
>
Petri suggested that this be named odp_cls_hash_result() or
odp_cls_dest_queue() since this is a cls API, not a queue API. I agree and
think odp_cls_hash_result() is clearer.
> +
> +/**
> * Discard a class-of-service along with all its associated resources
> *
> * @param[in] cos_id class-of-service instance.
> @@ -245,6 +293,31 @@ int odp_cos_queue_set(odp_cos_t cos_id, odp_queue_t
> queue_id);
> odp_queue_t odp_cos_queue(odp_cos_t cos_id);
>
> /**
> + * Get the number of queues linked with the specific class-of-service
> + *
> + * @param cos_id class-of-service instance.
> + *
> + * @return Number of queues linked with the
> class-of-service.
> + */
> +uint32_t odp_cos_num_queue(odp_cos_t cos_id);
>
Petri suggested this be named odp_cls_num_cos_queues() since odp_cls_... is
the standard prefix for Classifier APIs. Since this is an extension of the
existing odp_cos_queue() API, this name seems OK to me.
> +
> +/**
> + * Get the list of queue associated with the specific class-of-service
> + *
> + * @param[in] cos_id class-of-service instance.
> + *
> + * @param[out] queue Array of queue handles associated
> + * with the class-of-service.
> + *
> + * @param[in] num Maximum number of queue handles to output.
> + *
> + * @return Number of queues linked with CoS
> + * @retval <0 on failure
> + */
> +uint32_t odp_cos_queue_multi(odp_cos_t cos_id, odp_queue_t queue[],
> + uint32_t num);
>
Petri pointed out that the _multi() suffix is reserved for actions that
have multiple inputs, so odp_cos_queues() would be better here.
odp_cls_cos_queues() is also OK, but since this is in the existing
odp_cos_queue() "family", odp_cos_queues() seems fine here.
BTW, the only inconsistency we have in the current CLS API names is in CoS
creation. We have the API odp_cls_cos_create() but odp_cos_destroy().
Either we should have all ODP Classifier APIs use the odp_cls_ prefix, or
if we allow both odp_cls and odp_cos (since cos = class of service, hence
classifier is implicit in that abbreviation) then perhaps we should
deprecate odp_cls_cos_create() and have odp_cos_create() instead.
> +
> +/**
> * Assign packet drop policy for specific class-of-service
> *
> * @param[in] cos_id class-of-service instance.
> --
> 1.9.1
>
>
