Petri Savolainen(psavol) replied on github web page:
include/odp/api/spec/classification.h
@@ -107,6 +108,61 @@ 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 cross the specified
+ * threshold values. RED is enabled when 'red_enable' boolean is true and
+ * the resource usage is equal to or greater than the minimum threshold value.
+ * Resource usage could be defined as the percentage of pool being full or the
+ * number of packets/bytes occupied in the queue depening on the platform
+ * capabilities.
+ * 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.
+ *
+ * Drop probability is configured as follows
+ * * 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
+ *
+ * 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.
+ */
+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 minimum threshold value and is disabled otherwise
+ */
+ odp_threshold_t red_threshold;
+} odp_red_param_t;
+
+/** Back pressure (BP)
+ * When back pressure is enabled for a particular flow, the HW can send
+ * back pressure information to the remote peer indicating a network
congestion.
+ */
+
+typedef struct odp_bp_param_t {
+ /** A boolean to enable Back pressure
+ * When true, back pressure is enabled and configured with the BP
+ * parameters. Otherwise BP parameters are ignored.
+ */
+ odp_bool_t bp_enable;
+
+ /** Threshold value for back pressure.
+ * BP is enabled when queue or pool value is equal to or greater
+ * than the max backpressure threshold.
+ * Min threshold parameters are ignored for BP configuration.
+ */
+ odp_threshold_t bp_threshold;
+} odp_bp_param_t;
Comment:
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_r151950381
updated_at 2017-11-20 13:07:56
