Regards,
Bala
On 25 July 2017 at 06:51, 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 */
> };
> };
>
Okay. Will update accordingly.
> +
>> + /* 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.
>
Agreed.
>
>
>> +
>> +/**
>> * 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.
>
This inconsistency can be taken as a separate patch once this gets merged.
>
>
>> +
>> +/**
>> * Assign packet drop policy for specific class-of-service
>> *
>> * @param[in] cos_id class-of-service instance.
>> --
>> 1.9.1
>>
>>
>
