Balasubramanian Manoharan(bala-manoharan) replied on github web page:
platform/linux-generic/include/odp/api/std_types.h
line 6
@@ -29,6 +29,8 @@ extern "C" {
typedef int odp_bool_t;
+typedef uint16_t odp_percent_t;
+
/**
Comment:
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_r149860030
updated_at 2017-11-09 03:36:30
