Messages in this thread | | | From | Nadav Amit <> | Subject | Re: [PATCH] smp: generic ipi_raise tracepoint | Date | Thu, 21 May 2020 19:00:54 +0000 |
| |
> 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&data=02%7C01%7Cnamit%40vmware.com%7Ceb1fce63ca4644ab29ad08d7fcc022df%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637255774462316114&sdata=eKrYH1vLDaEk4QyN4ZLQQRCk%2BtVdGLq7K6xYn1s%2BjJo%3D&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.
| |