Re: [PATCH v5 2/2] ethdev: support xstats reset telemetry command

[fengchengwen <fengchengwen@huawei.com>]

Hi Thomas and Morten,

On 2023/7/3 21:44, Morten Brørup wrote:
>> From: Thomas Monjalon [mailto:thomas@monjalon.net]
>> Sent: Monday, 3 July 2023 09.20
>>
>> 03/07/2023 05:58, fengchengwen:
>>>
>>> On 2023/2/20 21:05, Thomas Monjalon wrote:
>>>> 17/02/2023 10:44, fengchengwen:
>>>>> On 2023/2/16 20:54, Bruce Richardson wrote:
>>>>>> On Thu, Feb 16, 2023 at 08:42:34PM +0800, fengchengwen wrote:
>>>>>>> On 2023/2/16 20:06, Ferruh Yigit wrote:
>>>>>>>> On 2/16/2023 11:53 AM, fengchengwen wrote:
>>>>>>>>> On 2023/2/15 11:19, Dongdong Liu wrote:
>>>>>>>>>> Hi Chengwen
>>>>>>>>>>
>>>>>>>>>> On 2023/2/9 10:32, Chengwen Feng wrote:
>>>>>>>>>>> The xstats reset is useful for debugging, so add it to the
>> ethdev
>>>>>>>>>>> telemetry command lists.
>>>>>>>>>>>
>>>>>>>>>>> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
>>>>>>>>>> This patch looks good, so
>>>>>>>>>> Reviewed-by: Dongdong Liu <liudongdong3@huawei.com>
>>>>>>>>>>
>>>>>>>>>> A minior question
>>>>>>>>>> Do we need to support stats reset ?
>>>>>>>>>
>>>>>>>>> Stats is contained by xstats, and future direction I think is
>> xstats.
>>>>>>>>> So I think we don't need support stats reset.
>>>>>>>>>
>>>>>>>>
>>>>>>>> I have similar question with Dongdong, readonly values are safe
>> for
>>>>>>>> telemetry, but modifying data can be more tricky since we don't
>> have
>>>>>>>> locking in ethdev APIs, this can cause concurrency issues.
>>>>>>>
>>>>>>> Yes, it indeed has concurrency issues.
>>>>>>>
>>>>>>>>
>>>>>>>> Overall do we want telemetry go that way and become something
>> that
>>>>>>>> alters ethdev data/config?
>>>>>>>
>>>>>>> There are at least two part of data: config and status.
>>>>>>> For stats (which belong status data) could help for debugging, I
>> think it's acceptable.
>>>>>>>
>>>>>>> As for concurrency issues. People should know what to do and when
>> to do, just like
>>>>>>> the don't invoke config API (e.g. dev_configure/dev_start/...)
>> concurrency.
>>>>>>>
>>>>>> While this is probably ok for now, I think in next release we
>> should look
>>>>>> to add some sort of support for locking for destructive ops in a
>> future
>>>>>> release. For example, we could:
>>>>>>
>>>>>> 1. Add support for marking a callback as "destructive" and only
>> allow it to
>>>>>> be called if only one connection is present or
>>>>>>
>>>>>> 2. Make it possible for callbacks to query the number of
>> connections so
>>>>>> that the callback itself is non-destructive in more than one
>> connection is
>>>>>> open.
>>>>>>
>>>>>> [Both of these will require locking support so that new
>> connections aren't
>>>>>> openned when the callback is in-flight!]
>>>>>
>>>>> Except telemetry, the application may have other console could
>> execute DPDK API.
>>>>> So I think trying to keep it simple, it's up to the user to invoke.
>>>>
>>>> No, the user should not be responsible for concurrency issues.
>>>> We can ask the app developper to take care,
>>>> but not to the user who has no control on what happens in the app.
>>>>
>>>> On a more general note, I feel the expansion of telemetry is not
>> controlled enough.
>>>> I would like to stop on adding more telemetry until we have a clear
>> guideline
>>>> about what is telemetry for and how to use it.
>>>
>>> Hi Thomas,
>>>
>>> Should this be discussed on TB?
>>
>> What would be your question exactly?
> 
> A general comment about telemetry:
> 
> If an application exposes telemetry through an end user facing API, e.g. http(s) REST, it would be nice if non-read-only telemetry paths are easy to identify by following some DPDK standard convention, so the application does not need to manually maintain an allow-list of read-only paths.

+1 for this point.

> 
> Bruce's documentation about trace/log/telemetry/dump might also need to be updated regarding non-read-only telemetry actions.

I just check Bruce's patch [1], and notice that the telemetry callback must be 'read-only': (Telemetry callbacks should not modify any program state, but be "read-only").

From internal product usage, we think xstats-reset is valid to identify problem, but this callback is not read-only.

We think telemetry callback should not limit to 'read-only'. Perhaps we could develop some strategy to better manage non-read-only callbacks (just like Morten's advise).

[1]: https://patchwork.dpdk.org/project/dpdk/patch/20230620170728.74117-3-bruce.richardson@intel.com/

> 
> 
> .
>