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.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
