Received-SPF: pass (google.com: domain of linux-kernel+bounces-89013-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Precedence: bulk
MIME-Version: 1.0
References: <20240229143432.273b4871@gandalf.local.home>
In-Reply-To: <20240229143432.273b4871@gandalf.local.home>
From: Jamal Hadi Salim <jhs@mojatatu.com>
Date: Fri, 1 Mar 2024 14:24:17 -0500
Message-ID: <CAM0EoMkOgTezVLnN7f1GoXTURQ73LmXjHnBjQBSBRPnv58K-VQ@mail.gmail.com>
Subject: Re: [PATCH] tracing/net_sched: Fix tracepoints that save qdisc_dev()
 as a string
To: Steven Rostedt <rostedt@goodmis.org>
Cc: LKML <linux-kernel@vger.kernel.org>, netdev@vger.kernel.org, 
	Masami Hiramatsu <mhiramat@kernel.org>, Mathieu Desnoyers <mathieu.desnoyers@efficios.com>, 
	Cong Wang <xiyou.wangcong@gmail.com>, vaclav.zindulka@tlapnet.cz, 
	Jiri Pirko <jiri@resnulli.us>, "David S. Miller" <davem@davemloft.net>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Thu, Feb 29, 2024 at 2:32=E2=80=AFPM Steven Rostedt <rostedt@goodmis.org=
> wrote:
>
> From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
>
> I'm updating __assign_str() and will be removing the second parameter. To
> make sure that it does not break anything, I make sure that it matches th=
e
> __string() field, as that is where the string is actually going to be
> saved in. To make sure there's nothing that breaks, I added a WARN_ON() t=
o
> make sure that what was used in __string() is the same that is used in
> __assign_str().
>
> In doing this change, an error was triggered as __assign_str() now expect=
s
> the string passed in to be a char * value. I instead had the following
> warning:
>
> include/trace/events/qdisc.h: In function =E2=80=98trace_event_raw_event_=
qdisc_reset=E2=80=99:
> include/trace/events/qdisc.h:91:35: error: passing argument 1 of 'strcmp'=
 from incompatible pointer type [-Werror=3Dincompatible-pointer-types]
>    91 |                 __assign_str(dev, qdisc_dev(q));
>
> That's because the qdisc_enqueue() and qdisc_reset() pass in qdisc_dev(q)
> to __assign_str() and to __string(). But that function returns a pointer
> to struct net_device and not a string.
>
> It appears that these events are just saving the pointer as a string and
> then reading it as a string as well.
>
> Use qdisc_dev(q)->name to save the device instead.
>
> Fixes: a34dac0b90552 ("net_sched: add tracepoints for qdisc_reset() and q=
disc_destroy()")
> Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>

Should this be targeting the net tree?
Otherwise, LGTM. Just wondering - this worked before because "name"
was the first field?

Reviewed-by: Jamal Hadi Salim <jhs@mojatatu.com>

cheers,
jamal
> ---
>  include/trace/events/qdisc.h | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/include/trace/events/qdisc.h b/include/trace/events/qdisc.h
> index a3995925cb05..1f4258308b96 100644
> --- a/include/trace/events/qdisc.h
> +++ b/include/trace/events/qdisc.h
> @@ -81,14 +81,14 @@ TRACE_EVENT(qdisc_reset,
>         TP_ARGS(q),
>
>         TP_STRUCT__entry(
> -               __string(       dev,            qdisc_dev(q)    )
> -               __string(       kind,           q->ops->id      )
> -               __field(        u32,            parent          )
> -               __field(        u32,            handle          )
> +               __string(       dev,            qdisc_dev(q)->name      )
> +               __string(       kind,           q->ops->id              )
> +               __field(        u32,            parent                  )
> +               __field(        u32,            handle                  )
>         ),
>
>         TP_fast_assign(
> -               __assign_str(dev, qdisc_dev(q));
> +               __assign_str(dev, qdisc_dev(q)->name);
>                 __assign_str(kind, q->ops->id);
>                 __entry->parent =3D q->parent;
>                 __entry->handle =3D q->handle;
> @@ -106,14 +106,14 @@ TRACE_EVENT(qdisc_destroy,
>         TP_ARGS(q),
>
>         TP_STRUCT__entry(
> -               __string(       dev,            qdisc_dev(q)    )
> -               __string(       kind,           q->ops->id      )
> -               __field(        u32,            parent          )
> -               __field(        u32,            handle          )
> +               __string(       dev,            qdisc_dev(q)->name      )
> +               __string(       kind,           q->ops->id              )
> +               __field(        u32,            parent                  )
> +               __field(        u32,            handle                  )
>         ),
>
>         TP_fast_assign(
> -               __assign_str(dev, qdisc_dev(q));
> +               __assign_str(dev, qdisc_dev(q)->name);
>                 __assign_str(kind, q->ops->id);
>                 __entry->parent =3D q->parent;
>                 __entry->handle =3D q->handle;
> --
> 2.43.0
>