Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:7 as permitted sender) client-ip=2620:137:e000::3:7;
MIME-Version: 1.0
References: <20231103131011.1316396-1-lb@semihalf.com> <20231103131011.1316396-12-lb@semihalf.com>
In-Reply-To: <20231103131011.1316396-12-lb@semihalf.com>
From:   jim.cromie@gmail.com
Date:   Sat, 4 Nov 2023 15:48:55 -0600
Message-ID: <CAJfuBxzhz7pBYkfqfPomH4PUzqLPX1nxsev4yrQ2P6m5hyMT+Q@mail.gmail.com>
Subject: Re: [PATCH v1 11/12] dyndbg: write debug logs to trace instance
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>,
        Steven Rostedt <rostedt@goodmis.org>,
        Vincent Whitchurch <vincent.whitchurch@axis.com>,
        Pekka Paalanen <ppaalanen@gmail.com>,
        Sean Paul <seanpaul@chromium.org>,
        Daniel Vetter <daniel@ffwll.ch>, linux-kernel@vger.kernel.org,
        upstream@semihalf.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Fri, Nov 3, 2023 at 7:10=E2=80=AFAM =C5=81ukasz Bartosik <lb@semihalf.co=
m> wrote:
>
> When trace is enabled (T flag is set) and trace_dst field is set
> to value greater than 0 (0 is reserved for trace events) then
> debug logs will be written to trace instance pointed by trace_dst
> value, for example when trace_dst value is 2 then debug logs will
> be written to <debugfs>/tracing/instances/dyndbg_inst_2 instance.
> Given trace instance will not be initialized until debug logs are
> requested to be written to it and afer init will persist until
> reboot.
>

restating 00 comments -

you can get rid of integer destination ids by adding a new command: open/cl=
ose

$> echo  \
 open kms-instance \;\
 class DRM_UT_KMS -T:kms-instance  # preset-dests-disable-sites \;\
> /proc/dynamic_debug/control

echo "class DRM_UT_KMS +T # enable sites, with the preselected data" >
/proc/dynamic_debug/control

open - assign name to id, reserve it, hide it from user
+T:valid-name # fail command if name wasnt opened

and +T  w/o dest means use existing setting, not just 0 (unless thats
the existing setting)

> Signed-off-by: =C5=81ukasz Bartosik <lb@semihalf.com>
> ---
>  lib/Kconfig.debug   |  1 +
>  lib/dynamic_debug.c | 79 ++++++++++++++++++++++++++++++++++++++++++---
>  2 files changed, 76 insertions(+), 4 deletions(-)
>
> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> index fa307f93fa2e..9617e92c046d 100644
> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> @@ -181,6 +181,7 @@ config DYNAMIC_DEBUG_CORE
>         bool "Enable core function of dynamic debug support"
>         depends on PRINTK
>         depends on (DEBUG_FS || PROC_FS)
> +       depends on TRACING
>         help
>           Enable core functional support of dynamic debug. It is useful
>           when you want to tie dynamic debug to your kernel modules with
> diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
> index c5cd28e74a02..541d9d522b3b 100644
> --- a/lib/dynamic_debug.c
> +++ b/lib/dynamic_debug.c
> @@ -36,6 +36,7 @@
>  #include <linux/sched.h>
>  #include <linux/device.h>
>  #include <linux/netdevice.h>
> +#include <linux/trace.h>
>
>  #define CREATE_TRACE_POINTS
>  #include <trace/events/dyndbg.h>
> @@ -81,6 +82,18 @@ module_param(verbose, int, 0644);
>  MODULE_PARM_DESC(verbose, " dynamic_debug/control processing "
>                  "( 0 =3D off (default), 1 =3D module add/rm, 2 =3D >cont=
rol summary, 3 =3D parsing, 4 =3D per-site changes)");
>
> +/*
> + * When trace is enabled (T flag is set) and trace_dst field is set
> + * to value greater than 0 (0 is reserved for trace events) then
> + * debug logs will be written to trace instance pointed by trace_dst
> + * value, for example when trace_dst value is 2 then debug logs will
> + * be written to <debugfs>/tracing/instances/dyndbg_inst_2 instance.
> + * Given trace instance will not be initialized until debug logs are
> + * requested to be written to it and afer init will persist until
> + * reboot.
> + */
> +static struct trace_array *trace_arr[TRACE_DST_MAX];
> +
>  static inline struct dd_ctrl *get_ctrl(struct _ddebug *desc)
>  {
>         return &desc->ctrl;
> @@ -255,6 +268,45 @@ static struct ddebug_class_map *ddebug_find_valid_cl=
ass(struct ddebug_table cons
>         return NULL;
>  }
>
> +static int handle_trace_dst(struct dd_ctrl *ctrl)
> +{
> +#define TRACE_INST_NAME_LEN 16
> +       char instance_name[TRACE_INST_NAME_LEN];
> +       struct trace_array *arr;
> +       int ret =3D -EINVAL;
> +
> +       /* check if trace (T flag) is enabled */
> +       if (!(ctrl->flags & _DPRINTK_FLAGS_TRACE))
> +               return 0;
> +
> +       /* check if trace destination are trace events */
> +       if (!ctrl->trace_dst)
> +               return 0;
> +
> +       /* check if trace instance is already set up */
> +       if (trace_arr[ctrl->trace_dst])
> +               return 0;
> +
> +       snprintf(instance_name, TRACE_INST_NAME_LEN,
> +                "dyndbg_inst_%u", ctrl->trace_dst);
> +       arr =3D trace_array_get_by_name(instance_name);
> +       if (!arr)
> +               goto err;
> +
> +       ret =3D trace_array_init_printk(arr);
> +       if (ret)
> +               goto err_init;
> +
> +       trace_arr[ctrl->trace_dst] =3D arr;
> +       return 0;
> +
> +err_init:
> +       trace_array_put(arr);
> +       trace_array_destroy(arr);
> +err:
> +       return ret;
> +}
> +
>  #define __outvar /* filled by callee */
>  /*
>   * Search the tables for _ddebug's which match the given `query' and
> @@ -338,6 +390,9 @@ static int ddebug_change(const struct ddebug_query *q=
uery,
>                         nctrl.trace_dst =3D modifiers->trace_dst;
>                         if (!memcmp(&nctrl, get_ctrl(dp), sizeof(nctrl)))
>                                 continue;
> +
> +                       if (handle_trace_dst(&nctrl))
> +                               continue;
>  #ifdef CONFIG_JUMP_LABEL
>                         if (get_flags(dp) & _DPRINTK_FLAGS_ENABLED) {
>                                 if (!(nctrl.flags & _DPRINTK_FLAGS_ENABLE=
D))
> @@ -977,8 +1032,8 @@ static DEFINE_PER_CPU(struct ddebug_trace_bufs, ddeb=
ug_trace_bufs);
>  static DEFINE_PER_CPU(int, ddebug_trace_reserve);
>
>  __printf(3, 0)
> -static void ddebug_trace(struct _ddebug *desc, const struct device *dev,
> -                        const char *fmt, va_list args)
> +static void ddebug_trace_event(struct _ddebug *desc, const struct device=
 *dev,
> +                              const char *fmt, va_list args)
>  {
>         struct ddebug_trace_buf *buf;
>         int bufidx;
> @@ -1010,6 +1065,15 @@ static void ddebug_trace(struct _ddebug *desc, con=
st struct device *dev,
>         preempt_enable_notrace();
>  }
>
> +__printf(2, 0)
> +static void ddebug_trace_instance(struct _ddebug *desc, const char *fmt,
> +                                 va_list *args)
> +{
> +       struct va_format vaf =3D { .fmt =3D fmt, .va =3D args};
> +
> +       trace_array_printk(trace_arr[get_trace_dst(desc)], _THIS_IP_, "%p=
V", &vaf);
> +}
> +
>  __printf(2, 3)
>  static void ddebug_printk(struct _ddebug *desc, const char *fmt, ...)
>  {
> @@ -1022,7 +1086,11 @@ static void ddebug_printk(struct _ddebug *desc, co=
nst char *fmt, ...)
>                  * All callers include the KERN_DEBUG prefix to keep the
>                  * vprintk case simple; strip it out for tracing.
>                  */
> -               ddebug_trace(desc, NULL, fmt + strlen(KERN_DEBUG), args);
> +               if (get_trace_dst(desc))
> +                       ddebug_trace_instance(desc, fmt, &args);
> +               else
> +                       ddebug_trace_event(desc, NULL,
> +                                          fmt + strlen(KERN_DEBUG), args=
);
>                 va_end(args);
>         }
>
> @@ -1044,7 +1112,10 @@ static void ddebug_dev_printk(struct _ddebug *desc=
, const struct device *dev,
>                 va_list args;
>
>                 va_start(args, fmt);
> -               ddebug_trace(desc, dev, fmt, args);
> +               if (get_trace_dst(desc))
> +                       ddebug_trace_instance(desc, fmt, &args);
> +               else
> +                       ddebug_trace_event(desc, dev, fmt, args);
>                 va_end(args);
>         }
>
> --
> 2.42.0.869.gea05f2083d-goog
>