lkml.org 
[lkml]   [2020]   [May]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH] smp: generic ipi_raise tracepoint
Date
> 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.

\
 
 \ /
  Last update: 2020-05-21 21:01    [W:0.065 / U:1.104 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site