Petri Savolainen(psavol) replied on github web page:
include/odp/api/spec/std_types.h
@@ -32,6 +32,13 @@ extern "C" {
*/
/**
+ * @typedef odp_percent_t
+ * Use odp_percent_t for specifying fields that are percentages. It is a fixed
+ * point integer whose units are expressed as one-hundredth of a percent.
+ * Hence 100% is represented as integer value 10000.
+ */
+
Comment:
Replace "@typedef odp_percent_t" doxygen tag with the short comment line
("Percent type"). The tag is not needed anymore as the typedef follows the
comment.
Also, remove the extra line feed.
> Petri Savolainen(psavol) wrote:
> bp_ prefixes can be removed from "bp_enable" and "bp_threshold".
>
> "BP is enabled when queue or pool value is equal to or greater than the max
> backpressure threshold."
> This should refer to "resource usage" (which has been defined on RED
> documentation). "queue and pool value" are undefined terms.
>> Petri Savolainen(psavol) wrote:
>> Remove this extra line feed.
>>> Petri Savolainen(psavol) wrote:
>>> "... RED is enabled when the resource limit is equal to ..."
>>>
>>> This refers still to "resource limit", it should be "resource usage" which
>>> is a term that has been define above.
>>>> Petri Savolainen(psavol) wrote:
>>>> red_ prefix can be dropped from the struct field names. When red param is
>>>> used, the name of the variable is likely red or red_param, etc. So, after
>>>> removing the prefix it looks like this
>>>>
>>>> red.enable = 1;
>>>> red.threshold.packets.min = 1000;
>>>>
>>>> vs
>>>>
>>>> red.red_enable = 1;
>>>> red.red_threshold....
>>>> ...
>>>>> Petri Savolainen(psavol) wrote:
>>>>> "Reaches between" is not valid since drop probability is defined also
>>>>> when <min and >max. "... when the packets in the queue/pool reaches
>>>>> between the specified threshold." -> "... when the packets in the
>>>>> queue/pool cross specified threshold values."
>>>>>
>>>>> The "resource limit" needs to be specified: is it free or used space? Or
>>>>> is it different for pools and queues: free space in a pool, used space in
>>>>> a queue? May be the text is easier to understand with a bullet list:
>>>>> * drop probability is 100%, when resource usage > threshold.max
>>>>> * drop probability is 0%, when resource usage < threshold.min
>>>>> * drop probability is between 0 ... 100%, when resource usage is between
>>>>> threshold.min and threshold.max
>>>>>
>>>>> ... and then define what "resource usage" means. Pools: space used,
>>>>> queues packet/bytes in queue
>>>>>> Petri Savolainen(psavol) wrote:
>>>>>> This should match the type enum: byte. Also better description is
>>>>>> needed: e.g. "Sum of all data bytes of all packets". Packet size does
>>>>>> not tell if it's the size of one or many packets.
>>>>>>> Petri Savolainen(psavol) wrote:
>>>>>>> This should match the type enum: packet
>>>>>>>> Petri Savolainen(psavol) wrote:
>>>>>>>> When percent is not abbreviated, then this should not be either. So,
>>>>>>>> _PERCENT and _PACKET, or PCT and PKT.
>>>>>>>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>>>>>>>> As we discussed during today's ARCH call. `max` and `min` can be
>>>>>>>>> renamed `start` and `stop` to more accurately reflect the actions.
>>>>>>>>> RED / backpressure begins when utilization hits the `start` threshold
>>>>>>>>> and is deasserted when utilization drops back to the `stop`
>>>>>>>>> threshold. So `start` >= `stop`. If the two are equal it means there
>>>>>>>>> is no hysteresis.
>>>>>>>>>
>>>>>>>>> The question arises whether a third `max` threshold value should
>>>>>>>>> exist at which drops / backpressure is held at 100%. The idea here is
>>>>>>>>> to reserve a portion of the pool/queue resource for application use
>>>>>>>>> independent of use by PktIOs. Since this may not be feasible in all
>>>>>>>>> implementations this should probably be advertised with additional
>>>>>>>>> capability info.
>>>>>>>>>> Balasubramanian Manoharan(bala-manoharan) wrote:
>>>>>>>>>> I can move the typedef here.
>>>>>>>>>> Not sure if I understand the need to move to uint32_t, I have only
>>>>>>>>>> defined 100% as 10,000 and we should be fine with uin16_t. Any other
>>>>>>>>>> specific reason for using uint32_t?
>>>>>>>>>>> Balasubramanian Manoharan(bala-manoharan) wrote:
>>>>>>>>>>> The drop probability is 100% only when the pool is completely full
>>>>>>>>>>> i.e there is no further buffer to allocate packet. The intention of
>>>>>>>>>>> this description is that when resource usage is greater than
>>>>>>>>>>> threshold.max then the drop probability is enabled and packets will
>>>>>>>>>>> get dropped on an increasing drop probability and when it is less
>>>>>>>>>>> than min threshold then drop probability is disabled.
>>>>>>>>>>>> Petri Savolainen(psavol) wrote:
>>>>>>>>>>>> I think it should be:
>>>>>>>>>>>> * drop probability == 100%, when resource usage > threshold.max
>>>>>>>>>>>> * drop probability == 0%, when resource usage < threshold.min
>>>>>>>>>>>> * drop probability between 0 ... 100%, when resource usage
>>>>>>>>>>>> between min and max
>>>>>>>>>>>>
>>>>>>>>>>>> Now the text describe min/max as hysteresis: > max enables, < min
>>>>>>>>>>>> disables. Which is not the intention, I guess.
>>>>>>>>>>>>> Petri Savolainen(psavol) wrote:
>>>>>>>>>>>>> Another option for enums: ODP_THRESHOLD_PCT, ODP_THRESHOLD_PKT,
>>>>>>>>>>>>> ODP_THRESHOLD_BYTE
>>>>>>>>>>>>>> Petri Savolainen(psavol) wrote:
>>>>>>>>>>>>>> the enum: odp_threshold_type_t
>>>>>>>>>>>>>> the bitfield: odp_threshold_types_t
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> The enum values need to be UPPERCASE and contain a common
>>>>>>>>>>>>>> prefix: ODP_THLD_PERCENT, ODP_THLD_PACKET, ODP_THLD_BYTE
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> If bitfield is only needed in one place (one capability struct)
>>>>>>>>>>>>>> it could be defined there only (no typedef needed).
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Petri Savolainen(psavol) wrote:
>>>>>>>>>>>>>>> Since API specifies already what odp_percent_t is, it's better
>>>>>>>>>>>>>>> do also the typedef here. Also the documentation should say
>>>>>>>>>>>>>>> that it's _unsigned_ integer. May be uint32_t is safer choice
>>>>>>>>>>>>>>> than uint16_t, so that percent calculations do not easily wrap
>>>>>>>>>>>>>>> around.
>>>>>>>>>>>>>>>> Balasubramanian Manoharan(bala-manoharan) wrote:
>>>>>>>>>>>>>>>> Done.
>>>>>>>>>>>>>>>>> Balasubramanian Manoharan(bala-manoharan) wrote:
>>>>>>>>>>>>>>>>> Currently, I have followed the syntax existing in api-next.
>>>>>>>>>>>>>>>>> If ABI changes are merged first I will change my patch to
>>>>>>>>>>>>>>>>> match the specifications. If my patch gets merged first,
>>>>>>>>>>>>>>>>> change this as part of your ABI spec.
>>>>>>>>>>>>>>>>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>>>>>>>>>>>>>>>>> We shouldn't leave fields undefined in the returned
>>>>>>>>>>>>>>>>>> `odp_cls_capability_t` struct. Either set `threshold_red`
>>>>>>>>>>>>>>>>>> and `threshold_bp` explicitly or just clear the struct to
>>>>>>>>>>>>>>>>>> zeros at the start of this routine.
>>>>>>>>>>>>>>>>>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>>>>>>>>>>>>>>>>>> No, `odp_bool_t` exact implementation is not part of the
>>>>>>>>>>>>>>>>>>> spec, as it is part of platform ABI. Percent type is just
>>>>>>>>>>>>>>>>>>> data, so from my point of view it should not be a part of
>>>>>>>>>>>>>>>>>>> ABI, but rather be a part of API spec.
>>>>>>>>>>>>>>>>>>>> Balasubramanian Manoharan(bala-manoharan) wrote:
>>>>>>>>>>>>>>>>>>>> They typedef for odp_bool_t is still under
>>>>>>>>>>>>>>>>>>>> platform/linux-generic directory hence I had this here. I
>>>>>>>>>>>>>>>>>>>> am fine pushing this to spec/std_types.h but is odp_bool_t
>>>>>>>>>>>>>>>>>>>> going to be moved later?
>>>>>>>>>>>>>>>>>>>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>>>>>>>>>>>>>>>>>>>> @bala-manoharan @Bill-Fischofer-Linaro In my opinion,
>>>>>>>>>>>>>>>>>>>>> let's just push it into `odp/api/spec/std_types.h`.
>>>>>>>>>>>>>>>>>>>>>> Balasubramanian Manoharan(bala-manoharan) wrote:
>>>>>>>>>>>>>>>>>>>>>> Oops. Will take care of that in next version.
>>>>>>>>>>>>>>>>>>>>>>> Balasubramanian Manoharan(bala-manoharan) wrote:
>>>>>>>>>>>>>>>>>>>>>>> I felt using a bitfield is more useful in exposing the
>>>>>>>>>>>>>>>>>>>>>>> platform capability whereas using an enum makes more
>>>>>>>>>>>>>>>>>>>>>>> sense during configuration. I am open for suggestions
>>>>>>>>>>>>>>>>>>>>>>> along these lines.
>>>>>>>>>>>>>>>>>>>>>>>> Balasubramanian Manoharan(bala-manoharan) wrote:
>>>>>>>>>>>>>>>>>>>>>>>> Back pressure is to indicate the remote peer that
>>>>>>>>>>>>>>>>>>>>>>>> there is a network congestion on this particular flow.
>>>>>>>>>>>>>>>>>>>>>>>> Whether the remote peer takes respective action is
>>>>>>>>>>>>>>>>>>>>>>>> beyond the scope of this RFC.
>>>>>>>>>>>>>>>>>>>>>>>>> Balasubramanian Manoharan(bala-manoharan) wrote:
>>>>>>>>>>>>>>>>>>>>>>>>> NXP is implementing RED at queue level. Hence I had
>>>>>>>>>>>>>>>>>>>>>>>>> added this based on the feedback from Nikhil.
>>>>>>>>>>>>>>>>>>>>>>>>>> Balasubramanian Manoharan(bala-manoharan) wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>> The previous PR was handling this RED and BP at cos
>>>>>>>>>>>>>>>>>>>>>>>>>> level. There were some concerns from NXP since CoS
>>>>>>>>>>>>>>>>>>>>>>>>>> is a logical entity and application configuration
>>>>>>>>>>>>>>>>>>>>>>>>>> should make sure that multiple CoS with different
>>>>>>>>>>>>>>>>>>>>>>>>>> RED configuration should not direct to the same pool
>>>>>>>>>>>>>>>>>>>>>>>>>> or queue.
>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>> HW implements the RED and BP effectively either at
>>>>>>>>>>>>>>>>>>>>>>>>>> the pool or the queue.
>>>>>>>>>>>>>>>>>>>>>>>>>> Since a pool represents the real bottleneck for
>>>>>>>>>>>>>>>>>>>>>>>>>> resource exhausion and packet drop.
>>>>>>>>>>>>>>>>>>>>>>>>>> We can discuss further in todays Public call.
>>>>>>>>>>>>>>>>>>>>>>>>>>> Balasubramanian Manoharan(bala-manoharan) wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>>> The implementation provides preference whether the
>>>>>>>>>>>>>>>>>>>>>>>>>>> RED and BP are configured either in pool or queue
>>>>>>>>>>>>>>>>>>>>>>>>>>> and provide support for which threshold method is
>>>>>>>>>>>>>>>>>>>>>>>>>>> supported using capability.
>>>>>>>>>>>>>>>>>>>>>>>>>>> I believe this is sufficient from the
>>>>>>>>>>>>>>>>>>>>>>>>>>> implementation point of view.
>>>>>>>>>>>>>>>>>>>>>>>>>>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>>>> Adding new files requires updates to the various
>>>>>>>>>>>>>>>>>>>>>>>>>>>> Makefile.am and related autotools. That's why
>>>>>>>>>>>>>>>>>>>>>>>>>>>> Travis is reporting failures since these new files
>>>>>>>>>>>>>>>>>>>>>>>>>>>> aren't able to be found / used.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Need to clarify under what conditions RED (and
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> BP) apply to pools. If an application calls
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> `odp_packet_alloc()` that should either succeed
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> or fail. "Drop probability" doesn't make any
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> obvious sense here. Tying these concepts to
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> `odp_pktio_t` configurations seems more in
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> keeping with what's needed, but I understand the
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> difficulties that that caused in the previous PR.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Perhaps tying this to a `odp_cos_t` might work?
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> An `odp_cos_t` has an associated queue and pool
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> and these can then be viewed as admission
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> controls to packets being added to a CoS. That
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> would mean that these features would only be
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> applicable if the classifier is being used, but
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> is that a significant drawback since we want to
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> encourage its use anyway?
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Back pressure in this context presumably means
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> that `odp_queue_enq()` calls stall until the
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> back pressure is relieved. Not sure if that's
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> what's really desired for queues in general.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> RED for a queue in doesn't seem to make sense
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> for queues in general. RED is used for
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> admission control, which is why it's tied to
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> PktIOs. If an application calls
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> `odp_queue_enq()` that should either succeed or
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> fail. A "drop probability" doesn't make sense
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> here. We need to clarify that somehow and I'm
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> not sure how that can be done without tying the
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> configuration back to an `odp_pktio_t`.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Same comment as for pools. It would seem these
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> should be individual `odp_support_t`
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> indications.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Wouldn't these be better done as individual
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> `odp_support_t` configurations? That way
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> implementations can indicate not just support
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> but preferences.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> We've defined what we mean by RED as
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> assigning a drop probability but what we
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> mean by "back pressure" seems very fuzzy
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> here. This needs to be more specific if an
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> application is to know how to make use of
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> this.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> We use `_t` suffixes rather than `_e` for
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> `enums` so `odp_threshold_type_t` would be
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> more standard here, but we already have an
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> `odp_threshold_type_t` defined above. The
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> problem seems to be `_t` already implies
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> type, so "type type" is redundant.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Perhaps a better choice is an
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> `odp_threshold_t` has an
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> `odp_threshold_metric_t metric` field that
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> is the enum above. It's not clear how the
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> `odp_threshold_type_t` bits are intended to
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> be used. Capabilities?
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Bill Fischofer(Bill-Fischofer-Linaro)
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Need Doxygen for each struct and union
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Bill Fischofer(Bill-Fischofer-Linaro)
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Doxygen 1.8.13 requires every element to
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> be documented.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Bill Fischofer(Bill-Fischofer-Linaro)
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Again this new type should follow the PR
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> #250 model.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Bill Fischofer(Bill-Fischofer-Linaro)
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Since we've given conceptual approval
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> to PR #250, these changes should be
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> based on that and this definition would
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> be part of abi-default/std_types.h.
https://github.com/Linaro/odp/pull/277#discussion_r151951334
updated_at 2017-11-20 13:07:56
