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_ 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_r151948968
updated_at 2017-11-20 13:07:56
