Message-ID: <20240223134653.524a5c9e@gandalf.local.home>
Date: Fri, 23 Feb 2024 13:46:53 -0500
From: Steven Rostedt <rostedt@...dmis.org>
To: Jeff Johnson <quic_jjohnson@...cinc.com>
Cc: LKML <linux-kernel@...r.kernel.org>, Linux Trace Kernel
 <linux-trace-kernel@...r.kernel.org>, Masami Hiramatsu
 <mhiramat@...nel.org>, Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
 Linus Torvalds <torvalds@...ux-foundation.org>,
 <linuxppc-dev@...ts.ozlabs.org>, <kvm@...r.kernel.org>,
 <linux-block@...r.kernel.org>, <linux-cxl@...r.kernel.org>,
 <linux-media@...r.kernel.org>, <dri-devel@...ts.freedesktop.org>,
 <amd-gfx@...ts.freedesktop.org>, <intel-gfx@...ts.freedesktop.org>,
 <intel-xe@...ts.freedesktop.org>, <linux-arm-msm@...r.kernel.org>,
 <freedreno@...ts.freedesktop.org>, <virtualization@...ts.linux.dev>,
 <linux-rdma@...r.kernel.org>, <linux-pm@...r.kernel.org>,
 <iommu@...ts.linux.dev>, <linux-tegra@...r.kernel.org>,
 <netdev@...r.kernel.org>, <linux-hyperv@...r.kernel.org>,
 <ath10k@...ts.infradead.org>, <linux-wireless@...r.kernel.org>,
 <ath11k@...ts.infradead.org>, <ath12k@...ts.infradead.org>,
 <brcm80211@...ts.linux.dev>, <brcm80211-dev-list.pdl@...adcom.com>,
 <linux-usb@...r.kernel.org>, <linux-bcachefs@...r.kernel.org>,
 <linux-nfs@...r.kernel.org>, <ocfs2-devel@...ts.linux.dev>,
 <linux-cifs@...r.kernel.org>, <linux-xfs@...r.kernel.org>,
 <linux-edac@...r.kernel.org>, <selinux@...r.kernel.org>,
 <linux-btrfs@...r.kernel.org>, <linux-erofs@...ts.ozlabs.org>,
 <linux-f2fs-devel@...ts.sourceforge.net>, <linux-hwmon@...r.kernel.org>,
 <io-uring@...r.kernel.org>, <linux-sound@...r.kernel.org>,
 <bpf@...r.kernel.org>, <linux-wpan@...r.kernel.org>, <dev@...nvswitch.org>,
 <linux-s390@...r.kernel.org>, <tipc-discussion@...ts.sourceforge.net>,
 Julia Lawall <Julia.Lawall@...ia.fr>
Subject: Re: [FYI][PATCH] tracing/treewide: Remove second parameter of
 __assign_str()

On Fri, 23 Feb 2024 10:30:45 -0800
Jeff Johnson <quic_jjohnson@...cinc.com> wrote:

> On 2/23/2024 9:56 AM, Steven Rostedt wrote:
> > From: "Steven Rostedt (Google)" <rostedt@...dmis.org>
> > 
> > [
> >    This is a treewide change. I will likely re-create this patch again in
> >    the second week of the merge window of v6.9 and submit it then. Hoping
> >    to keep the conflicts that it will cause to a minimum.
> > ]
> > 
> > With the rework of how the __string() handles dynamic strings where it
> > saves off the source string in field in the helper structure[1], the
> > assignment of that value to the trace event field is stored in the helper
> > value and does not need to be passed in again.  
> 
> Just curious if this could be done piecemeal by first changing the
> macros to be variadic macros which allows you to ignore the extra
> argument. The callers could then be modified in their separate trees.
> And then once all the callers have be merged, the macros could be
> changed to no longer be variadic.

I weighed doing that, but I think ripping off the band-aid is a better
approach. One thing I found is that leaving unused parameters in the macros
can cause bugs itself. I found one case doing my clean up, where an unused
parameter in one of the macros was bogus, and when I made it a used
parameter, it broke the build.

I think for tree-wide changes, the preferred approach is to do one big
patch at once. And since this only affects TRACE_EVENT() macros, it
hopefully would not be too much of a burden (although out of tree users may
suffer from this, but do we care?)

Now one thing I could do is to not remove the parameter, but just add:

	WARN_ON_ONCE((src) != __data_offsets->item##_ptr_);

in the __assign_str() macro to make sure that it's still the same that is
assigned. But I'm not sure how useful that is, and still causes burden to
have it. I never really liked the passing of the string in two places to
begin with.

-- Steve

Powered by blists - more mailing lists