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

["Morten Brørup" <mb@smartsharesystems.com>]

> 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.

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