Re: [FYI][PATCH] tracing/treewide: Remove second parameter of __assign_str()

[Steven Rostedt <rostedt@goodmis.org>]

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(&region->dev));
> > +			__assign_str(region);
> >  			memcpy(__entry->uuid, &region->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/