Petri Savolainen(psavol) replied on github web page:
include/odp/api/spec/classification.h
@@ -107,6 +108,55 @@ typedef union odp_cls_pmr_terms_t {
uint64_t all_bits;
} odp_cls_pmr_terms_t;
+/** Random Early Detection (RED)
+ * Random Early Detection is enabled to initiate a drop probability
+ * for the incoming packet when the packets in the queue/pool reaches
+ * a specified threshold.
+ * When RED is enabled for a particular flow then further incoming
+ * packets are assigned a drop probability based on the size of the
+ * pool/queue and the drop probability becomes 100% when the queue/pool
+ * is full.
+ * RED is logically configured in the CoS and could be implemented
+ * in either pool or queue linked to the CoS depending on
+ * platform capabilities. Application should make sure not to link
+ * multiple CoS with different RED or BP configuration to the same queue
+ * or pool.
+ * RED is enabled when the resource limit is equal to or greater than
+ * the maximum threshold value and is disabled when resource limit
+ * is less than or equal to minimum threshold value. */
+typedef struct odp_red_param_t {
+ /** A boolean to enable RED
+ * When true, RED is enabled and configured with RED parameters.
+ * Otherwise, RED parameters are ignored. */
+ odp_bool_t red_enable;
+
+ /** Threshold parameters for RED
+ * RED is enabled when the resource limit is equal to or greater than
+ * the maximum threshold value and is disabled when resource limit
+ * is less than or equal to minimum threshold value. */
+ odp_threshold_t red_threshold;
Comment:
"... 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_r151949438
updated_at 2017-11-20 13:07:56
