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

[Thomas Monjalon <thomas@monjalon.net>]

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?