This is very good discussion, but +cc the ODP mailing list as this sort of detail belongs on the list so that others can see and contribute to it.
Bill On Thu, Apr 14, 2016 at 12:20 PM, Barry Spinney <[email protected]> wrote: > > > So my latest comments are in *green* below. We are starting to run out > > of bold primary colors, so we had better settle this soon. > > > > OK, in summary I will remove priority setting, require TCP as a condition > > for setting ECN and research the ability/need to change a subset of > > of the dscp field (perhaps a dscp mask parameter). > > > > Thanx Barry. > > > > > > *From:* Bala Manoharan [mailto:[email protected]] > *Sent:* Thursday, April 14, 2016 12:13 PM > > *To:* Barry Spinney > *Cc:* Bill Fischofer; Savolainen, Petri (Nokia - FI/Espoo) > *Subject:* Re: New traffic_mngr API > > > > Hi Barry, > > > > My comments are in blue below. > > > Regards, > Bala > > > > On 14 April 2016 at 20:45, Barry Spinney <[email protected]> wrote: > > > > My comments are in red below. > > In summary, as indicated by my comments below, the ONLY TM marking > > change I am planning on doing NOW is the merge the IPv4 and IPv6 function > > calls and capability fields. Based upon additional feedback from you > (see Questions > > included in the red comments) and others like Petri and Bill, there could > still be > > more changes. > > > > Thanx Barry > > > > *From:* Bala Manoharan [mailto:[email protected]] > *Sent:* Thursday, April 14, 2016 8:01 AM > *To:* Barry Spinney > *Cc:* Bill Fischofer; Savolainen, Petri (Nokia - FI/Espoo) > *Subject:* Re: New traffic_mngr API > > Hi Barry, > > IMO, we can drop the idea of having priority value (both for vlan and > dscp) set as part of marking in this first version (Monarch version) so > that the API gets simplified. > > So, my question is WHY did Cavium consider having the ability to set the > priority based on egress color valuable in the first place? > > And then why drop this feature? > > So my thinking is that if Cavium had legitimate uses for this capability > it seems reasonable that some other SOC will eventually want it. > > That leads me to want to keep it. > > HOWEVER IF Cavium eliminated this capability because further customer use > case research showed no one wanted this feature, > > then that would lead me to eliminate it. > > So I propose keeping it UNTIL I hear from you that Cavium discovered that > NO one wanted or could benefit from this feature. > > > > [Bala] I can say from Cavium that this feature of changing priority will > NOT be implemented since there is no use-case and IMO we can remove this > ability. We can add this feature in the future if some other platforms > support the same. > > > > *[Barry] Ok OK, I give up. I will remove priority changing.* > > > > int odp_tm_vlan_marking(odp_tm_t odp_tm, > > odp_packet_color_t color, > > odp_bool_t priority_enabled, > > uint8_t new_priority, > > odp_bool_t > drop_eligible_enabled); > > In your proposed vlan marking API defined above it expects the user to > provide both drop eligibility and new_priority value, hence if a platform > supports only drop eligibility then the function cannot succeed since it > currently expects the implementation to support both. > > No. The new_priority value is ignored unless priority_enabled is TRUE. If > the platform only supports the setting of drop_eligible_enabled, then > > an ODP App can detect this be calling this function twice (per color?) – > once with one enabled TRUE and the other FALSE and then vice versa. > > If the call fails then the App knows that that capability does not exist > on this platform (at least for that color). If BOTH calls fail for SOME > color > > (say green) but if at least one succeeded for a different color, then the > App knows that that color is not supported for Marking. > > > > [Bala] Yes. My concern was to avoid calling of this function twice by the > application instead we can split this into two functions one setting > drop_eligibility and other setting priority so that for platforms which do > not set priority value there is no need to configure this boolean. > > > > Also you have defined two different APIs for TOS marking each one for ipv4 > and ipv6, I am not sure if such a differentiation is needed, we can combine > them into a single API. > > I also don’t know if such a differentiation is needed. I have no problem > combining them into a single *_IP_TOS_* function call . > > [Bala] Okay. > > > > *[Barry] I have completed the changes to traffic_mngr.h and > platform/linux-generic/include/odp_traffic_mngr_internal.h * > > *to** merge these into ONE, and am almost finished with the > platform/linux-generic/odp_traffic_mngr.c changes.* > > > > We can have the following assumptions while defining the Marking API for > simplicity > > * The marking APIs are applied only when the incoming packet contains the > relevant header (either VLAN, TCP or IPv4/ipv6) when the relevant header > is not available no change is done to the packet. > > I think that this is already clearly spelled out and implemented as such – > EXCEPT that the TOS changes (e.g. ECN) do NOT depend on the IP protocol. > > IF you think that, say, the ECN changes should ONLY happen for TCP pkts, > then PLEASE get back to me so I can add a comment to this effect > > and make a small change to the implementation. > > Bottom line - I have no problem adding a “TCP” condition to the *_ > ip_tos_marking() function, but we need to be clear as to whether this TCP > > condition applies to just ECN setting or to both enables, etc. > > > > [Bala] I would prefer adding ECN should ONLY happen for TCP pkts. > > > > *[Barry] OK. I will add this condition – but only for the ECN field > updates (i.e. the updating of the other 6 TOS bits can still happen* > > *for** non-TCP pkts). This is based upon my interpretation of your brief > answer to my question.* > > > > * DE --> DE bit is set to 1 when the same configuration is made by the > application. For eg when the application sets drop_eligibility as enabled > for YELLOW or RED color packet then the packets in those colors will have > the DE bit set. > > I think that this is already done. > > [Bala] Okay > > > > * ECN --> Valid value is only (ECN-CE ie bit value 11) and the application > can set the value for two colors RED and YELLOW. > > So when ecn_enabled is set by the application for colors YELLOW or RED the > same bits will be set in the packet. > > I am having trouble understanding the sentence above. For example what > does it mean “Valid value is only (ECN-CE ie bit value 11)”? > > Does this mean that the only valid value in incoming pkts is 0b11? Or > that this value is the only value that any outgoing ECN changed > > pkt will have? Also what does “the same bits will be set in the packet”? > > > > [Bala] Maybe I was not clear in my explanation earlier, understanding was > that when you configure ecn_enable for a particular colour then ECN bit > will be set as 11 for the packets in that colour. > > > > * Drop precedence --> When the application sets the drop precedence as > enabled for YELLOW color then any packet of YELLOW color will have the drop > precedence set as MEDIUM based on their class. > > I THINK that this is already handled by the current proposal, by having > the user program the 6 bits of the TOS field that are NOT the ECN field to > have > > a value that is interpreted as “Drop Precedence MEDIUM” based upon RFC > 2597. > > QUESTION – what do you mean by the word classs in the end of the sentence > “MEDIUM based on their class”? > > There is currently no concept of a “class” that affects the RFC 2597 6-bit > subfield value in the ODP TM API. > > Should there be such a capability? What would the semantics be? What > would the use case be? > > For now I am going to ignore this “class” comment. > > > > [Bala] you can ignore this class comment for now. (I was referring to AF > classes in RFC 2597) > > My concern was that this API provides the ability to configure all 6-bits > on TOS which I would like to avoid. Instead I would prefer the application > to only configure Drop precedence as either enabled or disabled. > > So that when the drop precedence is enabled then for RED colour packet the > implementation sets the drop precedence as HIGH and if the packet colour is > YELLOW then implementation sets the drop precedence as MEDIUM. > > > > *[Barry] Let me research this a little bit more (i.e. looking at the > appropriate RFC’s).* > > > > When application sets drop precedence as enabled for RED color then any > packet of RED color will have the drop precedence as HIGH. > > I THINK that this is already handled by the current proposal, by having > the user program the 6 bits of the TOS field that are NOT the ECN field to > have > > a value that is interpreted as “Drop Precedence HIGH” based upon RFC > 2597. By the way, hasn’t RFC 2597 been superseded/replaced by RFC 3168? > > > > [Bala] This could be useful since some routers might not be ECN capable. > > Regards, > Bala > > > > On 14 April 2016 at 11:45, Bala Manoharan <[email protected]> > wrote: > > Hi Barry, > > > > I was drafting my reply on the other mail thread. Since you have sent this > new version. > > I will consolidate and provide my inputs on these files. > > > Regards, > Bala > > > > On 14 April 2016 at 11:34, Barry Spinney <[email protected]> wrote: > > So Bala, instead of waiting for your answers, I took the liberty to > > make some assumptions and take some guesses and come up with > > a complete Marking API proposal. This is now in my latest local > > copy of the TrafficMngr header file, which I have attached. > > > > Because I am such a nice guy, instead of trying to find the relevant > sections affecting the marking API's I have also attached two header files > called > > marking.h and capabilities.h . marking.h is just the direct Marking API > > functions. capabilities.h contains the entire capabilities/requirements > > API - including a proposed set of bits for indicating support for > > the Marking functionality. > > > > Because I am NOT such a nice guy, I have also attached my updated > > versions of the ODPH header files eth.h and ip.h so that perhaps > > you or Bill and Petri can take a quick look before I turn them into > > patch files (hopefully tomorrow). > > > > Anyhow this is all a sneak preview, and as such any feedback of THESE > > files needs to happen soon. > > > > Thanx Barry. > > > > (p.s. all of these files have passed the checkpatch.pl script and > > have been run thru doxygen). > > > > > > > > > > >
_______________________________________________ lng-odp mailing list [email protected] https://lists.linaro.org/mailman/listinfo/lng-odp
