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.fischo...@linaro.org> wrote: > BTW, subject to that caveat I did review and test this patch series so > > Reviewed-and-Tested-by: Bill Fischofer <bill.fischo...@linaro.org> > > On Thu, Dec 17, 2015 at 6:26 AM, Bill Fischofer <bill.fischo...@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.manoha...@linaro.org> wrote: >>> >>> On 17 December 2015 at 17:31, Bill Fischofer <bill.fischo...@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.savolai...@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.fischo...@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.savolai...@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.fischo...@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.savolai...@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-boun...@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.manoha...@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.fischo...@linaro.org> >>> >> wrote: >>> >> > >>> >> > >>> >> > On Wed, Dec 16, 2015 at 3:28 AM, Balasubramanian Manoharan >>> >> > <bala.manoha...@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.manoha...@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 >> >> > -- Regards, Bala _______________________________________________ lng-odp mailing list lng-odp@lists.linaro.org https://lists.linaro.org/mailman/listinfo/lng-odp
