Re: [PATCH] smp: generic ipi_raise tracepoint

[Nadav Amit]

> On May 20, 2020, at 6:17 AM, Wojciech Kudla <wk.kernel@xxxxxxxxx> 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@xxxxxxxxx>
> ---
> 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.