BTW, we'll also need extensions to the classifier validation test suite to
cover these new APIs as well as User Guide updates. These can be separate
patches if desired.
On Mon, Jul 24, 2017 at 8:21 PM, Bill Fischofer <bill.fischo...@linaro.org>
wrote:
>
>
> 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
>>
>>
>
