Petri Savolainen(psavol) replied on github web page:
include/odp/api/spec/threshold.h
@@ -0,0 +1,105 @@
+/* Copyright (c) 2017, Linaro Limited
+ * All rights reserved.
+ *
+ * SPDX-License-Identifier: BSD-3-Clause
+ */
+
+/**
+ * @file
+ *
+ * ODP threshold descriptor
+ */
+
+#ifndef ODP_API_THRESHOLD_H_
+#define ODP_API_THRESHOLD_H_
+#include <odp/visibility_begin.h>
+#include <odp/api/std_types.h>
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+/**
+ * Supported threshold types
+ *
+ * Supported threshold types in a bit field structure.
+ */
+typedef union odp_threshold_types_t {
+ /** bitfields for different threshold types */
+ struct {
+ /** Percentage of the total size of pool or queue */
+ uint8_t percent:1;
+
+ /** Total number of all transient packets */
+ uint8_t pkt_cnt:1;
+
+ /** Total size of all transient packets in bytes */
+ uint8_t pkt_size:1;
+ };
+
+ /** All bits of the bit field structure */
+ uint8_t all_bits;
+} odp_threshold_types_t;
+
+/**
+ * ODP Threshold types
+ *
+ * Different types of threshold measurements
+ */
+typedef enum {
+ /** Percentage of the total size of pool or queue */
+ ODP_THRESHOLD_PERCENT,
+
+ /** Total number of all transient packets */
+ ODP_THRESHOLD_PKT,
Comment:
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_r151123090
updated_at 2017-11-15 13:56:00
