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

[fengchengwen <fengchengwen@huawei.com>]

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.

> 
> Any other thoughts or suggestions?
> 
> /Bruce
> .
>