[PATCH] smp: generic ipi_raise tracepoint

[PATCH] smp: generic ipi_raise tracepoint

[Wojciech Kudla]

Preliminary discussion: https://lkml.org/lkml/2020/5/13/1327
This patch avoids introducing arch-specific trace points by leveraging
existing definition for ipi_raise.

Issues to address in potential future work:
- make ipi reason available on generic smp code level (possible?)
- addition of ipi_entry/ipi_exit tracepoints in generic smp code

Signed-off-by: Wojciech Kudla <wk.kernel@gmail.com>
---
 kernel/smp.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/kernel/smp.c b/kernel/smp.c
index 7dbcb402c2fc..df6982a1d3f2 100644
--- a/kernel/smp.c
+++ b/kernel/smp.c
@@ -23,6 +23,11 @@
 
 #include "smpboot.h"
 
+#define CREATE_TRACE_POINTS
+#include <trace/events/ipi.h>
+
+static const char *ipi_reason_missing __tracepoint_string = "";
+
 enum {
 	CSD_FLAG_LOCK		= 0x01,
 	CSD_FLAG_SYNCHRONOUS	= 0x02,
@@ -34,6 +39,7 @@ struct call_function_data {
 	cpumask_var_t		cpumask_ipi;
 };
 
+
 static DEFINE_PER_CPU_ALIGNED(struct call_function_data, cfd_data);
 
 static DEFINE_PER_CPU_SHARED_ALIGNED(struct llist_head, call_single_queue);
@@ -176,8 +182,12 @@ static int generic_exec_single(int cpu, call_single_data_t *csd,
 	 * locking and barrier primitives. Generic code isn't really
 	 * equipped to do the right thing...
 	 */
-	if (llist_add(&csd->llist, &per_cpu(call_single_queue, cpu)))
+	if (llist_add(&csd->llist, &per_cpu(call_single_queue, cpu))) {
+		if (trace_ipi_raise_enabled())
+			trace_ipi_raise(cpumask_of(cpu), ipi_reason_missing);
+
 		arch_send_call_function_single_ipi(cpu);
+	}
 
 	return 0;
 }
@@ -474,6 +484,8 @@ void smp_call_function_many(const struct cpumask *mask,
 			__cpumask_set_cpu(cpu, cfd->cpumask_ipi);
 	}
 
+	trace_ipi_raise(cfd->cpumask_ipi, ipi_reason_missing);
+
 	/* Send a message to all CPUs in the map */
 	arch_send_call_function_ipi_mask(cfd->cpumask_ipi);
 
-- 
2.17.1

Re: [PATCH] smp: generic ipi_raise tracepoint

[Peter Zijlstra]

On Wed, May 20, 2020 at 02:17:21PM +0100, Wojciech Kudla wrote:
> Preliminary discussion: https://lkml.org/lkml/2020/5/13/1327

We have bright shiny links like: https://lkml.kernel.org/r/$MSG-ID for
that. they allow me to go find the email in my local archive without
having to use a browser.

> This patch avoids introducing arch-specific trace points by leveraging
> existing definition for ipi_raise.


> +static const char *ipi_reason_missing __tracepoint_string = "";

That is a pretty crap reason ;-)

Re: [PATCH] smp: generic ipi_raise tracepoint

[Wojciech Kudla]

On 20/05/2020 14:33, Peter Zijlstra wrote:
> We have bright shiny links like: https://lkml.kernel.org/r/$MSG-ID for
> that. they allow me to go find the email in my local archive without
> having to use a browser.

Apologies, beginner's mistake.

>> +static const char *ipi_reason_missing __tracepoint_string = "";
> 
> That is a pretty crap reason ;-)
> 

I knew this was a long shot. There is no obvious way to 
get/infer ipi reason in generic smp code at the moment.
Any suggestions what we can put here in the meantime?
Would "none" be more appropriate?

Re: [PATCH] smp: generic ipi_raise tracepoint

[Peter Zijlstra]

On Wed, May 20, 2020 at 02:42:10PM +0100, Wojciech Kudla wrote:
> On 20/05/2020 14:33, Peter Zijlstra wrote:
> > We have bright shiny links like: https://lkml.kernel.org/r/$MSG-ID for
> > that. they allow me to go find the email in my local archive without
> > having to use a browser.
> 
> Apologies, beginner's mistake.
> 
> >> +static const char *ipi_reason_missing __tracepoint_string = "";
> > 
> > That is a pretty crap reason ;-)
> > 
> 
> I knew this was a long shot. There is no obvious way to 
> get/infer ipi reason in generic smp code at the moment.
> Any suggestions what we can put here in the meantime?
> Would "none" be more appropriate?

Depends a bit on what you actually want to achieve here.

For actual proper debugging this stuff I think I'd find it more useful
to have tracepoints one level up. So instead of tracing the IPIs, trace
the actual remote function call requests.

And in that case, trace the mask and the symbol name from csd->func.

And if you have that, who cares about the actual IPIs.

Re: [PATCH] smp: generic ipi_raise tracepoint

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2060 bytes --]

Hi Wojciech,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on linus/master]
[also build test ERROR on v5.7-rc6 next-20200519]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Wojciech-Kudla/smp-generic-ipi_raise-tracepoint/20200521-031729
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 115a54162a6c0d0ef2aef25ebd0b61fc5e179ebe
config: arm64-randconfig-r002-20200520 (attached as .config)
compiler: clang version 11.0.0 (https://github.com/llvm/llvm-project 3393cc4cebf9969db94dc424b7a2b6195589c33b)
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install arm64 cross compiling tool for clang build
        # apt-get install binutils-aarch64-linux-gnu
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=arm64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <lkp@intel.com>

All errors (new ones prefixed by >>, old ones prefixed by <<):

>> aarch64-linux-gnu-ld: kernel/smp.o:include/trace/events/ipi.h:19: multiple definition of `__tracepoint_ipi_raise'; arch/arm64/kernel/smp.o:include/trace/events/ipi.h:19: first defined here
>> aarch64-linux-gnu-ld: kernel/smp.o:include/trace/events/ipi.h:64: multiple definition of `__tracepoint_ipi_entry'; arch/arm64/kernel/smp.o:include/trace/events/ipi.h:64: first defined here
>> aarch64-linux-gnu-ld: kernel/smp.o:include/trace/events/ipi.h:80: multiple definition of `__tracepoint_ipi_exit'; arch/arm64/kernel/smp.o:include/trace/events/ipi.h:80: first defined here

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 35834 bytes --]

Re: [PATCH] smp: generic ipi_raise tracepoint

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2096 bytes --]

Hi Wojciech,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on linus/master]
[also build test ERROR on v5.7-rc6 next-20200519]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Wojciech-Kudla/smp-generic-ipi_raise-tracepoint/20200521-031729
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 115a54162a6c0d0ef2aef25ebd0b61fc5e179ebe
config: arm64-randconfig-r002-20200520 (attached as .config)
compiler: clang version 11.0.0 (https://github.com/llvm/llvm-project 3393cc4cebf9969db94dc424b7a2b6195589c33b)
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install arm64 cross compiling tool for clang build
        # apt-get install binutils-aarch64-linux-gnu
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=arm64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <lkp@intel.com>

All errors (new ones prefixed by >>, old ones prefixed by <<):

>> aarch64-linux-gnu-ld: kernel/smp.o:include/trace/events/ipi.h:19: multiple definition of `__tracepoint_ipi_raise'; arch/arm64/kernel/smp.o:include/trace/events/ipi.h:19: first defined here
>> aarch64-linux-gnu-ld: kernel/smp.o:include/trace/events/ipi.h:64: multiple definition of `__tracepoint_ipi_entry'; arch/arm64/kernel/smp.o:include/trace/events/ipi.h:64: first defined here
>> aarch64-linux-gnu-ld: kernel/smp.o:include/trace/events/ipi.h:80: multiple definition of `__tracepoint_ipi_exit'; arch/arm64/kernel/smp.o:include/trace/events/ipi.h:80: first defined here

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 35834 bytes --]

Re: [PATCH] smp: generic ipi_raise tracepoint

[Nadav Amit]

> On May 20, 2020, at 6:17 AM, Wojciech Kudla <wk.kernel@gmail.com> wrote:
> 
> Preliminary discussion: https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Flkml.org%2Flkml%2F2020%2F5%2F13%2F1327&amp;data=02%7C01%7Cnamit%40vmware.com%7Ceb1fce63ca4644ab29ad08d7fcc022df%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637255774462316114&amp;sdata=eKrYH1vLDaEk4QyN4ZLQQRCk%2BtVdGLq7K6xYn1s%2BjJo%3D&amp;reserved=0
> This patch avoids introducing arch-specific trace points by leveraging
> existing definition for ipi_raise.
> 
> Issues to address in potential future work:
> - make ipi reason available on generic smp code level (possible?)
> - addition of ipi_entry/ipi_exit tracepoints in generic smp code
> 
> Signed-off-by: Wojciech Kudla <wk.kernel@gmail.com>
> ---
> kernel/smp.c | 14 +++++++++++++-
> 1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/smp.c b/kernel/smp.c
> index 7dbcb402c2fc..df6982a1d3f2 100644
> --- a/kernel/smp.c
> +++ b/kernel/smp.c
> @@ -23,6 +23,11 @@
> 
> #include "smpboot.h"
> 
> +#define CREATE_TRACE_POINTS
> +#include <trace/events/ipi.h>
> +
> +static const char *ipi_reason_missing __tracepoint_string = "";
> +
> enum {
> 	CSD_FLAG_LOCK		= 0x01,
> 	CSD_FLAG_SYNCHRONOUS	= 0x02,
> @@ -34,6 +39,7 @@ struct call_function_data {
> 	cpumask_var_t		cpumask_ipi;
> };
> 
> +

Unneeded redundant new line.

> static DEFINE_PER_CPU_ALIGNED(struct call_function_data, cfd_data);
> 
> static DEFINE_PER_CPU_SHARED_ALIGNED(struct llist_head, call_single_queue);
> @@ -176,8 +182,12 @@ static int generic_exec_single(int cpu, call_single_data_t *csd,
> 	 * locking and barrier primitives. Generic code isn't really
> 	 * equipped to do the right thing...
> 	 */
> -	if (llist_add(&csd->llist, &per_cpu(call_single_queue, cpu)))
> +	if (llist_add(&csd->llist, &per_cpu(call_single_queue, cpu))) {
> +		if (trace_ipi_raise_enabled())

Why do you need this check? trace_ipi_raise() will do the same check before
actual tracing:

	if (static_key_false(&__tracepoint_##name.key)

I guess you do it in order to avoid evaluation of cpumask_of() if tracing is
disabled, but it seems to me that the macro would only evaluate/call
cpumask_of() if tracing is indeed enabled.

> +			trace_ipi_raise(cpumask_of(cpu), ipi_reason_missing);
> +
> 		arch_send_call_function_single_ipi(cpu);
> +	}

In general, I think there are too many trace-points. They look benign(i.e.,
free), but can cause worse code to be generated as they behave as a memory
clobber. Many times the same result can be achieved with a probe.

Re: [PATCH] smp: generic ipi_raise tracepoint

[Wojciech Kudla]

On 21/05/2020 20:00, Nadav Amit wrote:

>> -	if (llist_add(&csd->llist, &per_cpu(call_single_queue, cpu)))
>> +	if (llist_add(&csd->llist, &per_cpu(call_single_queue, cpu))) {
>> +		if (trace_ipi_raise_enabled())
> 
> Why do you need this check? trace_ipi_raise() will do the same check before
> actual tracing:
> 
> 	if (static_key_false(&__tracepoint_##name.key)
> 

Yes, my motivation for conditional logic was performance-driven.
Thanks for pointing out the implicit check.

> 
> In general, I think there are too many trace-points. They look benign(i.e.,
> free), but can cause worse code to be generated as they behave as a memory
> clobber. Many times the same result can be achieved with a probe.
> 

Thank you for the review, I agree this may not be optimal. Let's just stop here.
There's a different patch I submitted today that follows Peter's suggestions
about smp function calls being much more sensible target for new tracepoints.

http://lkml.kernel.org/r/e6141d56-1da1-6c00-198f-cbe4385327ff@gmail.com

Thanks,
W.