From: Steven Rostedt <rostedt@goodmis.org>
To: Alison Schofield <alison.schofield@intel.com>
Cc: LKML <linux-kernel@vger.kernel.org>,
Linux Trace Kernel <linux-trace-kernel@vger.kernel.org>,
Masami Hiramatsu <mhiramat@kernel.org>,
Mathieu Desnoyers <mathieu.desnoyers@efficios.com>,
Linus Torvalds <torvalds@linux-foundation.org>,
linuxppc-dev@lists.ozlabs.org, kvm@vger.kernel.org,
linux-block@vger.kernel.org, linux-cxl@vger.kernel.org,
linux-media@vger.kernel.org, dri-devel@lists.freedesktop.org,
amd-gfx@lists.freedesktop.org, intel-gfx@lists.freedesktop.org,
intel-xe@lists.freedesktop.org, linux-arm-msm@vger.kernel.org,
freedreno@lists.freedesktop.org, virtualization@lists.linux.dev,
linux-rdma@vger.kernel.org, linux-pm@vger.kernel.org,
iommu@lists.linux.dev, linux-tegra@vger.kernel.org,
netdev@vger.kernel.org, linux-hyperv@vger.kernel.org,
ath10k@lists.infradead.org, linux-wireless@vger.kernel.org,
ath11k@lists.infradead.org, ath12k@lists.infradead.org,
brcm80211@lists.linux.dev, brcm80211-dev-list.pdl@broadcom.com,
linux-usb@vger.kernel.org, linux-bcachefs@vger.kernel.org,
linux-nfs@vger.kernel.org, ocfs2-devel@lists.linux.dev,
linux-cifs@vger.kernel.org, linux-xfs@vger.kernel.org,
linux-edac@vger.kernel.org, selinux@vger.kernel.org,
linux-btrfs@vger.kernel.org, linux-erofs@lists.ozlabs.org,
linux-f2fs-devel@lists.sourceforge.net,
linux-hwmon@vger.kernel.org, io-uring@vger.kernel.org,
linux-sound@vger.kernel.org, bpf@vger.kernel.org,
linux-wpan@vger.kernel.org, dev@openvswitch.org,
linux-s390@vger.kernel.org,
tipc-discussion@lists.sourceforge.net,
Julia Lawall <Julia.Lawall@inria.fr>
Subject: Re: [FYI][PATCH] tracing/treewide: Remove second parameter of __assign_str()
Date: Thu, 14 Mar 2024 14:34:06 -0400 [thread overview]
Message-ID: <20240314143406.6289a060@gandalf.local.home> (raw)
In-Reply-To: <ZfMslbCmCtyEaEWN@aschofie-mobl2>
On Thu, 14 Mar 2024 09:57:57 -0700
Alison Schofield <alison.schofield@intel.com> wrote:
> On Fri, Feb 23, 2024 at 12:56:34PM -0500, Steven Rostedt wrote:
> > From: "Steven Rostedt (Google)" <rostedt@goodmis.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.
> > ]
Note, change of plans. I plan on sending this in the next merge window, as
this merge window I have this patch:
https://lore.kernel.org/linux-trace-kernel/20240312113002.00031668@gandalf.local.home/
That will warn if the source string of __string() is different than the
source string of __assign_str(). I want to make sure they are identical
before just dropping one of them.
>
> > diff --git a/drivers/cxl/core/trace.h b/drivers/cxl/core/trace.h
> > index bdf117a33744..07ba4e033347 100644
> > --- a/drivers/cxl/core/trace.h
> > +++ b/drivers/cxl/core/trace.h
>
> snip to poison
>
> > @@ -668,8 +668,8 @@ TRACE_EVENT(cxl_poison,
> > ),
> >
> > TP_fast_assign(
> > - __assign_str(memdev, dev_name(&cxlmd->dev));
> > - __assign_str(host, dev_name(cxlmd->dev.parent));
> > + __assign_str(memdev);
> > + __assign_str(host);
>
> I think I get that the above changes work because the TP_STRUCT__entry for
> these did:
> __string(memdev, dev_name(&cxlmd->dev))
> __string(host, dev_name(cxlmd->dev.parent))
That's the point. They have to be identical or you will likely bug.
The __string(name, src) is used to find the string length of src which
allocates the necessary length on the ring buffer. The __assign_str(name, src)
will copy src into the ring buffer.
Similar to:
len = strlen(src);
buf = malloc(len);
strcpy(buf, str);
Where __string() is strlen() and __assign_str() is strcpy(). It doesn't
make sense to use two different strings, and if you did, it would likely be
a bug.
But the magic behind __string() does much more than just get the length of
the string, and it could easily save the pointer to the string (along with
its length) and have it copy that in the __assign_str() call, making the
src parameter of __assign_str() useless.
>
> > __entry->serial = cxlmd->cxlds->serial;
> > __entry->overflow_ts = cxl_poison_overflow(flags, overflow_ts);
> > __entry->dpa = cxl_poison_record_dpa(record);
> > @@ -678,12 +678,12 @@ TRACE_EVENT(cxl_poison,
> > __entry->trace_type = trace_type;
> > __entry->flags = flags;
> > if (region) {
> > - __assign_str(region, dev_name(®ion->dev));
> > + __assign_str(region);
> > memcpy(__entry->uuid, ®ion->params.uuid, 16);
> > __entry->hpa = cxl_trace_hpa(region, cxlmd,
> > __entry->dpa);
> > } else {
> > - __assign_str(region, "");
> > + __assign_str(region);
> > memset(__entry->uuid, 0, 16);
> > __entry->hpa = ULLONG_MAX;
>
> For the above 2, there was no helper in TP_STRUCT__entry. A recently
> posted patch is fixing that up to be __string(region, NULL) See [1],
> with the actual assignment still happening in TP_fast_assign.
__string(region, NULL) doesn't make sense. It's like:
len = strlen(NULL);
buf = malloc(len);
strcpy(buf, NULL);
??
I'll reply to that email.
-- Steve
>
> Does that assign logic need to move to the TP_STRUCT__entry definition
> when you merge these changes? I'm not clear how much logic is able to be
> included, ie like 'C' style code in the TP_STRUCT__entry.
>
> [1]
> https://lore.kernel.org/linux-cxl/20240314044301.2108650-1-alison.schofield@intel.com/
prev parent reply other threads:[~2024-03-14 18:32 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-02-23 17:56 [FYI][PATCH] tracing/treewide: Remove second parameter of __assign_str() Steven Rostedt
2024-02-23 18:06 ` Steven Rostedt
2024-02-23 18:30 ` Jeff Johnson
2024-02-23 18:46 ` Steven Rostedt
2024-02-23 19:50 ` Kent Overstreet
2024-02-23 20:03 ` Steven Rostedt
2024-02-23 20:45 ` Steven Rostedt
2024-03-14 16:57 ` Alison Schofield
2024-03-14 18:34 ` Steven Rostedt [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20240314143406.6289a060@gandalf.local.home \
--to=rostedt@goodmis.org \
--cc=Julia.Lawall@inria.fr \
--cc=alison.schofield@intel.com \
--cc=amd-gfx@lists.freedesktop.org \
--cc=ath10k@lists.infradead.org \
--cc=ath11k@lists.infradead.org \
--cc=ath12k@lists.infradead.org \
--cc=bpf@vger.kernel.org \
--cc=brcm80211-dev-list.pdl@broadcom.com \
--cc=brcm80211@lists.linux.dev \
--cc=dev@openvswitch.org \
--cc=dri-devel@lists.freedesktop.org \
--cc=freedreno@lists.freedesktop.org \
--cc=intel-gfx@lists.freedesktop.org \
--cc=intel-xe@lists.freedesktop.org \
--cc=io-uring@vger.kernel.org \
--cc=iommu@lists.linux.dev \
--cc=kvm@vger.kernel.org \
--cc=linux-arm-msm@vger.kernel.org \
--cc=linux-bcachefs@vger.kernel.org \
--cc=linux-block@vger.kernel.org \
--cc=linux-btrfs@vger.kernel.org \
--cc=linux-cifs@vger.kernel.org \
--cc=linux-cxl@vger.kernel.org \
--cc=linux-edac@vger.kernel.org \
--cc=linux-erofs@lists.ozlabs.org \
--cc=linux-f2fs-devel@lists.sourceforge.net \
--cc=linux-hwmon@vger.kernel.org \
--cc=linux-hyperv@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-media@vger.kernel.org \
--cc=linux-nfs@vger.kernel.org \
--cc=linux-pm@vger.kernel.org \
--cc=linux-rdma@vger.kernel.org \
--cc=linux-s390@vger.kernel.org \
--cc=linux-sound@vger.kernel.org \
--cc=linux-tegra@vger.kernel.org \
--cc=linux-trace-kernel@vger.kernel.org \
--cc=linux-usb@vger.kernel.org \
--cc=linux-wireless@vger.kernel.org \
--cc=linux-wpan@vger.kernel.org \
--cc=linux-xfs@vger.kernel.org \
--cc=linuxppc-dev@lists.ozlabs.org \
--cc=mathieu.desnoyers@efficios.com \
--cc=mhiramat@kernel.org \
--cc=netdev@vger.kernel.org \
--cc=ocfs2-devel@lists.linux.dev \
--cc=selinux@vger.kernel.org \
--cc=tipc-discussion@lists.sourceforge.net \
--cc=torvalds@linux-foundation.org \
--cc=virtualization@lists.linux.dev \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).