Re: [f2fs-dev] [PATCH] tracing/treewide: Remove second parameter of __assign_str()
Hello: This patch was applied to jaegeuk/f2fs.git (dev) by Steven Rostedt (Google) : On Thu, 16 May 2024 13:34:54 -0400 you wrote: > From: "Steven Rostedt (Google)" > > [ >This is a treewide change. I will likely re-create this patch again in >the second week of the merge window of v6.10 and submit it then. Hoping >to keep the conflicts that it will cause to a minimum. > ] > > [...] Here is the summary with links: - [f2fs-dev] tracing/treewide: Remove second parameter of __assign_str() https://git.kernel.org/jaegeuk/f2fs/c/2c92ca849fcc You are awesome, thank you! -- Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/patchwork/pwbot.html
Re: [PATCH] tracing/treewide: Remove second parameter of __assign_str()
On 5/17/24 11:00, Guenter Roeck wrote: On 5/17/24 10:48, Steven Rostedt wrote: On Fri, 17 May 2024 10:36:37 -0700 Guenter Roeck wrote: Building csky:allmodconfig (and others) ... failed -- Error log: In file included from include/trace/trace_events.h:419, from include/trace/define_trace.h:102, from drivers/cxl/core/trace.h:737, from drivers/cxl/core/trace.c:8: drivers/cxl/core/./trace.h:383:1: error: macro "__assign_str" passed 2 arguments, but takes just 1 This is with the patch applied on top of v6.9-8410-gff2632d7d08e. So far that seems to be the only build failure. Introduced with commit 6aec00139d3a8 ("cxl/core: Add region info to cxl_general_media and cxl_dram events"). Guess we'll see more of those towards the end of the commit window. Looks like I made this patch just before this commit was pulled into Linus's tree. Which is why I'll apply and rerun the above again probably on Tuesday of next week against Linus's latest. This patch made it through both an allyesconfig and an allmodconfig, but on the commit I had applied it to, which was: 1b294a1f3561 ("Merge tag 'net-next-6.10' of git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next") I'll be compiling those two builds after I update it then. I am currently repeating my test builds with the above errors fixed. That should take a couple of hours. I'll let you know how it goes. There are no more build failures caused by this patch after fixing the above errors. Tested-by: Guenter Roeck Guenter
Re: [PATCH] tracing/treewide: Remove second parameter of __assign_str()
On 5/17/24 10:48, Steven Rostedt wrote: On Fri, 17 May 2024 10:36:37 -0700 Guenter Roeck wrote: Building csky:allmodconfig (and others) ... failed -- Error log: In file included from include/trace/trace_events.h:419, from include/trace/define_trace.h:102, from drivers/cxl/core/trace.h:737, from drivers/cxl/core/trace.c:8: drivers/cxl/core/./trace.h:383:1: error: macro "__assign_str" passed 2 arguments, but takes just 1 This is with the patch applied on top of v6.9-8410-gff2632d7d08e. So far that seems to be the only build failure. Introduced with commit 6aec00139d3a8 ("cxl/core: Add region info to cxl_general_media and cxl_dram events"). Guess we'll see more of those towards the end of the commit window. Looks like I made this patch just before this commit was pulled into Linus's tree. Which is why I'll apply and rerun the above again probably on Tuesday of next week against Linus's latest. This patch made it through both an allyesconfig and an allmodconfig, but on the commit I had applied it to, which was: 1b294a1f3561 ("Merge tag 'net-next-6.10' of git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next") I'll be compiling those two builds after I update it then. I am currently repeating my test builds with the above errors fixed. That should take a couple of hours. I'll let you know how it goes. Guenter
Re: [PATCH] tracing/treewide: Remove second parameter of __assign_str()
On Fri, 17 May 2024 10:36:37 -0700 Guenter Roeck wrote: > Building csky:allmodconfig (and others) ... failed > -- > Error log: > In file included from include/trace/trace_events.h:419, > from include/trace/define_trace.h:102, > from drivers/cxl/core/trace.h:737, > from drivers/cxl/core/trace.c:8: > drivers/cxl/core/./trace.h:383:1: error: macro "__assign_str" passed 2 > arguments, but takes just 1 > > This is with the patch applied on top of v6.9-8410-gff2632d7d08e. > So far that seems to be the only build failure. > Introduced with commit 6aec00139d3a8 ("cxl/core: Add region info to > cxl_general_media and cxl_dram events"). Guess we'll see more of those > towards the end of the commit window. Looks like I made this patch just before this commit was pulled into Linus's tree. Which is why I'll apply and rerun the above again probably on Tuesday of next week against Linus's latest. This patch made it through both an allyesconfig and an allmodconfig, but on the commit I had applied it to, which was: 1b294a1f3561 ("Merge tag 'net-next-6.10' of git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next") I'll be compiling those two builds after I update it then. -- Steve
Re: [PATCH] tracing/treewide: Remove second parameter of __assign_str()
On Thu, May 16, 2024 at 01:34:54PM -0400, Steven Rostedt wrote: > From: "Steven Rostedt (Google)" > > [ >This is a treewide change. I will likely re-create this patch again in >the second week of the merge window of v6.10 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. > > This means that with: > > __string(field, mystring) > > Which use to be assigned with __assign_str(field, mystring), no longer > needs the second parameter and it is unused. With this, __assign_str() > will now only get a single parameter. > > There's over 700 users of __assign_str() and because coccinelle does not > handle the TRACE_EVENT() macro I ended up using the following sed script: > > git grep -l __assign_str | while read a ; do > sed -e 's/\(__assign_str([^,]*[^ ,]\) *,[^;]*/\1)/' $a > /tmp/test-file; > mv /tmp/test-file $a; > done > > I then searched for __assign_str() that did not end with ';' as those > were multi line assignments that the sed script above would fail to catch. > Building csky:allmodconfig (and others) ... failed -- Error log: In file included from include/trace/trace_events.h:419, from include/trace/define_trace.h:102, from drivers/cxl/core/trace.h:737, from drivers/cxl/core/trace.c:8: drivers/cxl/core/./trace.h:383:1: error: macro "__assign_str" passed 2 arguments, but takes just 1 This is with the patch applied on top of v6.9-8410-gff2632d7d08e. So far that seems to be the only build failure. Introduced with commit 6aec00139d3a8 ("cxl/core: Add region info to cxl_general_media and cxl_dram events"). Guess we'll see more of those towards the end of the commit window. Guenter
Re: [PATCH] tracing/treewide: Remove second parameter of __assign_str()
On Thu, May 16, 2024 at 01:34:54PM -0400, Steven Rostedt wrote: > From: "Steven Rostedt (Google)" > > [ >This is a treewide change. I will likely re-create this patch again in >the second week of the merge window of v6.10 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. > > This means that with: > > __string(field, mystring) > > Which use to be assigned with __assign_str(field, mystring), no longer > needs the second parameter and it is unused. With this, __assign_str() > will now only get a single parameter. > > There's over 700 users of __assign_str() and because coccinelle does not > handle the TRACE_EVENT() macro I ended up using the following sed script: > > git grep -l __assign_str | while read a ; do > sed -e 's/\(__assign_str([^,]*[^ ,]\) *,[^;]*/\1)/' $a > /tmp/test-file; > mv /tmp/test-file $a; > done > > I then searched for __assign_str() that did not end with ';' as those > were multi line assignments that the sed script above would fail to catch. > > Note, the same updates will need to be done for: > > __assign_str_len() > __assign_rel_str() > __assign_rel_str_len() > > I tested this with both an allmodconfig and an allyesconfig (build only for > both). > > [1] > https://lore.kernel.org/linux-trace-kernel/2024011442.634192...@goodmis.org/ > > Cc: Masami Hiramatsu > Cc: Mathieu Desnoyers > Cc: Linus Torvalds > Cc: Julia Lawall > Signed-off-by: Steven Rostedt (Google) /me finds this pretty magical, but such is the way of macros. Thanks for being much smarter about them than me. :) Acked-by: Darrick J. Wong# xfs --D
Re: [PATCH] tracing/treewide: Remove second parameter of __assign_str()
On Thu, 16 May 2024 19:34:54 +0200, Steven Rostedt wrote: > > From: "Steven Rostedt (Google)" > > [ >This is a treewide change. I will likely re-create this patch again in >the second week of the merge window of v6.10 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. > > This means that with: > > __string(field, mystring) > > Which use to be assigned with __assign_str(field, mystring), no longer > needs the second parameter and it is unused. With this, __assign_str() > will now only get a single parameter. > > There's over 700 users of __assign_str() and because coccinelle does not > handle the TRACE_EVENT() macro I ended up using the following sed script: > > git grep -l __assign_str | while read a ; do > sed -e 's/\(__assign_str([^,]*[^ ,]\) *,[^;]*/\1)/' $a > /tmp/test-file; > mv /tmp/test-file $a; > done > > I then searched for __assign_str() that did not end with ';' as those > were multi line assignments that the sed script above would fail to catch. > > Note, the same updates will need to be done for: > > __assign_str_len() > __assign_rel_str() > __assign_rel_str_len() > > I tested this with both an allmodconfig and an allyesconfig (build only for > both). > > [1] > https://lore.kernel.org/linux-trace-kernel/2024011442.634192...@goodmis.org/ > > Cc: Masami Hiramatsu > Cc: Mathieu Desnoyers > Cc: Linus Torvalds > Cc: Julia Lawall > Signed-off-by: Steven Rostedt (Google) For the sound part Acked-by: Takashi Iwai thanks, Takashi
Re: [PATCH] tracing/treewide: Remove second parameter of __assign_str()
On Thu, May 16, 2024 at 7:35 PM Steven Rostedt wrote: > > From: "Steven Rostedt (Google)" > > [ >This is a treewide change. I will likely re-create this patch again in >the second week of the merge window of v6.10 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. > > This means that with: > > __string(field, mystring) > > Which use to be assigned with __assign_str(field, mystring), no longer > needs the second parameter and it is unused. With this, __assign_str() > will now only get a single parameter. > > There's over 700 users of __assign_str() and because coccinelle does not > handle the TRACE_EVENT() macro I ended up using the following sed script: > > git grep -l __assign_str | while read a ; do > sed -e 's/\(__assign_str([^,]*[^ ,]\) *,[^;]*/\1)/' $a > /tmp/test-file; > mv /tmp/test-file $a; > done > > I then searched for __assign_str() that did not end with ';' as those > were multi line assignments that the sed script above would fail to catch. > > Note, the same updates will need to be done for: > > __assign_str_len() > __assign_rel_str() > __assign_rel_str_len() > > I tested this with both an allmodconfig and an allyesconfig (build only for > both). > > [1] > https://lore.kernel.org/linux-trace-kernel/2024011442.634192...@goodmis.org/ > > Cc: Masami Hiramatsu > Cc: Mathieu Desnoyers > Cc: Linus Torvalds > Cc: Julia Lawall > Signed-off-by: Steven Rostedt (Google) Acked-by: Rafael J. Wysocki # for thermal
Re: [FYI][PATCH] tracing/treewide: Remove second parameter of __assign_str()
On Thu, 14 Mar 2024 09:57:57 -0700 Alison Schofield wrote: > On Fri, Feb 23, 2024 at 12:56:34PM -0500, Steven Rostedt wrote: > > From: "Steven Rostedt (Google)" > > > > [ > >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.00031...@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.schofi...@intel.com/
Re: [FYI][PATCH] tracing/treewide: Remove second parameter of __assign_str()
On Fri, Feb 23, 2024 at 12:56:34PM -0500, Steven Rostedt wrote: > From: "Steven Rostedt (Google)" > > [ >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. > > This means that with: > > __string(field, mystring) > > Which use to be assigned with __assign_str(field, mystring), no longer > needs the second parameter and it is unused. With this, __assign_str() > will now only get a single parameter. > > There's over 700 users of __assign_str() and because coccinelle does not > handle the TRACE_EVENT() macro I ended up using the following sed script: > > git grep -l __assign_str | while read a ; do > sed -e 's/\(__assign_str([^,]*[^ ,]\) *,[^;]*/\1)/' $a > /tmp/test-file; > mv /tmp/test-file $a; > done > > I then searched for __assign_str() that did not end with ';' as those > were multi line assignments that the sed script above would fail to catch. > > Note, the same updates will need to be done for: > > __assign_str_len() > __assign_rel_str() > __assign_rel_str_len() > __assign_bitmask() > __assign_rel_bitmask() > __assign_cpumask() > __assign_rel_cpumask() > > [1] > https://lore.kernel.org/linux-trace-kernel/2024011442.634192...@goodmis.org/ > > Signed-off-by: Steven Rostedt (Google) > --- > arch/arm64/kernel/trace-events-emulation.h| 2 +- > arch/powerpc/include/asm/trace.h | 4 +- > arch/x86/kvm/trace.h | 2 +- > drivers/base/regmap/trace.h | 18 +-- > drivers/base/trace.h | 2 +- > drivers/block/rnbd/rnbd-srv-trace.h | 12 +- > drivers/cxl/core/trace.h | 24 ++-- snip to CXL > 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)) > __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. 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.schofi...@intel.com/ Thanks for helping, Alison > }
Re: [FYI][PATCH] tracing/treewide: Remove second parameter of __assign_str()
On Fri, Feb 23, 2024 at 01:46:53PM -0500, Steven Rostedt wrote: > On Fri, 23 Feb 2024 10:30:45 -0800 > Jeff Johnson wrote: > > > On 2/23/2024 9:56 AM, Steven Rostedt wrote: > > > From: "Steven Rostedt (Google)" > > > > > > [ > > >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?) Agreed on doing it all at once, it'll be way less spam for people to deal with. Tangentially related though, what would make me really happy is if we could create the string with in the TP__fast_assign() section. I have to have a bunch of annoying wrappers right now because the string length has to be known when we invoke the tracepoint.
Re: [FYI][PATCH] tracing/treewide: Remove second parameter of __assign_str()
On Fri, 23 Feb 2024 13:46:53 -0500 Steven Rostedt wrote: > 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. Hmm, maybe I'll just add this patch for 6.9 and then in 6.10 do the parameter removal. -- Steve diff --git a/include/trace/stages/stage6_event_callback.h b/include/trace/stages/stage6_event_callback.h index 0c0f50bcdc56..7372e2c2a0c4 100644 --- a/include/trace/stages/stage6_event_callback.h +++ b/include/trace/stages/stage6_event_callback.h @@ -35,6 +35,7 @@ #define __assign_str(dst, src) do {\ char *__str__ = __get_str(dst); \ int __len__ = __get_dynamic_array_len(dst) - 1; \ + WARN_ON_ONCE((src) != __data_offsets.dst##_ptr_); \ memcpy(__str__, __data_offsets.dst##_ptr_ ? : \ EVENT_NULL_STR, __len__);\ __str__[__len__] = '\0';\
Re: [FYI][PATCH] tracing/treewide: Remove second parameter of __assign_str()
On Fri, 23 Feb 2024 14:50:49 -0500 Kent Overstreet wrote: > Tangentially related though, what would make me really happy is if we > could create the string with in the TP__fast_assign() section. I have to > have a bunch of annoying wrappers right now because the string length > has to be known when we invoke the tracepoint. You can use __string_len() to determine the string length in the tracepoint (which is executed in the TP_fast_assign() section). My clean up patches will make __assign_str_len() obsolete too (I'm working on them now), and you can just use __assign_str(). I noticed that I don't have a string_len example in the sample code and I'm actually writing it now. // cutting out everything else: TRACE_EVENT(foo_bar, TP_PROTO(const char *foo, int bar), TP_ARGS(foo, bar), TP_STRUCT__entry( __string_len( lstr, foo,bar < strlen(foo) ? bar : strlen(foo) ) ), TP_fast_assign( __assign_str(lstr, foo); // Note, the above is with my updates, without them, you need to duplicate the logic // __assign_str_len(lstr, foo, bar < strlen(foo) ? bar : strlen(foo)); ), TP_printk("%s", __get_str(lstr)) ); The above will allocate "bar < strlen(foo) ? bar : strlen(foo)" size on the ring buffer. As the size is already stored, my clean up code uses that instead of requiring duplicating the logic again. -- Steve
Re: [FYI][PATCH] tracing/treewide: Remove second parameter of __assign_str()
On Fri, 23 Feb 2024 10:30:45 -0800 Jeff Johnson wrote: > On 2/23/2024 9:56 AM, Steven Rostedt wrote: > > From: "Steven Rostedt (Google)" > > > > [ > >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
Re: [FYI][PATCH] tracing/treewide: Remove second parameter of __assign_str()
On 2/23/2024 9:56 AM, Steven Rostedt wrote: > From: "Steven Rostedt (Google)" > > [ >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.
Re: [FYI][PATCH] tracing/treewide: Remove second parameter of __assign_str()
On Fri, 23 Feb 2024 12:56:34 -0500 Steven Rostedt wrote: > Note, the same updates will need to be done for: > > __assign_str_len() > __assign_rel_str() > __assign_rel_str_len() Correction: The below macros do not pass in their source to the entry macros, so they will not need to be updated. -- Steve > __assign_bitmask() > __assign_rel_bitmask() > __assign_cpumask() > __assign_rel_cpumask()