| Message ID | 20240229143432.273b4871@gandalf.local.home (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 51270d573a8d9dd5afdc7934de97d66c0e14b5fd |
| Headers | show |
| Series | tracing/net_sched: Fix tracepoints that save qdisc_dev() as a string | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
On Thu, Feb 29, 2024 at 2:32 PM 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 the > __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() to > 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 expects > the string passed in to be a char * value. I instead had the following > warning: > > include/trace/events/qdisc.h: In function ‘trace_event_raw_event_qdisc_reset’: > include/trace/events/qdisc.h:91:35: error: passing argument 1 of 'strcmp' from incompatible pointer type [-Werror=incompatible-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 qdisc_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 = q->parent; > __entry->handle = 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 = q->parent; > __entry->handle = q->handle; > -- > 2.43.0 >
On Fri, 1 Mar 2024 14:24:17 -0500 Jamal Hadi Salim <jhs@mojatatu.com> wrote: > > Fixes: a34dac0b90552 ("net_sched: add tracepoints for qdisc_reset() and qdisc_destroy()") > > Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org> > > Should this be targeting the net tree? I was going to say that I need this for my work, but my work is aimed at the next merge window, but this should go into the kernel now and be marked for stable. So yes, it probably should go through the net tree. Do I need to resubmit it? > Otherwise, LGTM. Just wondering - this worked before because "name" > was the first field? Looks like it. See commit 43a71cd66b9c0 ("net-device: reorganize net_device fast path variables") I wonder if there's anything else that uses a pointer to struct net_device thinking it can just be switched to find the name? > > Reviewed-by: Jamal Hadi Salim <jhs@mojatatu.com> Thanks, -- Steve
On Fri, Mar 1, 2024 at 2:59 PM Steven Rostedt <rostedt@goodmis.org> wrote: > > On Fri, 1 Mar 2024 14:24:17 -0500 > Jamal Hadi Salim <jhs@mojatatu.com> wrote: > > > > Fixes: a34dac0b90552 ("net_sched: add tracepoints for qdisc_reset() and qdisc_destroy()") > > > Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org> > > > > Should this be targeting the net tree? > > I was going to say that I need this for my work, but my work is aimed at > the next merge window, but this should go into the kernel now and be marked > for stable. So yes, it probably should go through the net tree. > > Do I need to resubmit it? > My view is it needs to be merged. I note there are some big changes in net and net-next trees right now that move the name - probably from 43a71cd66b9c0 Coco Li and Eric are on your Cc already. > > Otherwise, LGTM. Just wondering - this worked before because "name" > > was the first field? > > Looks like it. See commit 43a71cd66b9c0 ("net-device: reorganize net_device > fast path variables") > > I wonder if there's anything else that uses a pointer to struct net_device > thinking it can just be switched to find the name? > From a quick scan i cant find anything obvious. cheers, jamal > > > > Reviewed-by: Jamal Hadi Salim <jhs@mojatatu.com> > > Thanks, > > -- Steve
Hello: This patch was applied to netdev/net.git (main) by David S. Miller <davem@davemloft.net>: On Thu, 29 Feb 2024 14:34:44 -0500 you 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 the > __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() to > make sure that what was used in __string() is the same that is used in > __assign_str(). > > [...] Here is the summary with links: - tracing/net_sched: Fix tracepoints that save qdisc_dev() as a string https://git.kernel.org/netdev/net/c/51270d573a8d You are awesome, thank you!
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 = q->parent; __entry->handle = 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 = q->parent; __entry->handle = q->handle;