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.
+ */
+
Comment:
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_r151949517
updated_at 2017-11-20 13:07:56
