On Fri, Dec 9, 2016 at 9:52 PM, Bala Manoharan
<bala.manoha...@linaro.org> wrote:
> Regards,
> Bala
>
>
> On 10 December 2016 at 02:06, Bill Fischofer <bill.fischo...@linaro.org>
> wrote:
>> Is this supposed to be instead of queue groups or in addition to queue
>> group support?
>
> Yes. Basically the changes is that this proposal merges queue group
> and queue together.
The main problem I see with this approach is that it is not possible
for different PMRs to be feeding the same group of queues, which is
the only way to achieve "OR" functionality with the ODP classifier
architecture. Normally a user-provided queue can be the target of
multiple classes of service, and having a formal queue group object
would permit that to be extended to groups of queues. So this doesn't
seem to be a full functional replacement for queue groups.
>
>>
>> On Fri, Dec 9, 2016 at 5:54 AM, Balasubramanian Manoharan
>> <bala.manoha...@linaro.org> wrote:
>>> Adds support to spread packet from a single CoS to multiple queues by
>>> configuring hashing at CoS level.
>>>
>>> Signed-off-by: Balasubramanian Manoharan <bala.manoha...@linaro.org>
>>> ---
>>> include/odp/api/spec/classification.h | 45
>>> ++++++++++++++++++++++++++++++++---
>>> 1 file changed, 42 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/include/odp/api/spec/classification.h
>>> b/include/odp/api/spec/classification.h
>>> index 0e442c7..220b029 100644
>>> --- a/include/odp/api/spec/classification.h
>>> +++ b/include/odp/api/spec/classification.h
>>> @@ -126,6 +126,12 @@ typedef struct odp_cls_capability_t {
>>> /** Maximum number of CoS supported */
>>> unsigned max_cos;
>>>
>>> + /** Maximun number of queue supported per CoS */
>>
>> typo: queues. Having the variable being max_queue_supported is OK, but
>> a better name might be cos_max_queues or max_cos_queues to be explicit
>> about this being a CoS limit.
>>
>>> + unsigned max_queue_supported;
>>> +
>>> + /** Protocol header combination supported for Hashing */
>>> + odp_pktin_hash_proto_t hash_supported;
>>> +
>>> /** A Boolean to denote support of PMR range */
>>> odp_bool_t pmr_range_supported;
>>> } odp_cls_capability_t;
>>> @@ -164,9 +170,25 @@ typedef enum {
>>> * Used to communicate class of service creation options
>>> */
>>> typedef struct odp_cls_cos_param {
>>> - 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 */
>>> + /* Minimum number of queues to be linked to this CoS.
>>> + * If the number is greater than 1 then hashing has to be
>>> + * enabled */
>>> + uint32_t num_queue;
>>
>> If an application wishes to assign a queue itself (as is done today)
>> then is this set to 0 or 1? This should be stated explicitly. For
>> consistency I'd think we'd have 0 = I'm supplying my own queue, >0 =
>> implementation should create num_queues.
>
> I thought about this also but having number of queues as 0 seems
> contradicting to the basic behaviour of class of service.
>
>>
>>> + /* Denotes whether hashing is enabled for queues in this CoS
>>> + * When hashing is enabled the queues are created by the
>>> implementation
>>> + * and application need not configure any queue to the class of
>>> service
>>> + */
>>> + odp_bool_t enable_hashing;
>>
>> Why is this needed if it must be specified if num_queues >1 (and
>> presumably shouldn't be specified otherwise)? This seems redundant.
>
> IMO, I understand but having a boolean for enabling or disabling
> hashing seemed cleaner since hashing is required to support more than
> one queues.
Isn't that achieved by saying that the has must be specified if
num_queues > 1? If I specify enable_hashing but don't specify a hash
field then that would be equally invalid, so again this bit adds no
additional information.
>
>>
>>> + /* Protocol header fields which are included in packet input
>>> + * hash calculation */
>>> + odp_pktin_hash_proto_t hash;
>>> + /* If hashing is disabled, then application has to configure
>>> + * this queue and packets are delivered to this queue */
>>> + odp_queue_t queue;
>>
>> Again, this seems like it would be clearer to state that this is
>> supplied by the user if num_queues = 0 and ignored otherwise.
>>
>>> + /* 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 +231,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);
>>
>> What is the use case for this API? It's not obvious why this would be needed.
>
> This mechanism is useful when the application wants to initialise any
> context by knowing what will be the queue handle for expected packet
> headers. This is required since the queues are created by the
> implementation during hashing and application does not know the queue
> handle beforehand. Maybe I will update the documentation.
That seems very awkward, because now in addition to the
odp_queue_create() call I have to invent pseudo-packets to pass to
this routine to retrieve a queue result just so I can call
odp_queue_context_set() for that queue? Better to pass a large context
field and have odp_queue_create() divide that into individual queue
contexts as part of the create itself. That's basically what we
assumed would be done for odp_queue_group_create().
>
> -Bala
>>
>>> +
>>> +/**
>>> * Discard a class-of-service along with all its associated resources
>>> *
>>> * @param[in] cos_id class-of-service instance.
>>> --
>>> 1.9.1
>>>
