| Message ID | 1481284466-20536-1-git-send-email-bala.manoharan@linaro.org |
|---|---|
| State | New |
| Headers | show |
Is this supposed to be instead of queue groups or in addition to queue group support? On Fri, Dec 9, 2016 at 5:54 AM, Balasubramanian Manoharan <bala.manoharan@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.manoharan@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. > + /* 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. > + /* 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. > + > +/** > * Discard a class-of-service along with all its associated resources > * > * @param[in] cos_id class-of-service instance. > -- > 1.9.1 >
Regards, Bala On 10 December 2016 at 02:06, Bill Fischofer <bill.fischofer@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. > > On Fri, Dec 9, 2016 at 5:54 AM, Balasubramanian Manoharan > <bala.manoharan@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.manoharan@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. > >> + /* 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. -Bala > >> + >> +/** >> * Discard a class-of-service along with all its associated resources >> * >> * @param[in] cos_id class-of-service instance. >> -- >> 1.9.1 >>
On Fri, Dec 9, 2016 at 9:52 PM, Bala Manoharan <bala.manoharan@linaro.org> wrote: > Regards, > Bala > > > On 10 December 2016 at 02:06, Bill Fischofer <bill.fischofer@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.manoharan@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.manoharan@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 >>>
> -----Original Message----- > From: Bill Fischofer [mailto:bill.fischofer@linaro.org] > Sent: Monday, December 12, 2016 3:01 AM > To: Bala Manoharan <bala.manoharan@linaro.org> > Cc: lng-odp-forward <lng-odp@lists.linaro.org>; Savolainen, Petri (Nokia - > FI/Espoo) <petri.savolainen@nokia-bell-labs.com> > Subject: Re: [lng-odp] [RFC/API-NEXT 1/1] api: classification: packet > hashing per class of service > > On Fri, Dec 9, 2016 at 9:52 PM, Bala Manoharan > <bala.manoharan@linaro.org> wrote: > > Regards, > > Bala > > > > > > On 10 December 2016 at 02:06, Bill Fischofer <bill.fischofer@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. May be it's better to add "OR" functionality directly to PMR, instead of implementing it indirectly through multiple copies of the same destination queue (or same set of queue, or the same pool, ...). Currently pmr_create() can be used only for "AND" of terms. We could add a flag into odp_pmr_param_t which enables to define a series of ANDs and ORs. odp_pmr_t odp_cls_pmr_create(const odp_pmr_param_t *terms, int num_terms, odp_cos_t src_cos, odp_cos_t dst_cos); term[0] AND (term[1] OR term[2] OR term[3]) AND term[4] > >>> + /* 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. Implementation may be more efficient when it can create the queue(s). Even when application needs only one queue, it may not any particular destination queue (just an queue). I think the bool is redundant for enable/disable hash, since num > 1 requires hashing. However, it could be defined as create vs. use application provided queues. 'max_queue_supported' could be redefined as 'num_queues', which is the number of queues to be created, or number of queues application provides. It would be 1 for current API usage (of the application provided queue), but could be >1 if we change 'odp_queue_t queue' to 'odp_queue_t queue[]'. > > > > >> > >>> + /* 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(). I think it would be good to have two functions for requesting (implementation created) queue handles: This would output all destination queues of a cos (see odp_pktin_event_queue()). int odp_cls_cos_queue(odp_cos_t cos, odp_queue_t queues[], int num); This would output the queue which is the destination of the hash operation. As Bala says, this is need to find out which (implementation created) queue the flow will be forwarded to. Application may need to initialize the queue context for the (new) flow before any packets are received from the pktio interface. odp_queue_t odp_cls_cos_hash_lookup(odp_cos_t cos, odp_packet_t packet); -Petri
Regards, Bala On 12 December 2016 at 16:40, Savolainen, Petri (Nokia - FI/Espoo) <petri.savolainen@nokia-bell-labs.com> wrote: > > >> -----Original Message----- >> From: Bill Fischofer [mailto:bill.fischofer@linaro.org] >> Sent: Monday, December 12, 2016 3:01 AM >> To: Bala Manoharan <bala.manoharan@linaro.org> >> Cc: lng-odp-forward <lng-odp@lists.linaro.org>; Savolainen, Petri (Nokia - >> FI/Espoo) <petri.savolainen@nokia-bell-labs.com> >> Subject: Re: [lng-odp] [RFC/API-NEXT 1/1] api: classification: packet >> hashing per class of service >> >> On Fri, Dec 9, 2016 at 9:52 PM, Bala Manoharan >> <bala.manoharan@linaro.org> wrote: >> > Regards, >> > Bala >> > >> > >> > On 10 December 2016 at 02:06, Bill Fischofer <bill.fischofer@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. > > > May be it's better to add "OR" functionality directly to PMR, instead of implementing it indirectly through multiple copies of the same destination queue (or same set of queue, or the same pool, ...). > > Currently pmr_create() can be used only for "AND" of terms. We could add a flag into odp_pmr_param_t which enables to define a series of ANDs and ORs. > > odp_pmr_t odp_cls_pmr_create(const odp_pmr_param_t *terms, int num_terms, > odp_cos_t src_cos, odp_cos_t dst_cos); > > term[0] AND (term[1] OR term[2] OR term[3]) AND term[4] The OR functionality of PMR can be implemented using the current APIs also, Lets say if an application wants to create an OR of two pmrs PMR1 and PMR2 the following configuration can be used to achieve the same pmr1 = odp_cls_pmr_create(pmr1_params, 1, default_cos, cos1); pmr2 = odp_cls_pmr_create(pmr2_param, 1, default_cos, cos1); In the above configuration packets arriving at the default CoS will be directed to CoS1 if it matches either pmr1 or pmr2. > > > >> >>> + /* 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. > > > Implementation may be more efficient when it can create the queue(s). Even when application needs only one queue, it may not any particular destination queue (just an queue). > > I think the bool is redundant for enable/disable hash, since num > 1 requires hashing. However, it could be defined as create vs. use application provided queues. 'max_queue_supported' could be redefined as 'num_queues', which is the number of queues to be created, or number of queues application provides. It would be 1 for current API usage (of the application provided queue), but could be >1 if we change 'odp_queue_t queue' to 'odp_queue_t queue[]'. > > >> >> > >> >> >> >>> + /* 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(). This was mainly added for application to initialise any internal contexts which is not accessible for the implementation. > > > I think it would be good to have two functions for requesting (implementation created) queue handles: > > > This would output all destination queues of a cos (see odp_pktin_event_queue()). > > int odp_cls_cos_queue(odp_cos_t cos, odp_queue_t queues[], int num); The main concern I had with this type of function is that it will not be efficient if the application configures millions of queues. -Bala > > > This would output the queue which is the destination of the hash operation. As Bala says, this is need to find out which (implementation created) queue the flow will be forwarded to. Application may need to initialize the queue context for the (new) flow before any packets are received from the pktio interface. > > odp_queue_t odp_cls_cos_hash_lookup(odp_cos_t cos, odp_packet_t packet); > > > -Petri >
On Fri, Dec 9, 2016 at 5:54 AM, Balasubramanian Manoharan <bala.manoharan@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.manoharan@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 */ > + unsigned max_queue_supported; > + > + /** Protocol header combination supported for Hashing */ > + odp_pktin_hash_proto_t hash_supported; I like this idea and think it is critical for supporting implementations that can handle lots of queues. What I don't quite understand is how it relates to the rest of the Classification functionality. For example, I would like all packets coming from the physical interface to be hashed/parsed according to "hash_supported" and then assigned to their respective queues. I would then like to schedule from those queues. That is all. Is this possible? What priority and synchronization will those queues have? > /** 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; > + /* 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; > + /* 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; > + /* 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); > + > +/** > * Discard a class-of-service along with all its associated resources > * > * @param[in] cos_id class-of-service instance. > -- > 1.9.1 >
Regards, Bala On 6 April 2017 at 00:53, Brian Brooks <brian.brooks@linaro.org> wrote: > On Fri, Dec 9, 2016 at 5:54 AM, Balasubramanian Manoharan > <bala.manoharan@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.manoharan@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 */ >> + unsigned max_queue_supported; >> + >> + /** Protocol header combination supported for Hashing */ >> + odp_pktin_hash_proto_t hash_supported; > > I like this idea and think it is critical for supporting > implementations that can handle lots of queues. What I don't quite > understand is how it relates to the rest of the Classification > functionality. For example, I would like all packets coming from the > physical interface to be hashed/parsed according to "hash_supported" > and then assigned to their respective queues. I would then like to > schedule from those queues. That is all. Is this possible? What > priority and synchronization will those queues have? This could be done by adding this configuration to the default Cos in the pktio interface. Currently there is hashing support with pktio interface but that will distribute the packet to odp_pktin_queue_t and the packets have to be received using odp_pktin_recv() function. All the queues belonging to the single queue group will have the same priority and synchronisation. > >> /** 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; >> + /* 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; >> + /* 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; >> + /* 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); >> + >> +/** >> * Discard a class-of-service along with all its associated resources >> * >> * @param[in] cos_id class-of-service instance. >> -- >> 1.9.1 >>
Bala, any idea when this can advance from the RFC stage to a real patch? On Thu, Apr 6, 2017 at 6:15 AM, Bala Manoharan <bala.manoharan@linaro.org> wrote: > Regards, > Bala > > > On 6 April 2017 at 00:53, Brian Brooks <brian.brooks@linaro.org> wrote: >> On Fri, Dec 9, 2016 at 5:54 AM, Balasubramanian Manoharan >> <bala.manoharan@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.manoharan@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 */ >>> + unsigned max_queue_supported; >>> + >>> + /** Protocol header combination supported for Hashing */ >>> + odp_pktin_hash_proto_t hash_supported; >> >> I like this idea and think it is critical for supporting >> implementations that can handle lots of queues. What I don't quite >> understand is how it relates to the rest of the Classification >> functionality. For example, I would like all packets coming from the >> physical interface to be hashed/parsed according to "hash_supported" >> and then assigned to their respective queues. I would then like to >> schedule from those queues. That is all. Is this possible? What >> priority and synchronization will those queues have? > > This could be done by adding this configuration to the default Cos in > the pktio interface. > Currently there is hashing support with pktio interface but that will > distribute the packet to odp_pktin_queue_t > and the packets have to be received using odp_pktin_recv() function. > > All the queues belonging to the single queue group will have the same > priority and synchronisation. > > >> >>> /** 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; >>> + /* 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; >>> + /* 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; >>> + /* 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); >>> + >>> +/** >>> * Discard a class-of-service along with all its associated resources >>> * >>> * @param[in] cos_id class-of-service instance. >>> -- >>> 1.9.1 >>>
Hi Bill, I will post a patch implementing this proposal next week. Regards, Bala On 6 April 2017 at 18:07, Bill Fischofer <bill.fischofer@linaro.org> wrote: > Bala, any idea when this can advance from the RFC stage to a real patch? > > On Thu, Apr 6, 2017 at 6:15 AM, Bala Manoharan > <bala.manoharan@linaro.org> wrote: >> Regards, >> Bala >> >> >> On 6 April 2017 at 00:53, Brian Brooks <brian.brooks@linaro.org> wrote: >>> On Fri, Dec 9, 2016 at 5:54 AM, Balasubramanian Manoharan >>> <bala.manoharan@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.manoharan@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 */ >>>> + unsigned max_queue_supported; >>>> + >>>> + /** Protocol header combination supported for Hashing */ >>>> + odp_pktin_hash_proto_t hash_supported; >>> >>> I like this idea and think it is critical for supporting >>> implementations that can handle lots of queues. What I don't quite >>> understand is how it relates to the rest of the Classification >>> functionality. For example, I would like all packets coming from the >>> physical interface to be hashed/parsed according to "hash_supported" >>> and then assigned to their respective queues. I would then like to >>> schedule from those queues. That is all. Is this possible? What >>> priority and synchronization will those queues have? >> >> This could be done by adding this configuration to the default Cos in >> the pktio interface. >> Currently there is hashing support with pktio interface but that will >> distribute the packet to odp_pktin_queue_t >> and the packets have to be received using odp_pktin_recv() function. >> >> All the queues belonging to the single queue group will have the same >> priority and synchronisation. >> >> >>> >>>> /** 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; >>>> + /* 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; >>>> + /* 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; >>>> + /* 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); >>>> + >>>> +/** >>>> * Discard a class-of-service along with all its associated resources >>>> * >>>> * @param[in] cos_id class-of-service instance. >>>> -- >>>> 1.9.1 >>>>
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 */ + 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; + /* 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; + /* 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; + /* 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); + +/** * Discard a class-of-service along with all its associated resources * * @param[in] cos_id class-of-service instance.
Adds support to spread packet from a single CoS to multiple queues by configuring hashing at CoS level. Signed-off-by: Balasubramanian Manoharan <bala.manoharan@linaro.org> --- include/odp/api/spec/classification.h | 45 ++++++++++++++++++++++++++++++++--- 1 file changed, 42 insertions(+), 3 deletions(-) -- 1.9.1