| Message ID | 1450258133-3375-1-git-send-email-bala.manoharan@linaro.org |
|---|---|
| State | New |
| Headers | show |
On Wed, Dec 16, 2015 at 3:28 AM, Balasubramanian Manoharan < bala.manoharan@linaro.org> wrote: > class of service create function now takes pool, queue, drop policy and > name as input parameters. > Adds class of service parameter structure odp_cls_cos_param_t and > initialization function odp_cls_cos_param_init() > > Signed-off-by: Balasubramanian Manoharan <bala.manoharan@linaro.org> > --- > v2: additional documentation and review comments from Petri > > include/odp/api/classification.h | 37 > +++++++++++++++++++++++++++++++------ > 1 file changed, 31 insertions(+), 6 deletions(-) > > diff --git a/include/odp/api/classification.h > b/include/odp/api/classification.h > index 725e1ab..4db5bf0 100644 > --- a/include/odp/api/classification.h > +++ b/include/odp/api/classification.h > @@ -37,7 +37,7 @@ extern "C" { > > /** > * @def ODP_COS_INVALID > - * This value is returned from odp_cos_create() on failure, > + * This value is returned from odp_cls_cos_create() on failure, > * May also be used as a sink class of service that > * results in packets being discarded. > */ > @@ -60,9 +60,9 @@ extern "C" { > */ > > /** > - * Class-of-service packet drop policies > + * class of service packet drop policies > */ > -typedef enum odp_cos_drop { > +typedef enum odp_cls_drop { > ODP_COS_DROP_POOL, /**< Follow buffer pool drop policy */ > ODP_COS_DROP_NEVER, /**< Never drop, ignoring buffer pool > policy */ > } odp_drop_e; > @@ -89,14 +89,39 @@ typedef enum odp_cos_hdr_flow_fields { > } odp_cos_hdr_flow_fields_e; > > /** > + * Class of service parameters > + * 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_drop_e drop_policy; /**< Drop policy associated with CoS */ > +} odp_cls_cos_param_t; > Why odp_cls_cos_param_t and not just odp_cos_param_t ? See remarks under odp_cls_cos_create(). If that's changed then it would be consistent to change this as well. > + > +/** > + * Initialize class of service parameters > + * > + * Initialize an odp_cls_cos_param_t to its default value for all fields > + * > + * @param param Address of the odp_cls_cos_param_t to be initialized > + */ > +void odp_cls_cos_param_init(odp_cls_cos_param_t *param); > + > +/** > * Create a class-of-service > * > - * @param[in] name String intended for debugging purposes. > + * @param name String intended for debugging purposes. > * > - * @return Class of service instance identifier > + * @param param class of service parameters > + * > + * @retval class of service handle > * @retval ODP_COS_INVALID on failure. > + * > + * @note ODP_QUEUE_INVALID and ODP_POOL_INVALID are valid values for queue > + * and pool associated with a class of service and when any one of these > values > + * are configured as INVALID then the packets assigned to the CoS gets > dropped. > */ > -odp_cos_t odp_cos_create(const char *name); > +odp_cos_t odp_cls_cos_create(const char *name, odp_cls_cos_param_t > *param); > The additional parameter seems reasonable, but it's not clear why the function needs to be renamed cos already stands for Class of Service so odp_cls_cos_create() expands to ODP Classifier Class of Service Create which seems somewhat redundant. It's also inconsistent in that the created object is of type odp_cos_t, not odp_cls_cos_t and is non-symmetric with the existing odp_cos_destroy() function. I'd leave this as just odp_cos_create() with the additional parameter. > > /** > * Discard a class-of-service along with all its associated resources > -- > 1.9.1 > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > https://lists.linaro.org/mailman/listinfo/lng-odp >
The reason for having odp_cls_xxx is that class of service module contains class of service and packet matching rule and we need a common syntax to identify the module hence odp_cls_xxx is used as common prefix. I agree all the functions in classifier module needs to be changed to add the prefix odp_cls_xxx.I will add the rename as a separate patch for the remaining functions. Regards, Bala On 17 December 2015 at 08:51, Bill Fischofer <bill.fischofer@linaro.org> wrote: > > > On Wed, Dec 16, 2015 at 3:28 AM, Balasubramanian Manoharan > <bala.manoharan@linaro.org> wrote: >> >> class of service create function now takes pool, queue, drop policy and >> name as input parameters. >> Adds class of service parameter structure odp_cls_cos_param_t and >> initialization function odp_cls_cos_param_init() >> >> Signed-off-by: Balasubramanian Manoharan <bala.manoharan@linaro.org> >> --- >> v2: additional documentation and review comments from Petri >> >> include/odp/api/classification.h | 37 >> +++++++++++++++++++++++++++++++------ >> 1 file changed, 31 insertions(+), 6 deletions(-) >> >> diff --git a/include/odp/api/classification.h >> b/include/odp/api/classification.h >> index 725e1ab..4db5bf0 100644 >> --- a/include/odp/api/classification.h >> +++ b/include/odp/api/classification.h >> @@ -37,7 +37,7 @@ extern "C" { >> >> /** >> * @def ODP_COS_INVALID >> - * This value is returned from odp_cos_create() on failure, >> + * This value is returned from odp_cls_cos_create() on failure, >> * May also be used as a sink class of service that >> * results in packets being discarded. >> */ >> @@ -60,9 +60,9 @@ extern "C" { >> */ >> >> /** >> - * Class-of-service packet drop policies >> + * class of service packet drop policies >> */ >> -typedef enum odp_cos_drop { >> +typedef enum odp_cls_drop { >> ODP_COS_DROP_POOL, /**< Follow buffer pool drop policy */ >> ODP_COS_DROP_NEVER, /**< Never drop, ignoring buffer pool >> policy */ >> } odp_drop_e; >> @@ -89,14 +89,39 @@ typedef enum odp_cos_hdr_flow_fields { >> } odp_cos_hdr_flow_fields_e; >> >> /** >> + * Class of service parameters >> + * 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_drop_e drop_policy; /**< Drop policy associated with CoS */ >> +} odp_cls_cos_param_t; > > > Why odp_cls_cos_param_t and not just odp_cos_param_t ? See remarks under > odp_cls_cos_create(). If that's changed then it would be consistent to > change this as well. > >> >> + >> +/** >> + * Initialize class of service parameters >> + * >> + * Initialize an odp_cls_cos_param_t to its default value for all fields >> + * >> + * @param param Address of the odp_cls_cos_param_t to be initialized >> + */ >> +void odp_cls_cos_param_init(odp_cls_cos_param_t *param); >> + >> +/** >> * Create a class-of-service >> * >> - * @param[in] name String intended for debugging purposes. >> + * @param name String intended for debugging purposes. >> * >> - * @return Class of service instance identifier >> + * @param param class of service parameters >> + * >> + * @retval class of service handle >> * @retval ODP_COS_INVALID on failure. >> + * >> + * @note ODP_QUEUE_INVALID and ODP_POOL_INVALID are valid values for >> queue >> + * and pool associated with a class of service and when any one of these >> values >> + * are configured as INVALID then the packets assigned to the CoS gets >> dropped. >> */ >> -odp_cos_t odp_cos_create(const char *name); >> +odp_cos_t odp_cls_cos_create(const char *name, odp_cls_cos_param_t >> *param); > > > The additional parameter seems reasonable, but it's not clear why the > function needs to be renamed cos already stands for Class of Service so > odp_cls_cos_create() expands to ODP Classifier Class of Service Create which > seems somewhat redundant. It's also inconsistent in that the created object > is of type odp_cos_t, not odp_cls_cos_t and is non-symmetric with the > existing odp_cos_destroy() function. I'd leave this as just > odp_cos_create() with the additional parameter. >> >> >> /** >> * Discard a class-of-service along with all its associated resources >> -- >> 1.9.1 >> >> _______________________________________________ >> lng-odp mailing list >> lng-odp@lists.linaro.org >> https://lists.linaro.org/mailman/listinfo/lng-odp > >
My argument is that odp_cls_cos_xxx is redundant and odp_cos_xxx is both less cumbersome and more intuitive for the application writer. Consider that if a Class of Service is only used in classification then there's no need for the redundant cls prefix. But if we ever extend classes of service to be used by another component (e.g., allowing pktios to refer to classes of service directly) then the cls prefix would be confusingly restrictive. Aside from that, the name change itself represents an API change and it's not clear what benefits are gained by doing this. I'm not sure where this requirement originated, but perhaps we should revisit the reasoning behind it. On Thu, Dec 17, 2015 at 12:37 AM, Bala Manoharan <bala.manoharan@linaro.org> wrote: > The reason for having odp_cls_xxx is that class of service module > contains class of service and packet matching rule and we need a > common syntax to identify the module hence odp_cls_xxx is used as > common prefix. > > I agree all the functions in classifier module needs to be changed to > add the prefix odp_cls_xxx.I will add the rename as a separate patch > for the remaining functions. > > Regards, > Bala > > On 17 December 2015 at 08:51, Bill Fischofer <bill.fischofer@linaro.org> > wrote: > > > > > > On Wed, Dec 16, 2015 at 3:28 AM, Balasubramanian Manoharan > > <bala.manoharan@linaro.org> wrote: > >> > >> class of service create function now takes pool, queue, drop policy and > >> name as input parameters. > >> Adds class of service parameter structure odp_cls_cos_param_t and > >> initialization function odp_cls_cos_param_init() > >> > >> Signed-off-by: Balasubramanian Manoharan <bala.manoharan@linaro.org> > >> --- > >> v2: additional documentation and review comments from Petri > >> > >> include/odp/api/classification.h | 37 > >> +++++++++++++++++++++++++++++++------ > >> 1 file changed, 31 insertions(+), 6 deletions(-) > >> > >> diff --git a/include/odp/api/classification.h > >> b/include/odp/api/classification.h > >> index 725e1ab..4db5bf0 100644 > >> --- a/include/odp/api/classification.h > >> +++ b/include/odp/api/classification.h > >> @@ -37,7 +37,7 @@ extern "C" { > >> > >> /** > >> * @def ODP_COS_INVALID > >> - * This value is returned from odp_cos_create() on failure, > >> + * This value is returned from odp_cls_cos_create() on failure, > >> * May also be used as a sink class of service that > >> * results in packets being discarded. > >> */ > >> @@ -60,9 +60,9 @@ extern "C" { > >> */ > >> > >> /** > >> - * Class-of-service packet drop policies > >> + * class of service packet drop policies > >> */ > >> -typedef enum odp_cos_drop { > >> +typedef enum odp_cls_drop { > >> ODP_COS_DROP_POOL, /**< Follow buffer pool drop policy */ > >> ODP_COS_DROP_NEVER, /**< Never drop, ignoring buffer pool > >> policy */ > >> } odp_drop_e; > >> @@ -89,14 +89,39 @@ typedef enum odp_cos_hdr_flow_fields { > >> } odp_cos_hdr_flow_fields_e; > >> > >> /** > >> + * Class of service parameters > >> + * 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_drop_e drop_policy; /**< Drop policy associated with CoS */ > >> +} odp_cls_cos_param_t; > > > > > > Why odp_cls_cos_param_t and not just odp_cos_param_t ? See remarks under > > odp_cls_cos_create(). If that's changed then it would be consistent to > > change this as well. > > > >> > >> + > >> +/** > >> + * Initialize class of service parameters > >> + * > >> + * Initialize an odp_cls_cos_param_t to its default value for all > fields > >> + * > >> + * @param param Address of the odp_cls_cos_param_t to be initialized > >> + */ > >> +void odp_cls_cos_param_init(odp_cls_cos_param_t *param); > >> + > >> +/** > >> * Create a class-of-service > >> * > >> - * @param[in] name String intended for debugging purposes. > >> + * @param name String intended for debugging purposes. > >> * > >> - * @return Class of service instance identifier > >> + * @param param class of service parameters > >> + * > >> + * @retval class of service handle > >> * @retval ODP_COS_INVALID on failure. > >> + * > >> + * @note ODP_QUEUE_INVALID and ODP_POOL_INVALID are valid values for > >> queue > >> + * and pool associated with a class of service and when any one of > these > >> values > >> + * are configured as INVALID then the packets assigned to the CoS gets > >> dropped. > >> */ > >> -odp_cos_t odp_cos_create(const char *name); > >> +odp_cos_t odp_cls_cos_create(const char *name, odp_cls_cos_param_t > >> *param); > > > > > > The additional parameter seems reasonable, but it's not clear why the > > function needs to be renamed cos already stands for Class of Service so > > odp_cls_cos_create() expands to ODP Classifier Class of Service Create > which > > seems somewhat redundant. It's also inconsistent in that the created > object > > is of type odp_cos_t, not odp_cls_cos_t and is non-symmetric with the > > existing odp_cos_destroy() function. I'd leave this as just > > odp_cos_create() with the additional parameter. > >> > >> > >> /** > >> * Discard a class-of-service along with all its associated resources > >> -- > >> 1.9.1 > >> > >> _______________________________________________ > >> lng-odp mailing list > >> lng-odp@lists.linaro.org > >> https://lists.linaro.org/mailman/listinfo/lng-odp > > > > > > > > -- > Regards, > Bala >
I agree with that general convention, however a class of service is a first class entity (like a pool). It's why we have odp_pool_create() and parameterize that it's a buffer vs. packet pool instead of having separate odp_buffer_pool_create() and odp_packet_pool_create() APIs. Using that analogy odp_cos_create() is creating a class of service. The fact that a class of service is being used for classification purposes would be encoded (if required) in the new odp_cos_param_t struct used to qualify the creation. I agree that odp_drop_e is unspecific, however the logical qualification there would be odp_cos_drop_e (or _t) since it is an attribute on a class of service (you don't "drop" a classifier). Indeed the current typedef for odp_drop_e says enum odp_cos_drop so the result of the typedef should be an odp_cos_drop_t. Similarly for PMRs the PMR is again a first class object which is why odp_pmr_create() gives an odp_pmr_t. Again, for consistency if we needed to qualify it we'd add an odp_pmr_param_t struct to the create() call to do that. That would make sense if we wanted to have PMRs that were needed to be distinguished between use by classification vs. DPI, etc. On Thu, Dec 17, 2015 at 5:04 AM, Savolainen, Petri (Nokia - FI/Espoo) < petri.savolainen@nokia.com> wrote: > The rationale behind this is that classification API should follow the > same, prefixed naming convention than other APIs. CoS is a generally used > term and e.g. scheduler, TM, packet, etc APIs could define also APIs > around Class of Service. Here odp_cos_t is essentially a node in a > classification hierarchy tree. It’s similarly to TM API odp_tm_node_t , > which is correctly prefix and easy find when seen in code. > > > > Also other CLS API types (odp_drop_e, odp_pmr_t, odp_flowsig_t) should be > prefixed since they are not self-explanatory part of classification API > (only). For example, a future DPI API could define “Pattern Matching Rules”. > > > > -Petri > > > > > > *From:* lng-odp [mailto:lng-odp-bounces@lists.linaro.org] *On Behalf Of *EXT > Bill Fischofer > *Sent:* Thursday, December 17, 2015 11:59 AM > *To:* Bala Manoharan > *Cc:* LNG ODP Mailman List > *Subject:* Re: [lng-odp] [API-NEXT/PATCHv2 1/6] api: classification: add > class of serivce create api > > > > My argument is that odp_cls_cos_xxx is redundant and odp_cos_xxx is both > less cumbersome and more intuitive for the application writer. Consider > that if a Class of Service is only used in classification then there's no > need for the redundant cls prefix. But if we ever extend classes of > service to be used by another component (e.g., allowing pktios to refer to > classes of service directly) then the cls prefix would be confusingly > restrictive. > > > > Aside from that, the name change itself represents an API change and it's > not clear what benefits are gained by doing this. I'm not sure where this > requirement originated, but perhaps we should revisit the reasoning behind > it. > > > > On Thu, Dec 17, 2015 at 12:37 AM, Bala Manoharan < > bala.manoharan@linaro.org> wrote: > > The reason for having odp_cls_xxx is that class of service module > contains class of service and packet matching rule and we need a > common syntax to identify the module hence odp_cls_xxx is used as > common prefix. > > I agree all the functions in classifier module needs to be changed to > add the prefix odp_cls_xxx.I will add the rename as a separate patch > for the remaining functions. > > Regards, > Bala > > > On 17 December 2015 at 08:51, Bill Fischofer <bill.fischofer@linaro.org> > wrote: > > > > > > On Wed, Dec 16, 2015 at 3:28 AM, Balasubramanian Manoharan > > <bala.manoharan@linaro.org> wrote: > >> > >> class of service create function now takes pool, queue, drop policy and > >> name as input parameters. > >> Adds class of service parameter structure odp_cls_cos_param_t and > >> initialization function odp_cls_cos_param_init() > >> > >> Signed-off-by: Balasubramanian Manoharan <bala.manoharan@linaro.org> > >> --- > >> v2: additional documentation and review comments from Petri > >> > >> include/odp/api/classification.h | 37 > >> +++++++++++++++++++++++++++++++------ > >> 1 file changed, 31 insertions(+), 6 deletions(-) > >> > >> diff --git a/include/odp/api/classification.h > >> b/include/odp/api/classification.h > >> index 725e1ab..4db5bf0 100644 > >> --- a/include/odp/api/classification.h > >> +++ b/include/odp/api/classification.h > >> @@ -37,7 +37,7 @@ extern "C" { > >> > >> /** > >> * @def ODP_COS_INVALID > >> - * This value is returned from odp_cos_create() on failure, > >> + * This value is returned from odp_cls_cos_create() on failure, > >> * May also be used as a sink class of service that > >> * results in packets being discarded. > >> */ > >> @@ -60,9 +60,9 @@ extern "C" { > >> */ > >> > >> /** > >> - * Class-of-service packet drop policies > >> + * class of service packet drop policies > >> */ > >> -typedef enum odp_cos_drop { > >> +typedef enum odp_cls_drop { > >> ODP_COS_DROP_POOL, /**< Follow buffer pool drop policy */ > >> ODP_COS_DROP_NEVER, /**< Never drop, ignoring buffer pool > >> policy */ > >> } odp_drop_e; > >> @@ -89,14 +89,39 @@ typedef enum odp_cos_hdr_flow_fields { > >> } odp_cos_hdr_flow_fields_e; > >> > >> /** > >> + * Class of service parameters > >> + * 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_drop_e drop_policy; /**< Drop policy associated with CoS */ > >> +} odp_cls_cos_param_t; > > > > > > Why odp_cls_cos_param_t and not just odp_cos_param_t ? See remarks under > > odp_cls_cos_create(). If that's changed then it would be consistent to > > change this as well. > > > >> > >> + > >> +/** > >> + * Initialize class of service parameters > >> + * > >> + * Initialize an odp_cls_cos_param_t to its default value for all > fields > >> + * > >> + * @param param Address of the odp_cls_cos_param_t to be initialized > >> + */ > >> +void odp_cls_cos_param_init(odp_cls_cos_param_t *param); > >> + > >> +/** > >> * Create a class-of-service > >> * > >> - * @param[in] name String intended for debugging purposes. > >> + * @param name String intended for debugging purposes. > >> * > >> - * @return Class of service instance identifier > >> + * @param param class of service parameters > >> + * > >> + * @retval class of service handle > >> * @retval ODP_COS_INVALID on failure. > >> + * > >> + * @note ODP_QUEUE_INVALID and ODP_POOL_INVALID are valid values for > >> queue > >> + * and pool associated with a class of service and when any one of > these > >> values > >> + * are configured as INVALID then the packets assigned to the CoS gets > >> dropped. > >> */ > >> -odp_cos_t odp_cos_create(const char *name); > >> +odp_cos_t odp_cls_cos_create(const char *name, odp_cls_cos_param_t > >> *param); > > > > > > The additional parameter seems reasonable, but it's not clear why the > > function needs to be renamed cos already stands for Class of Service so > > odp_cls_cos_create() expands to ODP Classifier Class of Service Create > which > > seems somewhat redundant. It's also inconsistent in that the created > object > > is of type odp_cos_t, not odp_cls_cos_t and is non-symmetric with the > > existing odp_cos_destroy() function. I'd leave this as just > > odp_cos_create() with the additional parameter. > >> > >> > >> /** > >> * Discard a class-of-service along with all its associated resources > >> -- > >> 1.9.1 > >> > >> _______________________________________________ > >> lng-odp mailing list > >> lng-odp@lists.linaro.org > >> https://lists.linaro.org/mailman/listinfo/lng-odp > > > > > > > -- > Regards, > Bala > > >
Fair enough, but then it seems we then need to add the odp_cls prefix to a lot more APIs and typedefs in the existing classification.h to be fully consistent. Right now there's a confusing mix of those that do and do not have this prefix. On Thu, Dec 17, 2015 at 5:49 AM, Savolainen, Petri (Nokia - FI/Espoo) < petri.savolainen@nokia.com> wrote: > > > What e.g. scheduler or TM would do with a odp_cos_t which is associated > with queue, pool and drop_policy? > > > > typedef struct odp_cls_cos_param { > odp_queue_t queue; /**< Queue associated with CoS */ > odp_pool_t pool; /**< Pool associated with CoS */ > odp_drop_e drop_policy; /**< Drop policy associated with CoS */ > } odp_cls_cos_param_t; > > > > > > Scheduler (odp_queue_t) has its own priority scheduling and so does TM. > Cos_param.queue has meaning only when packet enters the system and is > stored first time to the queue. After that packet QoS (or CoS) is defined > by a chain of queues, scheduling, application processing and TM. The > initial input queue, pool or packet input drop policy does not matter in > later stages. So this CoS (and those parameters) are significant only for > the classifier. This CoS specification end when classifier does enqueue or > drop the packet. > > > > Sure, if entire ODP API level CoS property is specified later, we can use > odp_cos_t for that but currently that’s not the case. > > > > DPI PMR would be very different from classification PMRs. DPI API would > deal with regular expressions, etc language to express multitude of > different combinations of possible packet payload … whereas packet input > classification is shallow/fixed packet inspection on predefined header > fields (to keep it fast). > > > > -Petri > > > > > > > > *From:* EXT Bill Fischofer [mailto:bill.fischofer@linaro.org] > *Sent:* Thursday, December 17, 2015 1:27 PM > *To:* Savolainen, Petri (Nokia - FI/Espoo) > *Cc:* Bala Manoharan; LNG ODP Mailman List > > *Subject:* Re: [lng-odp] [API-NEXT/PATCHv2 1/6] api: classification: add > class of serivce create api > > > > I agree with that general convention, however a class of service is a > first class entity (like a pool). It's why we have odp_pool_create() and > parameterize that it's a buffer vs. packet pool instead of having separate > odp_buffer_pool_create() and odp_packet_pool_create() APIs. Using that > analogy odp_cos_create() is creating a class of service. The fact that a > class of service is being used for classification purposes would be encoded > (if required) in the new odp_cos_param_t struct used to qualify the > creation. > > > > I agree that odp_drop_e is unspecific, however the logical qualification > there would be odp_cos_drop_e (or _t) since it is an attribute on a class > of service (you don't "drop" a classifier). Indeed the current typedef for > odp_drop_e says enum odp_cos_drop so the result of the typedef should be an > odp_cos_drop_t. > > > > Similarly for PMRs the PMR is again a first class object which is why > odp_pmr_create() gives an odp_pmr_t. Again, for consistency if we needed > to qualify it we'd add an odp_pmr_param_t struct to the create() call to do > that. That would make sense if we wanted to have PMRs that were needed to > be distinguished between use by classification vs. DPI, etc. > > > > On Thu, Dec 17, 2015 at 5:04 AM, Savolainen, Petri (Nokia - FI/Espoo) < > petri.savolainen@nokia.com> wrote: > > The rationale behind this is that classification API should follow the > same, prefixed naming convention than other APIs. CoS is a generally used > term and e.g. scheduler, TM, packet, etc APIs could define also APIs > around Class of Service. Here odp_cos_t is essentially a node in a > classification hierarchy tree. It’s similarly to TM API odp_tm_node_t , > which is correctly prefix and easy find when seen in code. > > > > Also other CLS API types (odp_drop_e, odp_pmr_t, odp_flowsig_t) should be > prefixed since they are not self-explanatory part of classification API > (only). For example, a future DPI API could define “Pattern Matching Rules”. > > > > -Petri > > > > > > *From:* lng-odp [mailto:lng-odp-bounces@lists.linaro.org] *On Behalf Of *EXT > Bill Fischofer > *Sent:* Thursday, December 17, 2015 11:59 AM > *To:* Bala Manoharan > *Cc:* LNG ODP Mailman List > *Subject:* Re: [lng-odp] [API-NEXT/PATCHv2 1/6] api: classification: add > class of serivce create api > > > > My argument is that odp_cls_cos_xxx is redundant and odp_cos_xxx is both > less cumbersome and more intuitive for the application writer. Consider > that if a Class of Service is only used in classification then there's no > need for the redundant cls prefix. But if we ever extend classes of > service to be used by another component (e.g., allowing pktios to refer to > classes of service directly) then the cls prefix would be confusingly > restrictive. > > > > Aside from that, the name change itself represents an API change and it's > not clear what benefits are gained by doing this. I'm not sure where this > requirement originated, but perhaps we should revisit the reasoning behind > it. > > > > On Thu, Dec 17, 2015 at 12:37 AM, Bala Manoharan < > bala.manoharan@linaro.org> wrote: > > The reason for having odp_cls_xxx is that class of service module > contains class of service and packet matching rule and we need a > common syntax to identify the module hence odp_cls_xxx is used as > common prefix. > > I agree all the functions in classifier module needs to be changed to > add the prefix odp_cls_xxx.I will add the rename as a separate patch > for the remaining functions. > > Regards, > Bala > > > On 17 December 2015 at 08:51, Bill Fischofer <bill.fischofer@linaro.org> > wrote: > > > > > > On Wed, Dec 16, 2015 at 3:28 AM, Balasubramanian Manoharan > > <bala.manoharan@linaro.org> wrote: > >> > >> class of service create function now takes pool, queue, drop policy and > >> name as input parameters. > >> Adds class of service parameter structure odp_cls_cos_param_t and > >> initialization function odp_cls_cos_param_init() > >> > >> Signed-off-by: Balasubramanian Manoharan <bala.manoharan@linaro.org> > >> --- > >> v2: additional documentation and review comments from Petri > >> > >> include/odp/api/classification.h | 37 > >> +++++++++++++++++++++++++++++++------ > >> 1 file changed, 31 insertions(+), 6 deletions(-) > >> > >> diff --git a/include/odp/api/classification.h > >> b/include/odp/api/classification.h > >> index 725e1ab..4db5bf0 100644 > >> --- a/include/odp/api/classification.h > >> +++ b/include/odp/api/classification.h > >> @@ -37,7 +37,7 @@ extern "C" { > >> > >> /** > >> * @def ODP_COS_INVALID > >> - * This value is returned from odp_cos_create() on failure, > >> + * This value is returned from odp_cls_cos_create() on failure, > >> * May also be used as a sink class of service that > >> * results in packets being discarded. > >> */ > >> @@ -60,9 +60,9 @@ extern "C" { > >> */ > >> > >> /** > >> - * Class-of-service packet drop policies > >> + * class of service packet drop policies > >> */ > >> -typedef enum odp_cos_drop { > >> +typedef enum odp_cls_drop { > >> ODP_COS_DROP_POOL, /**< Follow buffer pool drop policy */ > >> ODP_COS_DROP_NEVER, /**< Never drop, ignoring buffer pool > >> policy */ > >> } odp_drop_e; > >> @@ -89,14 +89,39 @@ typedef enum odp_cos_hdr_flow_fields { > >> } odp_cos_hdr_flow_fields_e; > >> > >> /** > >> + * Class of service parameters > >> + * 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_drop_e drop_policy; /**< Drop policy associated with CoS */ > >> +} odp_cls_cos_param_t; > > > > > > Why odp_cls_cos_param_t and not just odp_cos_param_t ? See remarks under > > odp_cls_cos_create(). If that's changed then it would be consistent to > > change this as well. > > > >> > >> + > >> +/** > >> + * Initialize class of service parameters > >> + * > >> + * Initialize an odp_cls_cos_param_t to its default value for all > fields > >> + * > >> + * @param param Address of the odp_cls_cos_param_t to be initialized > >> + */ > >> +void odp_cls_cos_param_init(odp_cls_cos_param_t *param); > >> + > >> +/** > >> * Create a class-of-service > >> * > >> - * @param[in] name String intended for debugging purposes. > >> + * @param name String intended for debugging purposes. > >> * > >> - * @return Class of service instance identifier > >> + * @param param class of service parameters > >> + * > >> + * @retval class of service handle > >> * @retval ODP_COS_INVALID on failure. > >> + * > >> + * @note ODP_QUEUE_INVALID and ODP_POOL_INVALID are valid values for > >> queue > >> + * and pool associated with a class of service and when any one of > these > >> values > >> + * are configured as INVALID then the packets assigned to the CoS gets > >> dropped. > >> */ > >> -odp_cos_t odp_cos_create(const char *name); > >> +odp_cos_t odp_cls_cos_create(const char *name, odp_cls_cos_param_t > >> *param); > > > > > > The additional parameter seems reasonable, but it's not clear why the > > function needs to be renamed cos already stands for Class of Service so > > odp_cls_cos_create() expands to ODP Classifier Class of Service Create > which > > seems somewhat redundant. It's also inconsistent in that the created > object > > is of type odp_cos_t, not odp_cls_cos_t and is non-symmetric with the > > existing odp_cos_destroy() function. I'd leave this as just > > odp_cos_create() with the additional parameter. > >> > >> > >> /** > >> * Discard a class-of-service along with all its associated resources > >> -- > >> 1.9.1 > >> > >> _______________________________________________ > >> lng-odp mailing list > >> lng-odp@lists.linaro.org > >> https://lists.linaro.org/mailman/listinfo/lng-odp > > > > > > -- > Regards, > Bala > > > > >
Ok, then the patch should be reworked to do this. No point in dragging out the process through a series of disruptive changes. On Thu, Dec 17, 2015 at 5:56 AM, Savolainen, Petri (Nokia - FI/Espoo) < petri.savolainen@nokia.com> wrote: > Yes, that’s the end goal of this process. All classification API calls, > types, defines, … are prefixed with odp_cls_. > > > > -Petri > > > > > > *From:* EXT Bill Fischofer [mailto:bill.fischofer@linaro.org] > *Sent:* Thursday, December 17, 2015 1:54 PM > > *To:* Savolainen, Petri (Nokia - FI/Espoo) > *Cc:* Bala Manoharan; LNG ODP Mailman List > *Subject:* Re: [lng-odp] [API-NEXT/PATCHv2 1/6] api: classification: add > class of serivce create api > > > > Fair enough, but then it seems we then need to add the odp_cls prefix to a > lot more APIs and typedefs in the existing classification.h to be fully > consistent. Right now there's a confusing mix of those that do and do not > have this prefix. > > > > On Thu, Dec 17, 2015 at 5:49 AM, Savolainen, Petri (Nokia - FI/Espoo) < > petri.savolainen@nokia.com> wrote: > > > > What e.g. scheduler or TM would do with a odp_cos_t which is associated > with queue, pool and drop_policy? > > > > typedef struct odp_cls_cos_param { > odp_queue_t queue; /**< Queue associated with CoS */ > odp_pool_t pool; /**< Pool associated with CoS */ > odp_drop_e drop_policy; /**< Drop policy associated with CoS */ > } odp_cls_cos_param_t; > > > > > > Scheduler (odp_queue_t) has its own priority scheduling and so does TM. > Cos_param.queue has meaning only when packet enters the system and is > stored first time to the queue. After that packet QoS (or CoS) is defined > by a chain of queues, scheduling, application processing and TM. The > initial input queue, pool or packet input drop policy does not matter in > later stages. So this CoS (and those parameters) are significant only for > the classifier. This CoS specification end when classifier does enqueue or > drop the packet. > > > > Sure, if entire ODP API level CoS property is specified later, we can use > odp_cos_t for that but currently that’s not the case. > > > > DPI PMR would be very different from classification PMRs. DPI API would > deal with regular expressions, etc language to express multitude of > different combinations of possible packet payload … whereas packet input > classification is shallow/fixed packet inspection on predefined header > fields (to keep it fast). > > > > -Petri > > > > > > > > *From:* EXT Bill Fischofer [mailto:bill.fischofer@linaro.org] > *Sent:* Thursday, December 17, 2015 1:27 PM > *To:* Savolainen, Petri (Nokia - FI/Espoo) > *Cc:* Bala Manoharan; LNG ODP Mailman List > > > *Subject:* Re: [lng-odp] [API-NEXT/PATCHv2 1/6] api: classification: add > class of serivce create api > > > > I agree with that general convention, however a class of service is a > first class entity (like a pool). It's why we have odp_pool_create() and > parameterize that it's a buffer vs. packet pool instead of having separate > odp_buffer_pool_create() and odp_packet_pool_create() APIs. Using that > analogy odp_cos_create() is creating a class of service. The fact that a > class of service is being used for classification purposes would be encoded > (if required) in the new odp_cos_param_t struct used to qualify the > creation. > > > > I agree that odp_drop_e is unspecific, however the logical qualification > there would be odp_cos_drop_e (or _t) since it is an attribute on a class > of service (you don't "drop" a classifier). Indeed the current typedef for > odp_drop_e says enum odp_cos_drop so the result of the typedef should be an > odp_cos_drop_t. > > > > Similarly for PMRs the PMR is again a first class object which is why > odp_pmr_create() gives an odp_pmr_t. Again, for consistency if we needed > to qualify it we'd add an odp_pmr_param_t struct to the create() call to do > that. That would make sense if we wanted to have PMRs that were needed to > be distinguished between use by classification vs. DPI, etc. > > > > On Thu, Dec 17, 2015 at 5:04 AM, Savolainen, Petri (Nokia - FI/Espoo) < > petri.savolainen@nokia.com> wrote: > > The rationale behind this is that classification API should follow the > same, prefixed naming convention than other APIs. CoS is a generally used > term and e.g. scheduler, TM, packet, etc APIs could define also APIs > around Class of Service. Here odp_cos_t is essentially a node in a > classification hierarchy tree. It’s similarly to TM API odp_tm_node_t , > which is correctly prefix and easy find when seen in code. > > > > Also other CLS API types (odp_drop_e, odp_pmr_t, odp_flowsig_t) should be > prefixed since they are not self-explanatory part of classification API > (only). For example, a future DPI API could define “Pattern Matching Rules”. > > > > -Petri > > > > > > *From:* lng-odp [mailto:lng-odp-bounces@lists.linaro.org] *On Behalf Of *EXT > Bill Fischofer > *Sent:* Thursday, December 17, 2015 11:59 AM > *To:* Bala Manoharan > *Cc:* LNG ODP Mailman List > *Subject:* Re: [lng-odp] [API-NEXT/PATCHv2 1/6] api: classification: add > class of serivce create api > > > > My argument is that odp_cls_cos_xxx is redundant and odp_cos_xxx is both > less cumbersome and more intuitive for the application writer. Consider > that if a Class of Service is only used in classification then there's no > need for the redundant cls prefix. But if we ever extend classes of > service to be used by another component (e.g., allowing pktios to refer to > classes of service directly) then the cls prefix would be confusingly > restrictive. > > > > Aside from that, the name change itself represents an API change and it's > not clear what benefits are gained by doing this. I'm not sure where this > requirement originated, but perhaps we should revisit the reasoning behind > it. > > > > On Thu, Dec 17, 2015 at 12:37 AM, Bala Manoharan < > bala.manoharan@linaro.org> wrote: > > The reason for having odp_cls_xxx is that class of service module > contains class of service and packet matching rule and we need a > common syntax to identify the module hence odp_cls_xxx is used as > common prefix. > > I agree all the functions in classifier module needs to be changed to > add the prefix odp_cls_xxx.I will add the rename as a separate patch > for the remaining functions. > > Regards, > Bala > > > On 17 December 2015 at 08:51, Bill Fischofer <bill.fischofer@linaro.org> > wrote: > > > > > > On Wed, Dec 16, 2015 at 3:28 AM, Balasubramanian Manoharan > > <bala.manoharan@linaro.org> wrote: > >> > >> class of service create function now takes pool, queue, drop policy and > >> name as input parameters. > >> Adds class of service parameter structure odp_cls_cos_param_t and > >> initialization function odp_cls_cos_param_init() > >> > >> Signed-off-by: Balasubramanian Manoharan <bala.manoharan@linaro.org> > >> --- > >> v2: additional documentation and review comments from Petri > >> > >> include/odp/api/classification.h | 37 > >> +++++++++++++++++++++++++++++++------ > >> 1 file changed, 31 insertions(+), 6 deletions(-) > >> > >> diff --git a/include/odp/api/classification.h > >> b/include/odp/api/classification.h > >> index 725e1ab..4db5bf0 100644 > >> --- a/include/odp/api/classification.h > >> +++ b/include/odp/api/classification.h > >> @@ -37,7 +37,7 @@ extern "C" { > >> > >> /** > >> * @def ODP_COS_INVALID > >> - * This value is returned from odp_cos_create() on failure, > >> + * This value is returned from odp_cls_cos_create() on failure, > >> * May also be used as a sink class of service that > >> * results in packets being discarded. > >> */ > >> @@ -60,9 +60,9 @@ extern "C" { > >> */ > >> > >> /** > >> - * Class-of-service packet drop policies > >> + * class of service packet drop policies > >> */ > >> -typedef enum odp_cos_drop { > >> +typedef enum odp_cls_drop { > >> ODP_COS_DROP_POOL, /**< Follow buffer pool drop policy */ > >> ODP_COS_DROP_NEVER, /**< Never drop, ignoring buffer pool > >> policy */ > >> } odp_drop_e; > >> @@ -89,14 +89,39 @@ typedef enum odp_cos_hdr_flow_fields { > >> } odp_cos_hdr_flow_fields_e; > >> > >> /** > >> + * Class of service parameters > >> + * 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_drop_e drop_policy; /**< Drop policy associated with CoS */ > >> +} odp_cls_cos_param_t; > > > > > > Why odp_cls_cos_param_t and not just odp_cos_param_t ? See remarks under > > odp_cls_cos_create(). If that's changed then it would be consistent to > > change this as well. > > > >> > >> + > >> +/** > >> + * Initialize class of service parameters > >> + * > >> + * Initialize an odp_cls_cos_param_t to its default value for all > fields > >> + * > >> + * @param param Address of the odp_cls_cos_param_t to be initialized > >> + */ > >> +void odp_cls_cos_param_init(odp_cls_cos_param_t *param); > >> + > >> +/** > >> * Create a class-of-service > >> * > >> - * @param[in] name String intended for debugging purposes. > >> + * @param name String intended for debugging purposes. > >> * > >> - * @return Class of service instance identifier > >> + * @param param class of service parameters > >> + * > >> + * @retval class of service handle > >> * @retval ODP_COS_INVALID on failure. > >> + * > >> + * @note ODP_QUEUE_INVALID and ODP_POOL_INVALID are valid values for > >> queue > >> + * and pool associated with a class of service and when any one of > these > >> values > >> + * are configured as INVALID then the packets assigned to the CoS gets > >> dropped. > >> */ > >> -odp_cos_t odp_cos_create(const char *name); > >> +odp_cos_t odp_cls_cos_create(const char *name, odp_cls_cos_param_t > >> *param); > > > > > > The additional parameter seems reasonable, but it's not clear why the > > function needs to be renamed cos already stands for Class of Service so > > odp_cls_cos_create() expands to ODP Classifier Class of Service Create > which > > seems somewhat redundant. It's also inconsistent in that the created > object > > is of type odp_cos_t, not odp_cls_cos_t and is non-symmetric with the > > existing odp_cos_destroy() function. I'd leave this as just > > odp_cos_create() with the additional parameter. > >> > >> > >> /** > >> * Discard a class-of-service along with all its associated resources > >> -- > >> 1.9.1 > >> > >> _______________________________________________ > >> lng-odp mailing list > >> lng-odp@lists.linaro.org > >> https://lists.linaro.org/mailman/listinfo/lng-odp > > > > > > -- > Regards, > Bala > > > > > > >
On 17 December 2015 at 17:31, Bill Fischofer <bill.fischofer@linaro.org> wrote: > Ok, then the patch should be reworked to do this. No point in dragging out > the process through a series of disruptive changes. Yes. The ultimate goal is to add this prefix to all existing APIs. This patch only adds a new api and it was better to add the new API with the latest convention. I will post a separate patch which renames all the existing classification APIs with the prefix odp_cls_xxx Regards, Bala > > On Thu, Dec 17, 2015 at 5:56 AM, Savolainen, Petri (Nokia - FI/Espoo) > <petri.savolainen@nokia.com> wrote: >> >> Yes, that’s the end goal of this process. All classification API calls, >> types, defines, … are prefixed with odp_cls_. >> >> >> >> -Petri >> >> >> >> >> >> From: EXT Bill Fischofer [mailto:bill.fischofer@linaro.org] >> Sent: Thursday, December 17, 2015 1:54 PM >> >> >> To: Savolainen, Petri (Nokia - FI/Espoo) >> Cc: Bala Manoharan; LNG ODP Mailman List >> Subject: Re: [lng-odp] [API-NEXT/PATCHv2 1/6] api: classification: add >> class of serivce create api >> >> >> >> Fair enough, but then it seems we then need to add the odp_cls prefix to a >> lot more APIs and typedefs in the existing classification.h to be fully >> consistent. Right now there's a confusing mix of those that do and do not >> have this prefix. >> >> >> >> On Thu, Dec 17, 2015 at 5:49 AM, Savolainen, Petri (Nokia - FI/Espoo) >> <petri.savolainen@nokia.com> wrote: >> >> >> >> What e.g. scheduler or TM would do with a odp_cos_t which is associated >> with queue, pool and drop_policy? >> >> >> >> typedef struct odp_cls_cos_param { >> odp_queue_t queue; /**< Queue associated with CoS */ >> odp_pool_t pool; /**< Pool associated with CoS */ >> odp_drop_e drop_policy; /**< Drop policy associated with CoS */ >> } odp_cls_cos_param_t; >> >> >> >> >> >> Scheduler (odp_queue_t) has its own priority scheduling and so does TM. >> Cos_param.queue has meaning only when packet enters the system and is stored >> first time to the queue. After that packet QoS (or CoS) is defined by a >> chain of queues, scheduling, application processing and TM. The initial >> input queue, pool or packet input drop policy does not matter in later >> stages. So this CoS (and those parameters) are significant only for the >> classifier. This CoS specification end when classifier does enqueue or drop >> the packet. >> >> >> >> Sure, if entire ODP API level CoS property is specified later, we can use >> odp_cos_t for that but currently that’s not the case. >> >> >> >> DPI PMR would be very different from classification PMRs. DPI API would >> deal with regular expressions, etc language to express multitude of >> different combinations of possible packet payload … whereas packet input >> classification is shallow/fixed packet inspection on predefined header >> fields (to keep it fast). >> >> >> >> -Petri >> >> >> >> >> >> >> >> From: EXT Bill Fischofer [mailto:bill.fischofer@linaro.org] >> Sent: Thursday, December 17, 2015 1:27 PM >> To: Savolainen, Petri (Nokia - FI/Espoo) >> Cc: Bala Manoharan; LNG ODP Mailman List >> >> >> Subject: Re: [lng-odp] [API-NEXT/PATCHv2 1/6] api: classification: add >> class of serivce create api >> >> >> >> I agree with that general convention, however a class of service is a >> first class entity (like a pool). It's why we have odp_pool_create() and >> parameterize that it's a buffer vs. packet pool instead of having separate >> odp_buffer_pool_create() and odp_packet_pool_create() APIs. Using that >> analogy odp_cos_create() is creating a class of service. The fact that a >> class of service is being used for classification purposes would be encoded >> (if required) in the new odp_cos_param_t struct used to qualify the >> creation. >> >> >> >> I agree that odp_drop_e is unspecific, however the logical qualification >> there would be odp_cos_drop_e (or _t) since it is an attribute on a class of >> service (you don't "drop" a classifier). Indeed the current typedef for >> odp_drop_e says enum odp_cos_drop so the result of the typedef should be an >> odp_cos_drop_t. >> >> >> >> Similarly for PMRs the PMR is again a first class object which is why >> odp_pmr_create() gives an odp_pmr_t. Again, for consistency if we needed to >> qualify it we'd add an odp_pmr_param_t struct to the create() call to do >> that. That would make sense if we wanted to have PMRs that were needed to be >> distinguished between use by classification vs. DPI, etc. >> >> >> >> On Thu, Dec 17, 2015 at 5:04 AM, Savolainen, Petri (Nokia - FI/Espoo) >> <petri.savolainen@nokia.com> wrote: >> >> The rationale behind this is that classification API should follow the >> same, prefixed naming convention than other APIs. CoS is a generally used >> term and e.g. scheduler, TM, packet, etc APIs could define also APIs around >> Class of Service. Here odp_cos_t is essentially a node in a classification >> hierarchy tree. It’s similarly to TM API odp_tm_node_t , which is correctly >> prefix and easy find when seen in code. >> >> >> >> Also other CLS API types (odp_drop_e, odp_pmr_t, odp_flowsig_t) should be >> prefixed since they are not self-explanatory part of classification API >> (only). For example, a future DPI API could define “Pattern Matching Rules”. >> >> >> >> -Petri >> >> >> >> >> >> From: lng-odp [mailto:lng-odp-bounces@lists.linaro.org] On Behalf Of EXT >> Bill Fischofer >> Sent: Thursday, December 17, 2015 11:59 AM >> To: Bala Manoharan >> Cc: LNG ODP Mailman List >> Subject: Re: [lng-odp] [API-NEXT/PATCHv2 1/6] api: classification: add >> class of serivce create api >> >> >> >> My argument is that odp_cls_cos_xxx is redundant and odp_cos_xxx is both >> less cumbersome and more intuitive for the application writer. Consider >> that if a Class of Service is only used in classification then there's no >> need for the redundant cls prefix. But if we ever extend classes of service >> to be used by another component (e.g., allowing pktios to refer to classes >> of service directly) then the cls prefix would be confusingly restrictive. >> >> >> >> Aside from that, the name change itself represents an API change and it's >> not clear what benefits are gained by doing this. I'm not sure where this >> requirement originated, but perhaps we should revisit the reasoning behind >> it. >> >> >> >> On Thu, Dec 17, 2015 at 12:37 AM, Bala Manoharan >> <bala.manoharan@linaro.org> wrote: >> >> The reason for having odp_cls_xxx is that class of service module >> contains class of service and packet matching rule and we need a >> common syntax to identify the module hence odp_cls_xxx is used as >> common prefix. >> >> I agree all the functions in classifier module needs to be changed to >> add the prefix odp_cls_xxx.I will add the rename as a separate patch >> for the remaining functions. >> >> Regards, >> Bala >> >> >> On 17 December 2015 at 08:51, Bill Fischofer <bill.fischofer@linaro.org> >> wrote: >> > >> > >> > On Wed, Dec 16, 2015 at 3:28 AM, Balasubramanian Manoharan >> > <bala.manoharan@linaro.org> wrote: >> >> >> >> class of service create function now takes pool, queue, drop policy and >> >> name as input parameters. >> >> Adds class of service parameter structure odp_cls_cos_param_t and >> >> initialization function odp_cls_cos_param_init() >> >> >> >> Signed-off-by: Balasubramanian Manoharan <bala.manoharan@linaro.org> >> >> --- >> >> v2: additional documentation and review comments from Petri >> >> >> >> include/odp/api/classification.h | 37 >> >> +++++++++++++++++++++++++++++++------ >> >> 1 file changed, 31 insertions(+), 6 deletions(-) >> >> >> >> diff --git a/include/odp/api/classification.h >> >> b/include/odp/api/classification.h >> >> index 725e1ab..4db5bf0 100644 >> >> --- a/include/odp/api/classification.h >> >> +++ b/include/odp/api/classification.h >> >> @@ -37,7 +37,7 @@ extern "C" { >> >> >> >> /** >> >> * @def ODP_COS_INVALID >> >> - * This value is returned from odp_cos_create() on failure, >> >> + * This value is returned from odp_cls_cos_create() on failure, >> >> * May also be used as a sink class of service that >> >> * results in packets being discarded. >> >> */ >> >> @@ -60,9 +60,9 @@ extern "C" { >> >> */ >> >> >> >> /** >> >> - * Class-of-service packet drop policies >> >> + * class of service packet drop policies >> >> */ >> >> -typedef enum odp_cos_drop { >> >> +typedef enum odp_cls_drop { >> >> ODP_COS_DROP_POOL, /**< Follow buffer pool drop policy */ >> >> ODP_COS_DROP_NEVER, /**< Never drop, ignoring buffer pool >> >> policy */ >> >> } odp_drop_e; >> >> @@ -89,14 +89,39 @@ typedef enum odp_cos_hdr_flow_fields { >> >> } odp_cos_hdr_flow_fields_e; >> >> >> >> /** >> >> + * Class of service parameters >> >> + * 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_drop_e drop_policy; /**< Drop policy associated with CoS */ >> >> +} odp_cls_cos_param_t; >> > >> > >> > Why odp_cls_cos_param_t and not just odp_cos_param_t ? See remarks >> > under >> > odp_cls_cos_create(). If that's changed then it would be consistent to >> > change this as well. >> > >> >> >> >> + >> >> +/** >> >> + * Initialize class of service parameters >> >> + * >> >> + * Initialize an odp_cls_cos_param_t to its default value for all >> >> fields >> >> + * >> >> + * @param param Address of the odp_cls_cos_param_t to be initialized >> >> + */ >> >> +void odp_cls_cos_param_init(odp_cls_cos_param_t *param); >> >> + >> >> +/** >> >> * Create a class-of-service >> >> * >> >> - * @param[in] name String intended for debugging purposes. >> >> + * @param name String intended for debugging purposes. >> >> * >> >> - * @return Class of service instance identifier >> >> + * @param param class of service parameters >> >> + * >> >> + * @retval class of service handle >> >> * @retval ODP_COS_INVALID on failure. >> >> + * >> >> + * @note ODP_QUEUE_INVALID and ODP_POOL_INVALID are valid values for >> >> queue >> >> + * and pool associated with a class of service and when any one of >> >> these >> >> values >> >> + * are configured as INVALID then the packets assigned to the CoS gets >> >> dropped. >> >> */ >> >> -odp_cos_t odp_cos_create(const char *name); >> >> +odp_cos_t odp_cls_cos_create(const char *name, odp_cls_cos_param_t >> >> *param); >> > >> > >> > The additional parameter seems reasonable, but it's not clear why the >> > function needs to be renamed cos already stands for Class of Service so >> > odp_cls_cos_create() expands to ODP Classifier Class of Service Create >> > which >> > seems somewhat redundant. It's also inconsistent in that the created >> > object >> > is of type odp_cos_t, not odp_cls_cos_t and is non-symmetric with the >> > existing odp_cos_destroy() function. I'd leave this as just >> > odp_cos_create() with the additional parameter. >> >> >> >> >> >> /** >> >> * Discard a class-of-service along with all its associated resources >> >> -- >> >> 1.9.1 >> >> >> >> _______________________________________________ >> >> lng-odp mailing list >> >> lng-odp@lists.linaro.org >> >> https://lists.linaro.org/mailman/listinfo/lng-odp >> > >> > >> >> -- >> Regards, >> Bala >> >> >> >> >> >> > >
If we want to split this into multiple patches for review convenience that's fine. I'd just like to be sure that we don't have a series of API breaks for 1.6, 1.7, etc. Best to do that all at once on a release boundary. On Thu, Dec 17, 2015 at 6:25 AM, Bala Manoharan <bala.manoharan@linaro.org> wrote: > On 17 December 2015 at 17:31, Bill Fischofer <bill.fischofer@linaro.org> > wrote: > > Ok, then the patch should be reworked to do this. No point in dragging > out > > the process through a series of disruptive changes. > > Yes. The ultimate goal is to add this prefix to all existing APIs. > This patch only adds a new api and it was better to add the new API > with the latest convention. > > I will post a separate patch which renames all the existing > classification APIs with the prefix odp_cls_xxx > > Regards, > Bala > > > > On Thu, Dec 17, 2015 at 5:56 AM, Savolainen, Petri (Nokia - FI/Espoo) > > <petri.savolainen@nokia.com> wrote: > >> > >> Yes, that’s the end goal of this process. All classification API calls, > >> types, defines, … are prefixed with odp_cls_. > >> > >> > >> > >> -Petri > >> > >> > >> > >> > >> > >> From: EXT Bill Fischofer [mailto:bill.fischofer@linaro.org] > >> Sent: Thursday, December 17, 2015 1:54 PM > >> > >> > >> To: Savolainen, Petri (Nokia - FI/Espoo) > >> Cc: Bala Manoharan; LNG ODP Mailman List > >> Subject: Re: [lng-odp] [API-NEXT/PATCHv2 1/6] api: classification: add > >> class of serivce create api > >> > >> > >> > >> Fair enough, but then it seems we then need to add the odp_cls prefix > to a > >> lot more APIs and typedefs in the existing classification.h to be fully > >> consistent. Right now there's a confusing mix of those that do and do > not > >> have this prefix. > >> > >> > >> > >> On Thu, Dec 17, 2015 at 5:49 AM, Savolainen, Petri (Nokia - FI/Espoo) > >> <petri.savolainen@nokia.com> wrote: > >> > >> > >> > >> What e.g. scheduler or TM would do with a odp_cos_t which is associated > >> with queue, pool and drop_policy? > >> > >> > >> > >> typedef struct odp_cls_cos_param { > >> odp_queue_t queue; /**< Queue associated with CoS */ > >> odp_pool_t pool; /**< Pool associated with CoS */ > >> odp_drop_e drop_policy; /**< Drop policy associated with CoS */ > >> } odp_cls_cos_param_t; > >> > >> > >> > >> > >> > >> Scheduler (odp_queue_t) has its own priority scheduling and so does TM. > >> Cos_param.queue has meaning only when packet enters the system and is > stored > >> first time to the queue. After that packet QoS (or CoS) is defined by a > >> chain of queues, scheduling, application processing and TM. The initial > >> input queue, pool or packet input drop policy does not matter in later > >> stages. So this CoS (and those parameters) are significant only for the > >> classifier. This CoS specification end when classifier does enqueue or > drop > >> the packet. > >> > >> > >> > >> Sure, if entire ODP API level CoS property is specified later, we can > use > >> odp_cos_t for that but currently that’s not the case. > >> > >> > >> > >> DPI PMR would be very different from classification PMRs. DPI API would > >> deal with regular expressions, etc language to express multitude of > >> different combinations of possible packet payload … whereas packet input > >> classification is shallow/fixed packet inspection on predefined header > >> fields (to keep it fast). > >> > >> > >> > >> -Petri > >> > >> > >> > >> > >> > >> > >> > >> From: EXT Bill Fischofer [mailto:bill.fischofer@linaro.org] > >> Sent: Thursday, December 17, 2015 1:27 PM > >> To: Savolainen, Petri (Nokia - FI/Espoo) > >> Cc: Bala Manoharan; LNG ODP Mailman List > >> > >> > >> Subject: Re: [lng-odp] [API-NEXT/PATCHv2 1/6] api: classification: add > >> class of serivce create api > >> > >> > >> > >> I agree with that general convention, however a class of service is a > >> first class entity (like a pool). It's why we have odp_pool_create() > and > >> parameterize that it's a buffer vs. packet pool instead of having > separate > >> odp_buffer_pool_create() and odp_packet_pool_create() APIs. Using that > >> analogy odp_cos_create() is creating a class of service. The fact that > a > >> class of service is being used for classification purposes would be > encoded > >> (if required) in the new odp_cos_param_t struct used to qualify the > >> creation. > >> > >> > >> > >> I agree that odp_drop_e is unspecific, however the logical qualification > >> there would be odp_cos_drop_e (or _t) since it is an attribute on a > class of > >> service (you don't "drop" a classifier). Indeed the current typedef for > >> odp_drop_e says enum odp_cos_drop so the result of the typedef should > be an > >> odp_cos_drop_t. > >> > >> > >> > >> Similarly for PMRs the PMR is again a first class object which is why > >> odp_pmr_create() gives an odp_pmr_t. Again, for consistency if we > needed to > >> qualify it we'd add an odp_pmr_param_t struct to the create() call to do > >> that. That would make sense if we wanted to have PMRs that were needed > to be > >> distinguished between use by classification vs. DPI, etc. > >> > >> > >> > >> On Thu, Dec 17, 2015 at 5:04 AM, Savolainen, Petri (Nokia - FI/Espoo) > >> <petri.savolainen@nokia.com> wrote: > >> > >> The rationale behind this is that classification API should follow the > >> same, prefixed naming convention than other APIs. CoS is a generally > used > >> term and e.g. scheduler, TM, packet, etc APIs could define also APIs > around > >> Class of Service. Here odp_cos_t is essentially a node in a > classification > >> hierarchy tree. It’s similarly to TM API odp_tm_node_t , which is > correctly > >> prefix and easy find when seen in code. > >> > >> > >> > >> Also other CLS API types (odp_drop_e, odp_pmr_t, odp_flowsig_t) should > be > >> prefixed since they are not self-explanatory part of classification API > >> (only). For example, a future DPI API could define “Pattern Matching > Rules”. > >> > >> > >> > >> -Petri > >> > >> > >> > >> > >> > >> From: lng-odp [mailto:lng-odp-bounces@lists.linaro.org] On Behalf Of > EXT > >> Bill Fischofer > >> Sent: Thursday, December 17, 2015 11:59 AM > >> To: Bala Manoharan > >> Cc: LNG ODP Mailman List > >> Subject: Re: [lng-odp] [API-NEXT/PATCHv2 1/6] api: classification: add > >> class of serivce create api > >> > >> > >> > >> My argument is that odp_cls_cos_xxx is redundant and odp_cos_xxx is both > >> less cumbersome and more intuitive for the application writer. Consider > >> that if a Class of Service is only used in classification then there's > no > >> need for the redundant cls prefix. But if we ever extend classes of > service > >> to be used by another component (e.g., allowing pktios to refer to > classes > >> of service directly) then the cls prefix would be confusingly > restrictive. > >> > >> > >> > >> Aside from that, the name change itself represents an API change and > it's > >> not clear what benefits are gained by doing this. I'm not sure where > this > >> requirement originated, but perhaps we should revisit the reasoning > behind > >> it. > >> > >> > >> > >> On Thu, Dec 17, 2015 at 12:37 AM, Bala Manoharan > >> <bala.manoharan@linaro.org> wrote: > >> > >> The reason for having odp_cls_xxx is that class of service module > >> contains class of service and packet matching rule and we need a > >> common syntax to identify the module hence odp_cls_xxx is used as > >> common prefix. > >> > >> I agree all the functions in classifier module needs to be changed to > >> add the prefix odp_cls_xxx.I will add the rename as a separate patch > >> for the remaining functions. > >> > >> Regards, > >> Bala > >> > >> > >> On 17 December 2015 at 08:51, Bill Fischofer <bill.fischofer@linaro.org > > > >> wrote: > >> > > >> > > >> > On Wed, Dec 16, 2015 at 3:28 AM, Balasubramanian Manoharan > >> > <bala.manoharan@linaro.org> wrote: > >> >> > >> >> class of service create function now takes pool, queue, drop policy > and > >> >> name as input parameters. > >> >> Adds class of service parameter structure odp_cls_cos_param_t and > >> >> initialization function odp_cls_cos_param_init() > >> >> > >> >> Signed-off-by: Balasubramanian Manoharan <bala.manoharan@linaro.org> > >> >> --- > >> >> v2: additional documentation and review comments from Petri > >> >> > >> >> include/odp/api/classification.h | 37 > >> >> +++++++++++++++++++++++++++++++------ > >> >> 1 file changed, 31 insertions(+), 6 deletions(-) > >> >> > >> >> diff --git a/include/odp/api/classification.h > >> >> b/include/odp/api/classification.h > >> >> index 725e1ab..4db5bf0 100644 > >> >> --- a/include/odp/api/classification.h > >> >> +++ b/include/odp/api/classification.h > >> >> @@ -37,7 +37,7 @@ extern "C" { > >> >> > >> >> /** > >> >> * @def ODP_COS_INVALID > >> >> - * This value is returned from odp_cos_create() on failure, > >> >> + * This value is returned from odp_cls_cos_create() on failure, > >> >> * May also be used as a sink class of service that > >> >> * results in packets being discarded. > >> >> */ > >> >> @@ -60,9 +60,9 @@ extern "C" { > >> >> */ > >> >> > >> >> /** > >> >> - * Class-of-service packet drop policies > >> >> + * class of service packet drop policies > >> >> */ > >> >> -typedef enum odp_cos_drop { > >> >> +typedef enum odp_cls_drop { > >> >> ODP_COS_DROP_POOL, /**< Follow buffer pool drop policy */ > >> >> ODP_COS_DROP_NEVER, /**< Never drop, ignoring buffer pool > >> >> policy */ > >> >> } odp_drop_e; > >> >> @@ -89,14 +89,39 @@ typedef enum odp_cos_hdr_flow_fields { > >> >> } odp_cos_hdr_flow_fields_e; > >> >> > >> >> /** > >> >> + * Class of service parameters > >> >> + * 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_drop_e drop_policy; /**< Drop policy associated with CoS > */ > >> >> +} odp_cls_cos_param_t; > >> > > >> > > >> > Why odp_cls_cos_param_t and not just odp_cos_param_t ? See remarks > >> > under > >> > odp_cls_cos_create(). If that's changed then it would be consistent > to > >> > change this as well. > >> > > >> >> > >> >> + > >> >> +/** > >> >> + * Initialize class of service parameters > >> >> + * > >> >> + * Initialize an odp_cls_cos_param_t to its default value for all > >> >> fields > >> >> + * > >> >> + * @param param Address of the odp_cls_cos_param_t to be > initialized > >> >> + */ > >> >> +void odp_cls_cos_param_init(odp_cls_cos_param_t *param); > >> >> + > >> >> +/** > >> >> * Create a class-of-service > >> >> * > >> >> - * @param[in] name String intended for debugging purposes. > >> >> + * @param name String intended for debugging purposes. > >> >> * > >> >> - * @return Class of service instance identifier > >> >> + * @param param class of service parameters > >> >> + * > >> >> + * @retval class of service handle > >> >> * @retval ODP_COS_INVALID on failure. > >> >> + * > >> >> + * @note ODP_QUEUE_INVALID and ODP_POOL_INVALID are valid values for > >> >> queue > >> >> + * and pool associated with a class of service and when any one of > >> >> these > >> >> values > >> >> + * are configured as INVALID then the packets assigned to the CoS > gets > >> >> dropped. > >> >> */ > >> >> -odp_cos_t odp_cos_create(const char *name); > >> >> +odp_cos_t odp_cls_cos_create(const char *name, odp_cls_cos_param_t > >> >> *param); > >> > > >> > > >> > The additional parameter seems reasonable, but it's not clear why the > >> > function needs to be renamed cos already stands for Class of Service > so > >> > odp_cls_cos_create() expands to ODP Classifier Class of Service Create > >> > which > >> > seems somewhat redundant. It's also inconsistent in that the created > >> > object > >> > is of type odp_cos_t, not odp_cls_cos_t and is non-symmetric with the > >> > existing odp_cos_destroy() function. I'd leave this as just > >> > odp_cos_create() with the additional parameter. > >> >> > >> >> > >> >> /** > >> >> * Discard a class-of-service along with all its associated > resources > >> >> -- > >> >> 1.9.1 > >> >> > >> >> _______________________________________________ > >> >> lng-odp mailing list > >> >> lng-odp@lists.linaro.org > >> >> https://lists.linaro.org/mailman/listinfo/lng-odp > >> > > >> > > >> > >> -- > >> Regards, > >> Bala > >> > >> > >> > >> > >> > >> > > > > > > > > -- > Regards, > Bala >
BTW, subject to that caveat I did review and test this patch series so Reviewed-and-Tested-by: Bill Fischofer <bill.fischofer@linaro.org> On Thu, Dec 17, 2015 at 6:26 AM, Bill Fischofer <bill.fischofer@linaro.org> wrote: > If we want to split this into multiple patches for review convenience > that's fine. I'd just like to be sure that we don't have a series of API > breaks for 1.6, 1.7, etc. Best to do that all at once on a release > boundary. > > On Thu, Dec 17, 2015 at 6:25 AM, Bala Manoharan <bala.manoharan@linaro.org > > wrote: > >> On 17 December 2015 at 17:31, Bill Fischofer <bill.fischofer@linaro.org> >> wrote: >> > Ok, then the patch should be reworked to do this. No point in dragging >> out >> > the process through a series of disruptive changes. >> >> Yes. The ultimate goal is to add this prefix to all existing APIs. >> This patch only adds a new api and it was better to add the new API >> with the latest convention. >> >> I will post a separate patch which renames all the existing >> classification APIs with the prefix odp_cls_xxx >> >> Regards, >> Bala >> > >> > On Thu, Dec 17, 2015 at 5:56 AM, Savolainen, Petri (Nokia - FI/Espoo) >> > <petri.savolainen@nokia.com> wrote: >> >> >> >> Yes, that’s the end goal of this process. All classification API calls, >> >> types, defines, … are prefixed with odp_cls_. >> >> >> >> >> >> >> >> -Petri >> >> >> >> >> >> >> >> >> >> >> >> From: EXT Bill Fischofer [mailto:bill.fischofer@linaro.org] >> >> Sent: Thursday, December 17, 2015 1:54 PM >> >> >> >> >> >> To: Savolainen, Petri (Nokia - FI/Espoo) >> >> Cc: Bala Manoharan; LNG ODP Mailman List >> >> Subject: Re: [lng-odp] [API-NEXT/PATCHv2 1/6] api: classification: add >> >> class of serivce create api >> >> >> >> >> >> >> >> Fair enough, but then it seems we then need to add the odp_cls prefix >> to a >> >> lot more APIs and typedefs in the existing classification.h to be fully >> >> consistent. Right now there's a confusing mix of those that do and do >> not >> >> have this prefix. >> >> >> >> >> >> >> >> On Thu, Dec 17, 2015 at 5:49 AM, Savolainen, Petri (Nokia - FI/Espoo) >> >> <petri.savolainen@nokia.com> wrote: >> >> >> >> >> >> >> >> What e.g. scheduler or TM would do with a odp_cos_t which is associated >> >> with queue, pool and drop_policy? >> >> >> >> >> >> >> >> typedef struct odp_cls_cos_param { >> >> odp_queue_t queue; /**< Queue associated with CoS */ >> >> odp_pool_t pool; /**< Pool associated with CoS */ >> >> odp_drop_e drop_policy; /**< Drop policy associated with CoS */ >> >> } odp_cls_cos_param_t; >> >> >> >> >> >> >> >> >> >> >> >> Scheduler (odp_queue_t) has its own priority scheduling and so does TM. >> >> Cos_param.queue has meaning only when packet enters the system and is >> stored >> >> first time to the queue. After that packet QoS (or CoS) is defined by a >> >> chain of queues, scheduling, application processing and TM. The initial >> >> input queue, pool or packet input drop policy does not matter in later >> >> stages. So this CoS (and those parameters) are significant only for the >> >> classifier. This CoS specification end when classifier does enqueue or >> drop >> >> the packet. >> >> >> >> >> >> >> >> Sure, if entire ODP API level CoS property is specified later, we can >> use >> >> odp_cos_t for that but currently that’s not the case. >> >> >> >> >> >> >> >> DPI PMR would be very different from classification PMRs. DPI API would >> >> deal with regular expressions, etc language to express multitude of >> >> different combinations of possible packet payload … whereas packet >> input >> >> classification is shallow/fixed packet inspection on predefined header >> >> fields (to keep it fast). >> >> >> >> >> >> >> >> -Petri >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> From: EXT Bill Fischofer [mailto:bill.fischofer@linaro.org] >> >> Sent: Thursday, December 17, 2015 1:27 PM >> >> To: Savolainen, Petri (Nokia - FI/Espoo) >> >> Cc: Bala Manoharan; LNG ODP Mailman List >> >> >> >> >> >> Subject: Re: [lng-odp] [API-NEXT/PATCHv2 1/6] api: classification: add >> >> class of serivce create api >> >> >> >> >> >> >> >> I agree with that general convention, however a class of service is a >> >> first class entity (like a pool). It's why we have odp_pool_create() >> and >> >> parameterize that it's a buffer vs. packet pool instead of having >> separate >> >> odp_buffer_pool_create() and odp_packet_pool_create() APIs. Using that >> >> analogy odp_cos_create() is creating a class of service. The fact >> that a >> >> class of service is being used for classification purposes would be >> encoded >> >> (if required) in the new odp_cos_param_t struct used to qualify the >> >> creation. >> >> >> >> >> >> >> >> I agree that odp_drop_e is unspecific, however the logical >> qualification >> >> there would be odp_cos_drop_e (or _t) since it is an attribute on a >> class of >> >> service (you don't "drop" a classifier). Indeed the current typedef >> for >> >> odp_drop_e says enum odp_cos_drop so the result of the typedef should >> be an >> >> odp_cos_drop_t. >> >> >> >> >> >> >> >> Similarly for PMRs the PMR is again a first class object which is why >> >> odp_pmr_create() gives an odp_pmr_t. Again, for consistency if we >> needed to >> >> qualify it we'd add an odp_pmr_param_t struct to the create() call to >> do >> >> that. That would make sense if we wanted to have PMRs that were needed >> to be >> >> distinguished between use by classification vs. DPI, etc. >> >> >> >> >> >> >> >> On Thu, Dec 17, 2015 at 5:04 AM, Savolainen, Petri (Nokia - FI/Espoo) >> >> <petri.savolainen@nokia.com> wrote: >> >> >> >> The rationale behind this is that classification API should follow the >> >> same, prefixed naming convention than other APIs. CoS is a generally >> used >> >> term and e.g. scheduler, TM, packet, etc APIs could define also APIs >> around >> >> Class of Service. Here odp_cos_t is essentially a node in a >> classification >> >> hierarchy tree. It’s similarly to TM API odp_tm_node_t , which is >> correctly >> >> prefix and easy find when seen in code. >> >> >> >> >> >> >> >> Also other CLS API types (odp_drop_e, odp_pmr_t, odp_flowsig_t) should >> be >> >> prefixed since they are not self-explanatory part of classification API >> >> (only). For example, a future DPI API could define “Pattern Matching >> Rules”. >> >> >> >> >> >> >> >> -Petri >> >> >> >> >> >> >> >> >> >> >> >> From: lng-odp [mailto:lng-odp-bounces@lists.linaro.org] On Behalf Of >> EXT >> >> Bill Fischofer >> >> Sent: Thursday, December 17, 2015 11:59 AM >> >> To: Bala Manoharan >> >> Cc: LNG ODP Mailman List >> >> Subject: Re: [lng-odp] [API-NEXT/PATCHv2 1/6] api: classification: add >> >> class of serivce create api >> >> >> >> >> >> >> >> My argument is that odp_cls_cos_xxx is redundant and odp_cos_xxx is >> both >> >> less cumbersome and more intuitive for the application writer. >> Consider >> >> that if a Class of Service is only used in classification then there's >> no >> >> need for the redundant cls prefix. But if we ever extend classes of >> service >> >> to be used by another component (e.g., allowing pktios to refer to >> classes >> >> of service directly) then the cls prefix would be confusingly >> restrictive. >> >> >> >> >> >> >> >> Aside from that, the name change itself represents an API change and >> it's >> >> not clear what benefits are gained by doing this. I'm not sure where >> this >> >> requirement originated, but perhaps we should revisit the reasoning >> behind >> >> it. >> >> >> >> >> >> >> >> On Thu, Dec 17, 2015 at 12:37 AM, Bala Manoharan >> >> <bala.manoharan@linaro.org> wrote: >> >> >> >> The reason for having odp_cls_xxx is that class of service module >> >> contains class of service and packet matching rule and we need a >> >> common syntax to identify the module hence odp_cls_xxx is used as >> >> common prefix. >> >> >> >> I agree all the functions in classifier module needs to be changed to >> >> add the prefix odp_cls_xxx.I will add the rename as a separate patch >> >> for the remaining functions. >> >> >> >> Regards, >> >> Bala >> >> >> >> >> >> On 17 December 2015 at 08:51, Bill Fischofer < >> bill.fischofer@linaro.org> >> >> wrote: >> >> > >> >> > >> >> > On Wed, Dec 16, 2015 at 3:28 AM, Balasubramanian Manoharan >> >> > <bala.manoharan@linaro.org> wrote: >> >> >> >> >> >> class of service create function now takes pool, queue, drop policy >> and >> >> >> name as input parameters. >> >> >> Adds class of service parameter structure odp_cls_cos_param_t and >> >> >> initialization function odp_cls_cos_param_init() >> >> >> >> >> >> Signed-off-by: Balasubramanian Manoharan <bala.manoharan@linaro.org >> > >> >> >> --- >> >> >> v2: additional documentation and review comments from Petri >> >> >> >> >> >> include/odp/api/classification.h | 37 >> >> >> +++++++++++++++++++++++++++++++------ >> >> >> 1 file changed, 31 insertions(+), 6 deletions(-) >> >> >> >> >> >> diff --git a/include/odp/api/classification.h >> >> >> b/include/odp/api/classification.h >> >> >> index 725e1ab..4db5bf0 100644 >> >> >> --- a/include/odp/api/classification.h >> >> >> +++ b/include/odp/api/classification.h >> >> >> @@ -37,7 +37,7 @@ extern "C" { >> >> >> >> >> >> /** >> >> >> * @def ODP_COS_INVALID >> >> >> - * This value is returned from odp_cos_create() on failure, >> >> >> + * This value is returned from odp_cls_cos_create() on failure, >> >> >> * May also be used as a sink class of service that >> >> >> * results in packets being discarded. >> >> >> */ >> >> >> @@ -60,9 +60,9 @@ extern "C" { >> >> >> */ >> >> >> >> >> >> /** >> >> >> - * Class-of-service packet drop policies >> >> >> + * class of service packet drop policies >> >> >> */ >> >> >> -typedef enum odp_cos_drop { >> >> >> +typedef enum odp_cls_drop { >> >> >> ODP_COS_DROP_POOL, /**< Follow buffer pool drop policy */ >> >> >> ODP_COS_DROP_NEVER, /**< Never drop, ignoring buffer pool >> >> >> policy */ >> >> >> } odp_drop_e; >> >> >> @@ -89,14 +89,39 @@ typedef enum odp_cos_hdr_flow_fields { >> >> >> } odp_cos_hdr_flow_fields_e; >> >> >> >> >> >> /** >> >> >> + * Class of service parameters >> >> >> + * 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_drop_e drop_policy; /**< Drop policy associated with >> CoS */ >> >> >> +} odp_cls_cos_param_t; >> >> > >> >> > >> >> > Why odp_cls_cos_param_t and not just odp_cos_param_t ? See remarks >> >> > under >> >> > odp_cls_cos_create(). If that's changed then it would be consistent >> to >> >> > change this as well. >> >> > >> >> >> >> >> >> + >> >> >> +/** >> >> >> + * Initialize class of service parameters >> >> >> + * >> >> >> + * Initialize an odp_cls_cos_param_t to its default value for all >> >> >> fields >> >> >> + * >> >> >> + * @param param Address of the odp_cls_cos_param_t to be >> initialized >> >> >> + */ >> >> >> +void odp_cls_cos_param_init(odp_cls_cos_param_t *param); >> >> >> + >> >> >> +/** >> >> >> * Create a class-of-service >> >> >> * >> >> >> - * @param[in] name String intended for debugging purposes. >> >> >> + * @param name String intended for debugging purposes. >> >> >> * >> >> >> - * @return Class of service instance identifier >> >> >> + * @param param class of service parameters >> >> >> + * >> >> >> + * @retval class of service handle >> >> >> * @retval ODP_COS_INVALID on failure. >> >> >> + * >> >> >> + * @note ODP_QUEUE_INVALID and ODP_POOL_INVALID are valid values >> for >> >> >> queue >> >> >> + * and pool associated with a class of service and when any one of >> >> >> these >> >> >> values >> >> >> + * are configured as INVALID then the packets assigned to the CoS >> gets >> >> >> dropped. >> >> >> */ >> >> >> -odp_cos_t odp_cos_create(const char *name); >> >> >> +odp_cos_t odp_cls_cos_create(const char *name, odp_cls_cos_param_t >> >> >> *param); >> >> > >> >> > >> >> > The additional parameter seems reasonable, but it's not clear why the >> >> > function needs to be renamed cos already stands for Class of Service >> so >> >> > odp_cls_cos_create() expands to ODP Classifier Class of Service >> Create >> >> > which >> >> > seems somewhat redundant. It's also inconsistent in that the created >> >> > object >> >> > is of type odp_cos_t, not odp_cls_cos_t and is non-symmetric with the >> >> > existing odp_cos_destroy() function. I'd leave this as just >> >> > odp_cos_create() with the additional parameter. >> >> >> >> >> >> >> >> >> /** >> >> >> * Discard a class-of-service along with all its associated >> resources >> >> >> -- >> >> >> 1.9.1 >> >> >> >> >> >> _______________________________________________ >> >> >> lng-odp mailing list >> >> >> lng-odp@lists.linaro.org >> >> >> https://lists.linaro.org/mailman/listinfo/lng-odp >> >> > >> >> > >> >> >> >> -- >> >> Regards, >> >> Bala >> >> >> >> >> >> >> >> >> >> >> >> >> > >> > >> >> >> >> -- >> Regards, >> Bala >> > >
Agreed. But this odp_cls_cos_create() API change is not just a rename and it modifies the parameters to CoS create and hence it was better to do this in a separate review process. Regards, Bala On 17 December 2015 at 17:56, Bill Fischofer <bill.fischofer@linaro.org> wrote: > If we want to split this into multiple patches for review convenience that's > fine. I'd just like to be sure that we don't have a series of API breaks for > 1.6, 1.7, etc. Best to do that all at once on a release boundary. > > On Thu, Dec 17, 2015 at 6:25 AM, Bala Manoharan <bala.manoharan@linaro.org> > wrote: >> >> On 17 December 2015 at 17:31, Bill Fischofer <bill.fischofer@linaro.org> >> wrote: >> > Ok, then the patch should be reworked to do this. No point in dragging >> > out >> > the process through a series of disruptive changes. >> >> Yes. The ultimate goal is to add this prefix to all existing APIs. >> This patch only adds a new api and it was better to add the new API >> with the latest convention. >> >> I will post a separate patch which renames all the existing >> classification APIs with the prefix odp_cls_xxx >> >> Regards, >> Bala >> > >> > On Thu, Dec 17, 2015 at 5:56 AM, Savolainen, Petri (Nokia - FI/Espoo) >> > <petri.savolainen@nokia.com> wrote: >> >> >> >> Yes, that’s the end goal of this process. All classification API calls, >> >> types, defines, … are prefixed with odp_cls_. >> >> >> >> >> >> >> >> -Petri >> >> >> >> >> >> >> >> >> >> >> >> From: EXT Bill Fischofer [mailto:bill.fischofer@linaro.org] >> >> Sent: Thursday, December 17, 2015 1:54 PM >> >> >> >> >> >> To: Savolainen, Petri (Nokia - FI/Espoo) >> >> Cc: Bala Manoharan; LNG ODP Mailman List >> >> Subject: Re: [lng-odp] [API-NEXT/PATCHv2 1/6] api: classification: add >> >> class of serivce create api >> >> >> >> >> >> >> >> Fair enough, but then it seems we then need to add the odp_cls prefix >> >> to a >> >> lot more APIs and typedefs in the existing classification.h to be fully >> >> consistent. Right now there's a confusing mix of those that do and do >> >> not >> >> have this prefix. >> >> >> >> >> >> >> >> On Thu, Dec 17, 2015 at 5:49 AM, Savolainen, Petri (Nokia - FI/Espoo) >> >> <petri.savolainen@nokia.com> wrote: >> >> >> >> >> >> >> >> What e.g. scheduler or TM would do with a odp_cos_t which is associated >> >> with queue, pool and drop_policy? >> >> >> >> >> >> >> >> typedef struct odp_cls_cos_param { >> >> odp_queue_t queue; /**< Queue associated with CoS */ >> >> odp_pool_t pool; /**< Pool associated with CoS */ >> >> odp_drop_e drop_policy; /**< Drop policy associated with CoS */ >> >> } odp_cls_cos_param_t; >> >> >> >> >> >> >> >> >> >> >> >> Scheduler (odp_queue_t) has its own priority scheduling and so does TM. >> >> Cos_param.queue has meaning only when packet enters the system and is >> >> stored >> >> first time to the queue. After that packet QoS (or CoS) is defined by a >> >> chain of queues, scheduling, application processing and TM. The initial >> >> input queue, pool or packet input drop policy does not matter in later >> >> stages. So this CoS (and those parameters) are significant only for the >> >> classifier. This CoS specification end when classifier does enqueue or >> >> drop >> >> the packet. >> >> >> >> >> >> >> >> Sure, if entire ODP API level CoS property is specified later, we can >> >> use >> >> odp_cos_t for that but currently that’s not the case. >> >> >> >> >> >> >> >> DPI PMR would be very different from classification PMRs. DPI API would >> >> deal with regular expressions, etc language to express multitude of >> >> different combinations of possible packet payload … whereas packet >> >> input >> >> classification is shallow/fixed packet inspection on predefined header >> >> fields (to keep it fast). >> >> >> >> >> >> >> >> -Petri >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> From: EXT Bill Fischofer [mailto:bill.fischofer@linaro.org] >> >> Sent: Thursday, December 17, 2015 1:27 PM >> >> To: Savolainen, Petri (Nokia - FI/Espoo) >> >> Cc: Bala Manoharan; LNG ODP Mailman List >> >> >> >> >> >> Subject: Re: [lng-odp] [API-NEXT/PATCHv2 1/6] api: classification: add >> >> class of serivce create api >> >> >> >> >> >> >> >> I agree with that general convention, however a class of service is a >> >> first class entity (like a pool). It's why we have odp_pool_create() >> >> and >> >> parameterize that it's a buffer vs. packet pool instead of having >> >> separate >> >> odp_buffer_pool_create() and odp_packet_pool_create() APIs. Using that >> >> analogy odp_cos_create() is creating a class of service. The fact that >> >> a >> >> class of service is being used for classification purposes would be >> >> encoded >> >> (if required) in the new odp_cos_param_t struct used to qualify the >> >> creation. >> >> >> >> >> >> >> >> I agree that odp_drop_e is unspecific, however the logical >> >> qualification >> >> there would be odp_cos_drop_e (or _t) since it is an attribute on a >> >> class of >> >> service (you don't "drop" a classifier). Indeed the current typedef >> >> for >> >> odp_drop_e says enum odp_cos_drop so the result of the typedef should >> >> be an >> >> odp_cos_drop_t. >> >> >> >> >> >> >> >> Similarly for PMRs the PMR is again a first class object which is why >> >> odp_pmr_create() gives an odp_pmr_t. Again, for consistency if we >> >> needed to >> >> qualify it we'd add an odp_pmr_param_t struct to the create() call to >> >> do >> >> that. That would make sense if we wanted to have PMRs that were needed >> >> to be >> >> distinguished between use by classification vs. DPI, etc. >> >> >> >> >> >> >> >> On Thu, Dec 17, 2015 at 5:04 AM, Savolainen, Petri (Nokia - FI/Espoo) >> >> <petri.savolainen@nokia.com> wrote: >> >> >> >> The rationale behind this is that classification API should follow the >> >> same, prefixed naming convention than other APIs. CoS is a generally >> >> used >> >> term and e.g. scheduler, TM, packet, etc APIs could define also APIs >> >> around >> >> Class of Service. Here odp_cos_t is essentially a node in a >> >> classification >> >> hierarchy tree. It’s similarly to TM API odp_tm_node_t , which is >> >> correctly >> >> prefix and easy find when seen in code. >> >> >> >> >> >> >> >> Also other CLS API types (odp_drop_e, odp_pmr_t, odp_flowsig_t) should >> >> be >> >> prefixed since they are not self-explanatory part of classification API >> >> (only). For example, a future DPI API could define “Pattern Matching >> >> Rules”. >> >> >> >> >> >> >> >> -Petri >> >> >> >> >> >> >> >> >> >> >> >> From: lng-odp [mailto:lng-odp-bounces@lists.linaro.org] On Behalf Of >> >> EXT >> >> Bill Fischofer >> >> Sent: Thursday, December 17, 2015 11:59 AM >> >> To: Bala Manoharan >> >> Cc: LNG ODP Mailman List >> >> Subject: Re: [lng-odp] [API-NEXT/PATCHv2 1/6] api: classification: add >> >> class of serivce create api >> >> >> >> >> >> >> >> My argument is that odp_cls_cos_xxx is redundant and odp_cos_xxx is >> >> both >> >> less cumbersome and more intuitive for the application writer. >> >> Consider >> >> that if a Class of Service is only used in classification then there's >> >> no >> >> need for the redundant cls prefix. But if we ever extend classes of >> >> service >> >> to be used by another component (e.g., allowing pktios to refer to >> >> classes >> >> of service directly) then the cls prefix would be confusingly >> >> restrictive. >> >> >> >> >> >> >> >> Aside from that, the name change itself represents an API change and >> >> it's >> >> not clear what benefits are gained by doing this. I'm not sure where >> >> this >> >> requirement originated, but perhaps we should revisit the reasoning >> >> behind >> >> it. >> >> >> >> >> >> >> >> On Thu, Dec 17, 2015 at 12:37 AM, Bala Manoharan >> >> <bala.manoharan@linaro.org> wrote: >> >> >> >> The reason for having odp_cls_xxx is that class of service module >> >> contains class of service and packet matching rule and we need a >> >> common syntax to identify the module hence odp_cls_xxx is used as >> >> common prefix. >> >> >> >> I agree all the functions in classifier module needs to be changed to >> >> add the prefix odp_cls_xxx.I will add the rename as a separate patch >> >> for the remaining functions. >> >> >> >> Regards, >> >> Bala >> >> >> >> >> >> On 17 December 2015 at 08:51, Bill Fischofer >> >> <bill.fischofer@linaro.org> >> >> wrote: >> >> > >> >> > >> >> > On Wed, Dec 16, 2015 at 3:28 AM, Balasubramanian Manoharan >> >> > <bala.manoharan@linaro.org> wrote: >> >> >> >> >> >> class of service create function now takes pool, queue, drop policy >> >> >> and >> >> >> name as input parameters. >> >> >> Adds class of service parameter structure odp_cls_cos_param_t and >> >> >> initialization function odp_cls_cos_param_init() >> >> >> >> >> >> Signed-off-by: Balasubramanian Manoharan <bala.manoharan@linaro.org> >> >> >> --- >> >> >> v2: additional documentation and review comments from Petri >> >> >> >> >> >> include/odp/api/classification.h | 37 >> >> >> +++++++++++++++++++++++++++++++------ >> >> >> 1 file changed, 31 insertions(+), 6 deletions(-) >> >> >> >> >> >> diff --git a/include/odp/api/classification.h >> >> >> b/include/odp/api/classification.h >> >> >> index 725e1ab..4db5bf0 100644 >> >> >> --- a/include/odp/api/classification.h >> >> >> +++ b/include/odp/api/classification.h >> >> >> @@ -37,7 +37,7 @@ extern "C" { >> >> >> >> >> >> /** >> >> >> * @def ODP_COS_INVALID >> >> >> - * This value is returned from odp_cos_create() on failure, >> >> >> + * This value is returned from odp_cls_cos_create() on failure, >> >> >> * May also be used as a sink class of service that >> >> >> * results in packets being discarded. >> >> >> */ >> >> >> @@ -60,9 +60,9 @@ extern "C" { >> >> >> */ >> >> >> >> >> >> /** >> >> >> - * Class-of-service packet drop policies >> >> >> + * class of service packet drop policies >> >> >> */ >> >> >> -typedef enum odp_cos_drop { >> >> >> +typedef enum odp_cls_drop { >> >> >> ODP_COS_DROP_POOL, /**< Follow buffer pool drop policy */ >> >> >> ODP_COS_DROP_NEVER, /**< Never drop, ignoring buffer pool >> >> >> policy */ >> >> >> } odp_drop_e; >> >> >> @@ -89,14 +89,39 @@ typedef enum odp_cos_hdr_flow_fields { >> >> >> } odp_cos_hdr_flow_fields_e; >> >> >> >> >> >> /** >> >> >> + * Class of service parameters >> >> >> + * 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_drop_e drop_policy; /**< Drop policy associated with CoS >> >> >> */ >> >> >> +} odp_cls_cos_param_t; >> >> > >> >> > >> >> > Why odp_cls_cos_param_t and not just odp_cos_param_t ? See remarks >> >> > under >> >> > odp_cls_cos_create(). If that's changed then it would be consistent >> >> > to >> >> > change this as well. >> >> > >> >> >> >> >> >> + >> >> >> +/** >> >> >> + * Initialize class of service parameters >> >> >> + * >> >> >> + * Initialize an odp_cls_cos_param_t to its default value for all >> >> >> fields >> >> >> + * >> >> >> + * @param param Address of the odp_cls_cos_param_t to be >> >> >> initialized >> >> >> + */ >> >> >> +void odp_cls_cos_param_init(odp_cls_cos_param_t *param); >> >> >> + >> >> >> +/** >> >> >> * Create a class-of-service >> >> >> * >> >> >> - * @param[in] name String intended for debugging purposes. >> >> >> + * @param name String intended for debugging purposes. >> >> >> * >> >> >> - * @return Class of service instance identifier >> >> >> + * @param param class of service parameters >> >> >> + * >> >> >> + * @retval class of service handle >> >> >> * @retval ODP_COS_INVALID on failure. >> >> >> + * >> >> >> + * @note ODP_QUEUE_INVALID and ODP_POOL_INVALID are valid values >> >> >> for >> >> >> queue >> >> >> + * and pool associated with a class of service and when any one of >> >> >> these >> >> >> values >> >> >> + * are configured as INVALID then the packets assigned to the CoS >> >> >> gets >> >> >> dropped. >> >> >> */ >> >> >> -odp_cos_t odp_cos_create(const char *name); >> >> >> +odp_cos_t odp_cls_cos_create(const char *name, odp_cls_cos_param_t >> >> >> *param); >> >> > >> >> > >> >> > The additional parameter seems reasonable, but it's not clear why the >> >> > function needs to be renamed cos already stands for Class of Service >> >> > so >> >> > odp_cls_cos_create() expands to ODP Classifier Class of Service >> >> > Create >> >> > which >> >> > seems somewhat redundant. It's also inconsistent in that the created >> >> > object >> >> > is of type odp_cos_t, not odp_cls_cos_t and is non-symmetric with the >> >> > existing odp_cos_destroy() function. I'd leave this as just >> >> > odp_cos_create() with the additional parameter. >> >> >> >> >> >> >> >> >> /** >> >> >> * Discard a class-of-service along with all its associated >> >> >> resources >> >> >> -- >> >> >> 1.9.1 >> >> >> >> >> >> _______________________________________________ >> >> >> lng-odp mailing list >> >> >> lng-odp@lists.linaro.org >> >> >> https://lists.linaro.org/mailman/listinfo/lng-odp >> >> > >> >> > >> >> >> >> -- >> >> Regards, >> >> Bala >> >> >> >> >> >> >> >> >> >> >> >> >> > >> > >> >> >> >> -- >> Regards, >> Bala > >
Thanks Bill. @Petri: Can you please provide your reviewed-by to this API change. Regards, Bala On 17 December 2015 at 17:58, Bill Fischofer <bill.fischofer@linaro.org> wrote: > BTW, subject to that caveat I did review and test this patch series so > > Reviewed-and-Tested-by: Bill Fischofer <bill.fischofer@linaro.org> > > On Thu, Dec 17, 2015 at 6:26 AM, Bill Fischofer <bill.fischofer@linaro.org> > wrote: >> >> If we want to split this into multiple patches for review convenience >> that's fine. I'd just like to be sure that we don't have a series of API >> breaks for 1.6, 1.7, etc. Best to do that all at once on a release >> boundary. >> >> On Thu, Dec 17, 2015 at 6:25 AM, Bala Manoharan >> <bala.manoharan@linaro.org> wrote: >>> >>> On 17 December 2015 at 17:31, Bill Fischofer <bill.fischofer@linaro.org> >>> wrote: >>> > Ok, then the patch should be reworked to do this. No point in dragging >>> > out >>> > the process through a series of disruptive changes. >>> >>> Yes. The ultimate goal is to add this prefix to all existing APIs. >>> This patch only adds a new api and it was better to add the new API >>> with the latest convention. >>> >>> I will post a separate patch which renames all the existing >>> classification APIs with the prefix odp_cls_xxx >>> >>> Regards, >>> Bala >>> > >>> > On Thu, Dec 17, 2015 at 5:56 AM, Savolainen, Petri (Nokia - FI/Espoo) >>> > <petri.savolainen@nokia.com> wrote: >>> >> >>> >> Yes, that’s the end goal of this process. All classification API >>> >> calls, >>> >> types, defines, … are prefixed with odp_cls_. >>> >> >>> >> >>> >> >>> >> -Petri >>> >> >>> >> >>> >> >>> >> >>> >> >>> >> From: EXT Bill Fischofer [mailto:bill.fischofer@linaro.org] >>> >> Sent: Thursday, December 17, 2015 1:54 PM >>> >> >>> >> >>> >> To: Savolainen, Petri (Nokia - FI/Espoo) >>> >> Cc: Bala Manoharan; LNG ODP Mailman List >>> >> Subject: Re: [lng-odp] [API-NEXT/PATCHv2 1/6] api: classification: add >>> >> class of serivce create api >>> >> >>> >> >>> >> >>> >> Fair enough, but then it seems we then need to add the odp_cls prefix >>> >> to a >>> >> lot more APIs and typedefs in the existing classification.h to be >>> >> fully >>> >> consistent. Right now there's a confusing mix of those that do and do >>> >> not >>> >> have this prefix. >>> >> >>> >> >>> >> >>> >> On Thu, Dec 17, 2015 at 5:49 AM, Savolainen, Petri (Nokia - FI/Espoo) >>> >> <petri.savolainen@nokia.com> wrote: >>> >> >>> >> >>> >> >>> >> What e.g. scheduler or TM would do with a odp_cos_t which is >>> >> associated >>> >> with queue, pool and drop_policy? >>> >> >>> >> >>> >> >>> >> typedef struct odp_cls_cos_param { >>> >> odp_queue_t queue; /**< Queue associated with CoS */ >>> >> odp_pool_t pool; /**< Pool associated with CoS */ >>> >> odp_drop_e drop_policy; /**< Drop policy associated with CoS */ >>> >> } odp_cls_cos_param_t; >>> >> >>> >> >>> >> >>> >> >>> >> >>> >> Scheduler (odp_queue_t) has its own priority scheduling and so does >>> >> TM. >>> >> Cos_param.queue has meaning only when packet enters the system and is >>> >> stored >>> >> first time to the queue. After that packet QoS (or CoS) is defined by >>> >> a >>> >> chain of queues, scheduling, application processing and TM. The >>> >> initial >>> >> input queue, pool or packet input drop policy does not matter in later >>> >> stages. So this CoS (and those parameters) are significant only for >>> >> the >>> >> classifier. This CoS specification end when classifier does enqueue or >>> >> drop >>> >> the packet. >>> >> >>> >> >>> >> >>> >> Sure, if entire ODP API level CoS property is specified later, we can >>> >> use >>> >> odp_cos_t for that but currently that’s not the case. >>> >> >>> >> >>> >> >>> >> DPI PMR would be very different from classification PMRs. DPI API >>> >> would >>> >> deal with regular expressions, etc language to express multitude of >>> >> different combinations of possible packet payload … whereas packet >>> >> input >>> >> classification is shallow/fixed packet inspection on predefined header >>> >> fields (to keep it fast). >>> >> >>> >> >>> >> >>> >> -Petri >>> >> >>> >> >>> >> >>> >> >>> >> >>> >> >>> >> >>> >> From: EXT Bill Fischofer [mailto:bill.fischofer@linaro.org] >>> >> Sent: Thursday, December 17, 2015 1:27 PM >>> >> To: Savolainen, Petri (Nokia - FI/Espoo) >>> >> Cc: Bala Manoharan; LNG ODP Mailman List >>> >> >>> >> >>> >> Subject: Re: [lng-odp] [API-NEXT/PATCHv2 1/6] api: classification: add >>> >> class of serivce create api >>> >> >>> >> >>> >> >>> >> I agree with that general convention, however a class of service is a >>> >> first class entity (like a pool). It's why we have odp_pool_create() >>> >> and >>> >> parameterize that it's a buffer vs. packet pool instead of having >>> >> separate >>> >> odp_buffer_pool_create() and odp_packet_pool_create() APIs. Using >>> >> that >>> >> analogy odp_cos_create() is creating a class of service. The fact >>> >> that a >>> >> class of service is being used for classification purposes would be >>> >> encoded >>> >> (if required) in the new odp_cos_param_t struct used to qualify the >>> >> creation. >>> >> >>> >> >>> >> >>> >> I agree that odp_drop_e is unspecific, however the logical >>> >> qualification >>> >> there would be odp_cos_drop_e (or _t) since it is an attribute on a >>> >> class of >>> >> service (you don't "drop" a classifier). Indeed the current typedef >>> >> for >>> >> odp_drop_e says enum odp_cos_drop so the result of the typedef should >>> >> be an >>> >> odp_cos_drop_t. >>> >> >>> >> >>> >> >>> >> Similarly for PMRs the PMR is again a first class object which is why >>> >> odp_pmr_create() gives an odp_pmr_t. Again, for consistency if we >>> >> needed to >>> >> qualify it we'd add an odp_pmr_param_t struct to the create() call to >>> >> do >>> >> that. That would make sense if we wanted to have PMRs that were needed >>> >> to be >>> >> distinguished between use by classification vs. DPI, etc. >>> >> >>> >> >>> >> >>> >> On Thu, Dec 17, 2015 at 5:04 AM, Savolainen, Petri (Nokia - FI/Espoo) >>> >> <petri.savolainen@nokia.com> wrote: >>> >> >>> >> The rationale behind this is that classification API should follow the >>> >> same, prefixed naming convention than other APIs. CoS is a generally >>> >> used >>> >> term and e.g. scheduler, TM, packet, etc APIs could define also APIs >>> >> around >>> >> Class of Service. Here odp_cos_t is essentially a node in a >>> >> classification >>> >> hierarchy tree. It’s similarly to TM API odp_tm_node_t , which is >>> >> correctly >>> >> prefix and easy find when seen in code. >>> >> >>> >> >>> >> >>> >> Also other CLS API types (odp_drop_e, odp_pmr_t, odp_flowsig_t) should >>> >> be >>> >> prefixed since they are not self-explanatory part of classification >>> >> API >>> >> (only). For example, a future DPI API could define “Pattern Matching >>> >> Rules”. >>> >> >>> >> >>> >> >>> >> -Petri >>> >> >>> >> >>> >> >>> >> >>> >> >>> >> From: lng-odp [mailto:lng-odp-bounces@lists.linaro.org] On Behalf Of >>> >> EXT >>> >> Bill Fischofer >>> >> Sent: Thursday, December 17, 2015 11:59 AM >>> >> To: Bala Manoharan >>> >> Cc: LNG ODP Mailman List >>> >> Subject: Re: [lng-odp] [API-NEXT/PATCHv2 1/6] api: classification: add >>> >> class of serivce create api >>> >> >>> >> >>> >> >>> >> My argument is that odp_cls_cos_xxx is redundant and odp_cos_xxx is >>> >> both >>> >> less cumbersome and more intuitive for the application writer. >>> >> Consider >>> >> that if a Class of Service is only used in classification then there's >>> >> no >>> >> need for the redundant cls prefix. But if we ever extend classes of >>> >> service >>> >> to be used by another component (e.g., allowing pktios to refer to >>> >> classes >>> >> of service directly) then the cls prefix would be confusingly >>> >> restrictive. >>> >> >>> >> >>> >> >>> >> Aside from that, the name change itself represents an API change and >>> >> it's >>> >> not clear what benefits are gained by doing this. I'm not sure where >>> >> this >>> >> requirement originated, but perhaps we should revisit the reasoning >>> >> behind >>> >> it. >>> >> >>> >> >>> >> >>> >> On Thu, Dec 17, 2015 at 12:37 AM, Bala Manoharan >>> >> <bala.manoharan@linaro.org> wrote: >>> >> >>> >> The reason for having odp_cls_xxx is that class of service module >>> >> contains class of service and packet matching rule and we need a >>> >> common syntax to identify the module hence odp_cls_xxx is used as >>> >> common prefix. >>> >> >>> >> I agree all the functions in classifier module needs to be changed to >>> >> add the prefix odp_cls_xxx.I will add the rename as a separate patch >>> >> for the remaining functions. >>> >> >>> >> Regards, >>> >> Bala >>> >> >>> >> >>> >> On 17 December 2015 at 08:51, Bill Fischofer >>> >> <bill.fischofer@linaro.org> >>> >> wrote: >>> >> > >>> >> > >>> >> > On Wed, Dec 16, 2015 at 3:28 AM, Balasubramanian Manoharan >>> >> > <bala.manoharan@linaro.org> wrote: >>> >> >> >>> >> >> class of service create function now takes pool, queue, drop policy >>> >> >> and >>> >> >> name as input parameters. >>> >> >> Adds class of service parameter structure odp_cls_cos_param_t and >>> >> >> initialization function odp_cls_cos_param_init() >>> >> >> >>> >> >> Signed-off-by: Balasubramanian Manoharan >>> >> >> <bala.manoharan@linaro.org> >>> >> >> --- >>> >> >> v2: additional documentation and review comments from Petri >>> >> >> >>> >> >> include/odp/api/classification.h | 37 >>> >> >> +++++++++++++++++++++++++++++++------ >>> >> >> 1 file changed, 31 insertions(+), 6 deletions(-) >>> >> >> >>> >> >> diff --git a/include/odp/api/classification.h >>> >> >> b/include/odp/api/classification.h >>> >> >> index 725e1ab..4db5bf0 100644 >>> >> >> --- a/include/odp/api/classification.h >>> >> >> +++ b/include/odp/api/classification.h >>> >> >> @@ -37,7 +37,7 @@ extern "C" { >>> >> >> >>> >> >> /** >>> >> >> * @def ODP_COS_INVALID >>> >> >> - * This value is returned from odp_cos_create() on failure, >>> >> >> + * This value is returned from odp_cls_cos_create() on failure, >>> >> >> * May also be used as a sink class of service that >>> >> >> * results in packets being discarded. >>> >> >> */ >>> >> >> @@ -60,9 +60,9 @@ extern "C" { >>> >> >> */ >>> >> >> >>> >> >> /** >>> >> >> - * Class-of-service packet drop policies >>> >> >> + * class of service packet drop policies >>> >> >> */ >>> >> >> -typedef enum odp_cos_drop { >>> >> >> +typedef enum odp_cls_drop { >>> >> >> ODP_COS_DROP_POOL, /**< Follow buffer pool drop policy >>> >> >> */ >>> >> >> ODP_COS_DROP_NEVER, /**< Never drop, ignoring buffer >>> >> >> pool >>> >> >> policy */ >>> >> >> } odp_drop_e; >>> >> >> @@ -89,14 +89,39 @@ typedef enum odp_cos_hdr_flow_fields { >>> >> >> } odp_cos_hdr_flow_fields_e; >>> >> >> >>> >> >> /** >>> >> >> + * Class of service parameters >>> >> >> + * 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_drop_e drop_policy; /**< Drop policy associated with >>> >> >> CoS */ >>> >> >> +} odp_cls_cos_param_t; >>> >> > >>> >> > >>> >> > Why odp_cls_cos_param_t and not just odp_cos_param_t ? See remarks >>> >> > under >>> >> > odp_cls_cos_create(). If that's changed then it would be consistent >>> >> > to >>> >> > change this as well. >>> >> > >>> >> >> >>> >> >> + >>> >> >> +/** >>> >> >> + * Initialize class of service parameters >>> >> >> + * >>> >> >> + * Initialize an odp_cls_cos_param_t to its default value for all >>> >> >> fields >>> >> >> + * >>> >> >> + * @param param Address of the odp_cls_cos_param_t to be >>> >> >> initialized >>> >> >> + */ >>> >> >> +void odp_cls_cos_param_init(odp_cls_cos_param_t *param); >>> >> >> + >>> >> >> +/** >>> >> >> * Create a class-of-service >>> >> >> * >>> >> >> - * @param[in] name String intended for debugging purposes. >>> >> >> + * @param name String intended for debugging purposes. >>> >> >> * >>> >> >> - * @return Class of service instance identifier >>> >> >> + * @param param class of service parameters >>> >> >> + * >>> >> >> + * @retval class of service handle >>> >> >> * @retval ODP_COS_INVALID on failure. >>> >> >> + * >>> >> >> + * @note ODP_QUEUE_INVALID and ODP_POOL_INVALID are valid values >>> >> >> for >>> >> >> queue >>> >> >> + * and pool associated with a class of service and when any one of >>> >> >> these >>> >> >> values >>> >> >> + * are configured as INVALID then the packets assigned to the CoS >>> >> >> gets >>> >> >> dropped. >>> >> >> */ >>> >> >> -odp_cos_t odp_cos_create(const char *name); >>> >> >> +odp_cos_t odp_cls_cos_create(const char *name, odp_cls_cos_param_t >>> >> >> *param); >>> >> > >>> >> > >>> >> > The additional parameter seems reasonable, but it's not clear why >>> >> > the >>> >> > function needs to be renamed cos already stands for Class of Service >>> >> > so >>> >> > odp_cls_cos_create() expands to ODP Classifier Class of Service >>> >> > Create >>> >> > which >>> >> > seems somewhat redundant. It's also inconsistent in that the >>> >> > created >>> >> > object >>> >> > is of type odp_cos_t, not odp_cls_cos_t and is non-symmetric with >>> >> > the >>> >> > existing odp_cos_destroy() function. I'd leave this as just >>> >> > odp_cos_create() with the additional parameter. >>> >> >> >>> >> >> >>> >> >> /** >>> >> >> * Discard a class-of-service along with all its associated >>> >> >> resources >>> >> >> -- >>> >> >> 1.9.1 >>> >> >> >>> >> >> _______________________________________________ >>> >> >> lng-odp mailing list >>> >> >> lng-odp@lists.linaro.org >>> >> >> https://lists.linaro.org/mailman/listinfo/lng-odp >>> >> > >>> >> > >>> >> >>> >> -- >>> >> Regards, >>> >> Bala >>> >> >>> >> >>> >> >>> >> >>> >> >>> >> >>> > >>> > >>> >>> >>> >>> -- >>> Regards, >>> Bala >> >> >
The only thing is patches needed to reorder patches: if you stay on patch: validation: classification: add class of service create api odp_classification_basic.c:18:2: error: implicit declaration of function 'odp_cos_create' [-Werror=implicit-function-declaration] cos = odp_cos_create(name); Maxim. On 12/17/2015 16:20, Savolainen, Petri (Nokia - FI/Espoo) wrote: > [API-NEXT/PATCHv2 1/6] api: classification: add class of serivce create api > ^^^^^^^ > > typo can be fixed during merge: serivce -> service > > > Reviewed-by: Petri Savolainen <petri.savolainen@nokia.com> > > >> -----Original Message----- >> From: lng-odp [mailto:lng-odp-bounces@lists.linaro.org] On Behalf Of EXT >> Balasubramanian Manoharan >> Sent: Wednesday, December 16, 2015 11:29 AM >> To: lng-odp@lists.linaro.org >> Subject: [lng-odp] [API-NEXT/PATCHv2 1/6] api: classification: add class >> of serivce create api >> >> class of service create function now takes pool, queue, drop policy and >> name as input parameters. >> Adds class of service parameter structure odp_cls_cos_param_t and >> initialization function odp_cls_cos_param_init() >> >> Signed-off-by: Balasubramanian Manoharan <bala.manoharan@linaro.org> >> --- >> v2: additional documentation and review comments from Petri >> >> include/odp/api/classification.h | 37 +++++++++++++++++++++++++++++++---- >> -- >> 1 file changed, 31 insertions(+), 6 deletions(-) >> >> diff --git a/include/odp/api/classification.h >> b/include/odp/api/classification.h >> index 725e1ab..4db5bf0 100644 >> --- a/include/odp/api/classification.h >> +++ b/include/odp/api/classification.h >> @@ -37,7 +37,7 @@ extern "C" { >> >> /** >> * @def ODP_COS_INVALID >> - * This value is returned from odp_cos_create() on failure, >> + * This value is returned from odp_cls_cos_create() on failure, >> * May also be used as a sink class of service that >> * results in packets being discarded. >> */ >> @@ -60,9 +60,9 @@ extern "C" { >> */ >> >> /** >> - * Class-of-service packet drop policies >> + * class of service packet drop policies >> */ >> -typedef enum odp_cos_drop { >> +typedef enum odp_cls_drop { >> ODP_COS_DROP_POOL, /**< Follow buffer pool drop policy */ >> ODP_COS_DROP_NEVER, /**< Never drop, ignoring buffer pool policy >> */ >> } odp_drop_e; >> @@ -89,14 +89,39 @@ typedef enum odp_cos_hdr_flow_fields { >> } odp_cos_hdr_flow_fields_e; >> >> /** >> + * Class of service parameters >> + * 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_drop_e drop_policy; /**< Drop policy associated with CoS */ >> +} odp_cls_cos_param_t; >> + >> +/** >> + * Initialize class of service parameters >> + * >> + * Initialize an odp_cls_cos_param_t to its default value for all fields >> + * >> + * @param param Address of the odp_cls_cos_param_t to be initialized >> + */ >> +void odp_cls_cos_param_init(odp_cls_cos_param_t *param); >> + >> +/** >> * Create a class-of-service >> * >> - * @param[in] name String intended for debugging purposes. >> + * @param name String intended for debugging purposes. >> * >> - * @return Class of service instance identifier >> + * @param param class of service parameters >> + * >> + * @retval class of service handle >> * @retval ODP_COS_INVALID on failure. >> + * >> + * @note ODP_QUEUE_INVALID and ODP_POOL_INVALID are valid values for >> queue >> + * and pool associated with a class of service and when any one of these >> values >> + * are configured as INVALID then the packets assigned to the CoS gets >> dropped. >> */ >> -odp_cos_t odp_cos_create(const char *name); >> +odp_cos_t odp_cls_cos_create(const char *name, odp_cls_cos_param_t >> *param); >> >> /** >> * Discard a class-of-service along with all its associated resources >> -- >> 1.9.1 >> >> _______________________________________________ >> lng-odp mailing list >> lng-odp@lists.linaro.org >> https://lists.linaro.org/mailman/listinfo/lng-odp > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > https://lists.linaro.org/mailman/listinfo/lng-odp
Merged with contatitating patches to keep git bisectability. Maxim. On 12/17/2015 16:20, Savolainen, Petri (Nokia - FI/Espoo) wrote: > [API-NEXT/PATCHv2 1/6] api: classification: add class of serivce create api > ^^^^^^^ > > typo can be fixed during merge: serivce -> service > > > Reviewed-by: Petri Savolainen <petri.savolainen@nokia.com> > > >> -----Original Message----- >> From: lng-odp [mailto:lng-odp-bounces@lists.linaro.org] On Behalf Of EXT >> Balasubramanian Manoharan >> Sent: Wednesday, December 16, 2015 11:29 AM >> To: lng-odp@lists.linaro.org >> Subject: [lng-odp] [API-NEXT/PATCHv2 1/6] api: classification: add class >> of serivce create api >> >> class of service create function now takes pool, queue, drop policy and >> name as input parameters. >> Adds class of service parameter structure odp_cls_cos_param_t and >> initialization function odp_cls_cos_param_init() >> >> Signed-off-by: Balasubramanian Manoharan <bala.manoharan@linaro.org> >> --- >> v2: additional documentation and review comments from Petri >> >> include/odp/api/classification.h | 37 +++++++++++++++++++++++++++++++---- >> -- >> 1 file changed, 31 insertions(+), 6 deletions(-) >> >> diff --git a/include/odp/api/classification.h >> b/include/odp/api/classification.h >> index 725e1ab..4db5bf0 100644 >> --- a/include/odp/api/classification.h >> +++ b/include/odp/api/classification.h >> @@ -37,7 +37,7 @@ extern "C" { >> >> /** >> * @def ODP_COS_INVALID >> - * This value is returned from odp_cos_create() on failure, >> + * This value is returned from odp_cls_cos_create() on failure, >> * May also be used as a sink class of service that >> * results in packets being discarded. >> */ >> @@ -60,9 +60,9 @@ extern "C" { >> */ >> >> /** >> - * Class-of-service packet drop policies >> + * class of service packet drop policies >> */ >> -typedef enum odp_cos_drop { >> +typedef enum odp_cls_drop { >> ODP_COS_DROP_POOL, /**< Follow buffer pool drop policy */ >> ODP_COS_DROP_NEVER, /**< Never drop, ignoring buffer pool policy >> */ >> } odp_drop_e; >> @@ -89,14 +89,39 @@ typedef enum odp_cos_hdr_flow_fields { >> } odp_cos_hdr_flow_fields_e; >> >> /** >> + * Class of service parameters >> + * 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_drop_e drop_policy; /**< Drop policy associated with CoS */ >> +} odp_cls_cos_param_t; >> + >> +/** >> + * Initialize class of service parameters >> + * >> + * Initialize an odp_cls_cos_param_t to its default value for all fields >> + * >> + * @param param Address of the odp_cls_cos_param_t to be initialized >> + */ >> +void odp_cls_cos_param_init(odp_cls_cos_param_t *param); >> + >> +/** >> * Create a class-of-service >> * >> - * @param[in] name String intended for debugging purposes. >> + * @param name String intended for debugging purposes. >> * >> - * @return Class of service instance identifier >> + * @param param class of service parameters >> + * >> + * @retval class of service handle >> * @retval ODP_COS_INVALID on failure. >> + * >> + * @note ODP_QUEUE_INVALID and ODP_POOL_INVALID are valid values for >> queue >> + * and pool associated with a class of service and when any one of these >> values >> + * are configured as INVALID then the packets assigned to the CoS gets >> dropped. >> */ >> -odp_cos_t odp_cos_create(const char *name); >> +odp_cos_t odp_cls_cos_create(const char *name, odp_cls_cos_param_t >> *param); >> >> /** >> * Discard a class-of-service along with all its associated resources >> -- >> 1.9.1 >> >> _______________________________________________ >> lng-odp mailing list >> lng-odp@lists.linaro.org >> https://lists.linaro.org/mailman/listinfo/lng-odp > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > https://lists.linaro.org/mailman/listinfo/lng-odp
diff --git a/include/odp/api/classification.h b/include/odp/api/classification.h index 725e1ab..4db5bf0 100644 --- a/include/odp/api/classification.h +++ b/include/odp/api/classification.h @@ -37,7 +37,7 @@ extern "C" { /** * @def ODP_COS_INVALID - * This value is returned from odp_cos_create() on failure, + * This value is returned from odp_cls_cos_create() on failure, * May also be used as a sink class of service that * results in packets being discarded. */ @@ -60,9 +60,9 @@ extern "C" { */ /** - * Class-of-service packet drop policies + * class of service packet drop policies */ -typedef enum odp_cos_drop { +typedef enum odp_cls_drop { ODP_COS_DROP_POOL, /**< Follow buffer pool drop policy */ ODP_COS_DROP_NEVER, /**< Never drop, ignoring buffer pool policy */ } odp_drop_e; @@ -89,14 +89,39 @@ typedef enum odp_cos_hdr_flow_fields { } odp_cos_hdr_flow_fields_e; /** + * Class of service parameters + * 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_drop_e drop_policy; /**< Drop policy associated with CoS */ +} odp_cls_cos_param_t; + +/** + * Initialize class of service parameters + * + * Initialize an odp_cls_cos_param_t to its default value for all fields + * + * @param param Address of the odp_cls_cos_param_t to be initialized + */ +void odp_cls_cos_param_init(odp_cls_cos_param_t *param); + +/** * Create a class-of-service * - * @param[in] name String intended for debugging purposes. + * @param name String intended for debugging purposes. * - * @return Class of service instance identifier + * @param param class of service parameters + * + * @retval class of service handle * @retval ODP_COS_INVALID on failure. + * + * @note ODP_QUEUE_INVALID and ODP_POOL_INVALID are valid values for queue + * and pool associated with a class of service and when any one of these values + * are configured as INVALID then the packets assigned to the CoS gets dropped. */ -odp_cos_t odp_cos_create(const char *name); +odp_cos_t odp_cls_cos_create(const char *name, odp_cls_cos_param_t *param); /** * Discard a class-of-service along with all its associated resources
class of service create function now takes pool, queue, drop policy and name as input parameters. Adds class of service parameter structure odp_cls_cos_param_t and initialization function odp_cls_cos_param_init() Signed-off-by: Balasubramanian Manoharan <bala.manoharan@linaro.org> --- v2: additional documentation and review comments from Petri include/odp/api/classification.h | 37 +++++++++++++++++++++++++++++++------ 1 file changed, 31 insertions(+), 6 deletions(-)