Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:6 as permitted sender) client-ip=2620:137:e000::3:6;
MIME-Version: 1.0
References: <20230915154856.1896062-1-lb@semihalf.com> <CAJfuBxyFYyGCtr5i=P7N=1oX3J=jmdp1VLGLt+z1fAnuvGK2aA@mail.gmail.com>
 <CAK8ByeJBrPEQSgUc91LQO9Krzjh2pauhMTjEC82M8ozayE76Yg@mail.gmail.com>
In-Reply-To: <CAK8ByeJBrPEQSgUc91LQO9Krzjh2pauhMTjEC82M8ozayE76Yg@mail.gmail.com>
From:   jim.cromie@gmail.com
Date:   Mon, 2 Oct 2023 14:49:20 -0600
Message-ID: <CAJfuBxxmL-GtBgt=033F9UNeLCreFbJh3HrQQN2nYKwR_0uTbg@mail.gmail.com>
Subject: Re: [PATCH v1] dynamic_debug: add support for logs destination
To:     =?UTF-8?Q?=C5=81ukasz_Bartosik?= <lb@semihalf.com>
Cc:     Jason Baron <jbaron@akamai.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        Kees Cook <keescook@chromium.org>,
        Douglas Anderson <dianders@chromium.org>,
        Guenter Roeck <groeck@google.com>,
        Yaniv Tzoreff <yanivt@google.com>,
        Benson Leung <bleung@google.com>, linux-kernel@vger.kernel.org,
        upstream@semihalf.com,
        Vincent Whitchurch <vincent.whitchurch@axis.com>,
        Steven Rostedt <rostedt@goodmis.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Mon, Sep 18, 2023 at 1:20=E2=80=AFAM =C5=81ukasz Bartosik <lb@semihalf.c=
om> wrote:
>
> pt., 15 wrz 2023 o 20:02 <jim.cromie@gmail.com> napisa=C5=82(a):
> >
> > On Fri, Sep 15, 2023 at 9:49=E2=80=AFAM =C5=81ukasz Bartosik <lb@semiha=
lf.com> wrote:
> > >

hi Lukasz,

sorry my kernel-time has been in my own trees.

What I dont understand is why +T is insufficient.

IIUC, tracefs is intended for production use.
thats why each event can be enabled / disabled
- to select and minimize whats traced, and not impact the system

and +T  can forward all pr_debugs to trace,
(by 1-few trace events defined similarly to others)
or very few, giving yet another selection mechanism
to choose or eliminate specific pr-debugs and reduce traffic to
interesting stuff.

Once your debug is in the trace-buf,
shouldnt user-space be deciding what to do with it ?
a smart daemon could leverage tracefs to good effect.

IMO the main value of +T is that it allows feeding existing pr_debugs
into the place where other trace-data is already integrated and managed.

At this point, I dont see any extra destination handling as prudent.

Jim


> > > Add support for selection of dynamic debug logs destination.
> > > When enabled it allows to configure destination of each callsite
> > > individually. The option adds a framework (described by struct
> > > ddebug_dst_ops) which provides flexible means to add a new destinatio=
n
> > > for debug logs. On top of the framework support for trace instance as
> > > destination is added. By default destination of debug logs is syslog.
> > > Syslog can be set explicitly by using dst keyword or is selected by
> > > default when dst keyword is omitted, for example:
> >
> > A while back,
> > Cc: Vincent Whitchurch <vincent.whitchurch@axis.com>
> > proposed these patches:
> >       https://lore.kernel.org/lkml/20200825153338.17061-1-vincent.whitc=
hurch@axis.com/
> >
> > his approach adds a single flag, +x or (my pref) +T
> > which sends the pr_debug to tracefs, independent of +p.
> > Steve Rostedt offered feedback on one of the versions,
> > and one could read that as provisional acceptance.
> >
> > Ive worked his patchset since, it is here:
> > https://github.com/jimc/linux/tree/dd-trc-11
> > (it has some unrelated bits too)
> >
> > but it sits atop another patchset:
> > https://lore.kernel.org/lkml/20230911230838.14461-1-jim.cromie@gmail.co=
m/
> > or for git remote add
> > https://github.com/jimc/linux/tree/dd-fix-5i
> >
>
> I looked through the patches you pointed me to. The solution with
> +T/+x is elegant and tailored to specific needs.
> Our rationale to write pr_debug/dev_dbg logs to tracefs is similar to
> Vincent's to aid us in debugging.
> Unfortunately the solution with +T/+x does not cover our use case.
>
> Our use case is different. We experience issues reported by Chromebook
> users which are difficult to reproduce
> that's why we would like to enable debug logs to be written to tracefs
> on productions systems.
> When users experiences an issues and sends us a feedback report the
> debug logs written to tracefs would be
> attached to the feedback report. We would like to write debug logs to
> a separate trace instances based on
> the subsystem/driver. Our initial areas of interest/issues are usbcore
> and thunderbolt drivers/subsystems.
> More may come in the future.
>
> With my proposal this could be achieved for the thunderbolt subsystem
> with the following steps:
> * enable kernel configuration option CONFIG_DYNAMIC_DEBUG_CORE,
> * enable kernel configuration option DYNAMIC_DEBUG_DST and set its
>   operation mode to Static.
> * add the option to the thunderbolt's drivers/thunderbolt/Makefile
> in order to compile in debug logs:
>         CFLAGS_nhi.o :=3D -DDYNAMIC_DEBUG_MODULE
> * compile the kernel and update it on a target device,
> * finally append the entry to the kernel boot command line:
>         thunderbolt.dyndbg=3D"dst trace:thunderbolt =3Dp"
>
> Both the solutions (I mean +x/+T and mine proposal) have pros and cons.
> Maybe both could coexist together, +x/+T by default and this patch as
> configurable option
> which adds more flexibility ?
>
> >
> > syslog and/or tracefs seems sufficient, do you have a 3rd destination ?
> >
>
> I don't have a use case for the third destination, but for example
> adding socket destination to my proposal would be relatively fast and
> easy (that would be for limited use only after a network is up and
> running).
>
> Thanks,
> Lukasz
>
> > thanks