> On May 20, 2020, at 6:17 AM, Wojciech Kudla <[email protected]> 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 <[email protected]>
> ---
> 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.
