* [PATCH] tracepoints: Code clean up
@ 2021-02-04 18:27 Steven Rostedt
2021-02-04 19:13 ` Mathieu Desnoyers
0 siblings, 1 reply; 3+ messages in thread
From: Steven Rostedt @ 2021-02-04 18:27 UTC (permalink / raw)
To: LKML; +Cc: Mathieu Desnoyers, Ingo Molnar, Andrew Morton, Peter Zijlstra
From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
Restructure the code a bit to make it simpler, fix some formatting problems
and add READ_ONCE/WRITE_ONCE to make sure there's no compiler tear downs on
changes to variables that can be accessed across CPUs.
Started with Mathieu Desnoyers's patch:
Link: https://lore.kernel.org/lkml/20210203175741.20665-1-mathieu.desnoyers@efficios.com/
And will keep his signature, but I will take the responsibility of this
being correct, and keep the authorship.
Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
include/linux/tracepoint.h | 2 +-
kernel/tracepoint.c | 92 +++++++++++++++-----------------------
2 files changed, 37 insertions(+), 57 deletions(-)
diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
index 966ed8980327..dc1d4c612cc3 100644
--- a/include/linux/tracepoint.h
+++ b/include/linux/tracepoint.h
@@ -309,7 +309,7 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
rcu_dereference_raw((&__tracepoint_##_name)->funcs); \
if (it_func_ptr) { \
do { \
- it_func = (it_func_ptr)->func; \
+ it_func = READ_ONCE((it_func_ptr)->func); \
__data = (it_func_ptr)->data; \
((void(*)(void *, proto))(it_func))(__data, args); \
} while ((++it_func_ptr)->func); \
diff --git a/kernel/tracepoint.c b/kernel/tracepoint.c
index e8f20ae29c18..4b9de79bb927 100644
--- a/kernel/tracepoint.c
+++ b/kernel/tracepoint.c
@@ -136,9 +136,9 @@ func_add(struct tracepoint_func **funcs, struct tracepoint_func *tp_func,
int prio)
{
struct tracepoint_func *old, *new;
- int nr_probes = 0;
- int stub_funcs = 0;
- int pos = -1;
+ int iter_probes; /* Iterate over old probe array. */
+ int nr_probes = 0; /* Counter for probes */
+ int pos = -1; /* New position */
if (WARN_ON(!tp_func->func))
return ERR_PTR(-EINVAL);
@@ -147,54 +147,39 @@ func_add(struct tracepoint_func **funcs, struct tracepoint_func *tp_func,
old = *funcs;
if (old) {
/* (N -> N+1), (N != 0, 1) probes */
- for (nr_probes = 0; old[nr_probes].func; nr_probes++) {
- /* Insert before probes of lower priority */
- if (pos < 0 && old[nr_probes].prio < prio)
- pos = nr_probes;
- if (old[nr_probes].func == tp_func->func &&
- old[nr_probes].data == tp_func->data)
+ for (iter_probes = 0; old[iter_probes].func; iter_probes++) {
+ if (old[iter_probes].func == tp_stub_func)
+ continue; /* Skip stub functions. */
+ if (old[iter_probes].func == tp_func->func &&
+ old[iter_probes].data == tp_func->data)
return ERR_PTR(-EEXIST);
- if (old[nr_probes].func == tp_stub_func)
- stub_funcs++;
+ nr_probes++;
}
}
- /* + 2 : one for new probe, one for NULL func - stub functions */
- new = allocate_probes(nr_probes + 2 - stub_funcs);
+ /* + 2 : one for new probe, one for NULL func */
+ new = allocate_probes(nr_probes + 2);
if (new == NULL)
return ERR_PTR(-ENOMEM);
if (old) {
- if (stub_funcs) {
- /* Need to copy one at a time to remove stubs */
- int probes = 0;
-
- pos = -1;
- for (nr_probes = 0; old[nr_probes].func; nr_probes++) {
- if (old[nr_probes].func == tp_stub_func)
- continue;
- if (pos < 0 && old[nr_probes].prio < prio)
- pos = probes++;
- new[probes++] = old[nr_probes];
- }
- nr_probes = probes;
- if (pos < 0)
- pos = probes;
- else
- nr_probes--; /* Account for insertion */
-
- } else if (pos < 0) {
- pos = nr_probes;
- memcpy(new, old, nr_probes * sizeof(struct tracepoint_func));
- } else {
- /* Copy higher priority probes ahead of the new probe */
- memcpy(new, old, pos * sizeof(struct tracepoint_func));
- /* Copy the rest after it. */
- memcpy(new + pos + 1, old + pos,
- (nr_probes - pos) * sizeof(struct tracepoint_func));
+ pos = -1;
+ nr_probes = 0;
+ for (iter_probes = 0; old[iter_probes].func; iter_probes++) {
+ if (old[iter_probes].func == tp_stub_func)
+ continue;
+ /* Insert before probes of lower priority */
+ if (pos < 0 && old[iter_probes].prio < prio)
+ pos = nr_probes++;
+ new[nr_probes++] = old[iter_probes];
}
- } else
+ if (pos < 0)
+ pos = nr_probes++;
+ /* nr_probes now points to the end of the new array */
+ } else {
pos = 0;
+ nr_probes = 1; /* must point at end of array */
+ }
new[pos] = *tp_func;
- new[nr_probes + 1].func = NULL;
+ new[nr_probes].func = NULL;
*funcs = new;
debug_print_probes(*funcs);
return old;
@@ -237,11 +222,12 @@ static void *func_remove(struct tracepoint_func **funcs,
/* + 1 for NULL */
new = allocate_probes(nr_probes - nr_del + 1);
if (new) {
- for (i = 0; old[i].func; i++)
- if ((old[i].func != tp_func->func
- || old[i].data != tp_func->data)
- && old[i].func != tp_stub_func)
+ for (i = 0; old[i].func; i++) {
+ if ((old[i].func != tp_func->func ||
+ old[i].data != tp_func->data) &&
+ old[i].func != tp_stub_func)
new[j++] = old[i];
+ }
new[nr_probes - nr_del].func = NULL;
*funcs = new;
} else {
@@ -249,17 +235,11 @@ static void *func_remove(struct tracepoint_func **funcs,
* Failed to allocate, replace the old function
* with calls to tp_stub_func.
*/
- for (i = 0; old[i].func; i++)
+ for (i = 0; old[i].func; i++) {
if (old[i].func == tp_func->func &&
- old[i].data == tp_func->data) {
- old[i].func = tp_stub_func;
- /* Set the prio to the next event. */
- if (old[i + 1].func)
- old[i].prio =
- old[i + 1].prio;
- else
- old[i].prio = -1;
- }
+ old[i].data == tp_func->data)
+ WRITE_ONCE(old[i].func, tp_stub_func);
+ }
*funcs = old;
}
}
--
2.25.4
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH] tracepoints: Code clean up
2021-02-04 18:27 [PATCH] tracepoints: Code clean up Steven Rostedt
@ 2021-02-04 19:13 ` Mathieu Desnoyers
2021-02-04 19:21 ` Steven Rostedt
0 siblings, 1 reply; 3+ messages in thread
From: Mathieu Desnoyers @ 2021-02-04 19:13 UTC (permalink / raw)
To: rostedt; +Cc: linux-kernel, Ingo Molnar, Andrew Morton, Peter Zijlstra
----- On Feb 4, 2021, at 1:27 PM, rostedt rostedt@goodmis.org wrote:
> From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
>
> Restructure the code a bit to make it simpler, fix some formatting problems
> and add READ_ONCE/WRITE_ONCE to make sure there's no compiler tear downs on
compiler tear downs -> compiler load/store tearing.
> changes to variables that can be accessed across CPUs.
>
> Started with Mathieu Desnoyers's patch:
>
> Link:
> https://lore.kernel.org/lkml/20210203175741.20665-1-mathieu.desnoyers@efficios.com/
>
> And will keep his signature, but I will take the responsibility of this
> being correct, and keep the authorship.
>
> Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> ---
> include/linux/tracepoint.h | 2 +-
> kernel/tracepoint.c | 92 +++++++++++++++-----------------------
> 2 files changed, 37 insertions(+), 57 deletions(-)
>
> diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
> index 966ed8980327..dc1d4c612cc3 100644
> --- a/include/linux/tracepoint.h
> +++ b/include/linux/tracepoint.h
> @@ -309,7 +309,7 @@ static inline struct tracepoint
> *tracepoint_ptr_deref(tracepoint_ptr_t *p)
> rcu_dereference_raw((&__tracepoint_##_name)->funcs); \
> if (it_func_ptr) { \
> do { \
> - it_func = (it_func_ptr)->func; \
> + it_func = READ_ONCE((it_func_ptr)->func); \
> __data = (it_func_ptr)->data; \
> ((void(*)(void *, proto))(it_func))(__data, args); \
> } while ((++it_func_ptr)->func); \
> diff --git a/kernel/tracepoint.c b/kernel/tracepoint.c
> index e8f20ae29c18..4b9de79bb927 100644
> --- a/kernel/tracepoint.c
> +++ b/kernel/tracepoint.c
> @@ -136,9 +136,9 @@ func_add(struct tracepoint_func **funcs, struct
> tracepoint_func *tp_func,
> int prio)
> {
> struct tracepoint_func *old, *new;
> - int nr_probes = 0;
> - int stub_funcs = 0;
> - int pos = -1;
> + int iter_probes; /* Iterate over old probe array. */
> + int nr_probes = 0; /* Counter for probes */
> + int pos = -1; /* New position */
New position -> Insertion position into new array
>
> if (WARN_ON(!tp_func->func))
> return ERR_PTR(-EINVAL);
> @@ -147,54 +147,39 @@ func_add(struct tracepoint_func **funcs, struct
> tracepoint_func *tp_func,
> old = *funcs;
> if (old) {
> /* (N -> N+1), (N != 0, 1) probes */
> - for (nr_probes = 0; old[nr_probes].func; nr_probes++) {
> - /* Insert before probes of lower priority */
> - if (pos < 0 && old[nr_probes].prio < prio)
> - pos = nr_probes;
> - if (old[nr_probes].func == tp_func->func &&
> - old[nr_probes].data == tp_func->data)
> + for (iter_probes = 0; old[iter_probes].func; iter_probes++) {
> + if (old[iter_probes].func == tp_stub_func)
> + continue; /* Skip stub functions. */
> + if (old[iter_probes].func == tp_func->func &&
> + old[iter_probes].data == tp_func->data)
> return ERR_PTR(-EEXIST);
> - if (old[nr_probes].func == tp_stub_func)
> - stub_funcs++;
> + nr_probes++;
> }
> }
> - /* + 2 : one for new probe, one for NULL func - stub functions */
> - new = allocate_probes(nr_probes + 2 - stub_funcs);
> + /* + 2 : one for new probe, one for NULL func */
> + new = allocate_probes(nr_probes + 2);
> if (new == NULL)
> return ERR_PTR(-ENOMEM);
> if (old) {
> - if (stub_funcs) {
> - /* Need to copy one at a time to remove stubs */
> - int probes = 0;
> -
> - pos = -1;
> - for (nr_probes = 0; old[nr_probes].func; nr_probes++) {
> - if (old[nr_probes].func == tp_stub_func)
> - continue;
> - if (pos < 0 && old[nr_probes].prio < prio)
> - pos = probes++;
> - new[probes++] = old[nr_probes];
> - }
> - nr_probes = probes;
> - if (pos < 0)
> - pos = probes;
> - else
> - nr_probes--; /* Account for insertion */
> -
> - } else if (pos < 0) {
> - pos = nr_probes;
> - memcpy(new, old, nr_probes * sizeof(struct tracepoint_func));
> - } else {
> - /* Copy higher priority probes ahead of the new probe */
> - memcpy(new, old, pos * sizeof(struct tracepoint_func));
> - /* Copy the rest after it. */
> - memcpy(new + pos + 1, old + pos,
> - (nr_probes - pos) * sizeof(struct tracepoint_func));
> + pos = -1;
pos is already initialized to -1 at function beginning.
> + nr_probes = 0;
> + for (iter_probes = 0; old[iter_probes].func; iter_probes++) {
> + if (old[iter_probes].func == tp_stub_func)
> + continue;
> + /* Insert before probes of lower priority */
> + if (pos < 0 && old[iter_probes].prio < prio)
> + pos = nr_probes++;
> + new[nr_probes++] = old[iter_probes];
> }
> - } else
> + if (pos < 0)
> + pos = nr_probes++;
> + /* nr_probes now points to the end of the new array */
> + } else {
> pos = 0;
> + nr_probes = 1; /* must point at end of array */
Yep, much nicer.
> + }
> new[pos] = *tp_func;
> - new[nr_probes + 1].func = NULL;
> + new[nr_probes].func = NULL;
> *funcs = new;
> debug_print_probes(*funcs);
> return old;
> @@ -237,11 +222,12 @@ static void *func_remove(struct tracepoint_func **funcs,
> /* + 1 for NULL */
> new = allocate_probes(nr_probes - nr_del + 1);
> if (new) {
> - for (i = 0; old[i].func; i++)
> - if ((old[i].func != tp_func->func
> - || old[i].data != tp_func->data)
> - && old[i].func != tp_stub_func)
> + for (i = 0; old[i].func; i++) {
> + if ((old[i].func != tp_func->func ||
> + old[i].data != tp_func->data) &&
> + old[i].func != tp_stub_func)
> new[j++] = old[i];
> + }
> new[nr_probes - nr_del].func = NULL;
> *funcs = new;
> } else {
> @@ -249,17 +235,11 @@ static void *func_remove(struct tracepoint_func **funcs,
> * Failed to allocate, replace the old function
> * with calls to tp_stub_func.
> */
> - for (i = 0; old[i].func; i++)
> + for (i = 0; old[i].func; i++) {
> if (old[i].func == tp_func->func &&
> - old[i].data == tp_func->data) {
> - old[i].func = tp_stub_func;
> - /* Set the prio to the next event. */
> - if (old[i + 1].func)
> - old[i].prio =
> - old[i + 1].prio;
> - else
> - old[i].prio = -1;
> - }
> + old[i].data == tp_func->data)
> + WRITE_ONCE(old[i].func, tp_stub_func);
> + }
The rest looks good, thanks!
You can keep my signed-off-by, and if needed may add my reviewed-by tag
as well. ;-)
Mathieu
> *funcs = old;
> }
> }
> --
> 2.25.4
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH] tracepoints: Code clean up
2021-02-04 19:13 ` Mathieu Desnoyers
@ 2021-02-04 19:21 ` Steven Rostedt
0 siblings, 0 replies; 3+ messages in thread
From: Steven Rostedt @ 2021-02-04 19:21 UTC (permalink / raw)
To: Mathieu Desnoyers
Cc: linux-kernel, Ingo Molnar, Andrew Morton, Peter Zijlstra
On Thu, 4 Feb 2021 14:13:59 -0500 (EST)
Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote:
> ----- On Feb 4, 2021, at 1:27 PM, rostedt rostedt@goodmis.org wrote:
>
> > From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
> >
> > Restructure the code a bit to make it simpler, fix some formatting problems
> > and add READ_ONCE/WRITE_ONCE to make sure there's no compiler tear downs on
>
> compiler tear downs -> compiler load/store tearing.
Will change.
>
> > changes to variables that can be accessed across CPUs.
> >
> > Started with Mathieu Desnoyers's patch:
> >
> > Link:
> > https://lore.kernel.org/lkml/20210203175741.20665-1-mathieu.desnoyers@efficios.com/
> >
> > And will keep his signature, but I will take the responsibility of this
> > being correct, and keep the authorship.
> >
> > Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> > Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> > ---
> > include/linux/tracepoint.h | 2 +-
> > kernel/tracepoint.c | 92 +++++++++++++++-----------------------
> > 2 files changed, 37 insertions(+), 57 deletions(-)
> >
> > diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
> > index 966ed8980327..dc1d4c612cc3 100644
> > --- a/include/linux/tracepoint.h
> > +++ b/include/linux/tracepoint.h
> > @@ -309,7 +309,7 @@ static inline struct tracepoint
> > *tracepoint_ptr_deref(tracepoint_ptr_t *p)
> > rcu_dereference_raw((&__tracepoint_##_name)->funcs); \
> > if (it_func_ptr) { \
> > do { \
> > - it_func = (it_func_ptr)->func; \
> > + it_func = READ_ONCE((it_func_ptr)->func); \
> > __data = (it_func_ptr)->data; \
> > ((void(*)(void *, proto))(it_func))(__data, args); \
> > } while ((++it_func_ptr)->func); \
> > diff --git a/kernel/tracepoint.c b/kernel/tracepoint.c
> > index e8f20ae29c18..4b9de79bb927 100644
> > --- a/kernel/tracepoint.c
> > +++ b/kernel/tracepoint.c
> > @@ -136,9 +136,9 @@ func_add(struct tracepoint_func **funcs, struct
> > tracepoint_func *tp_func,
> > int prio)
> > {
> > struct tracepoint_func *old, *new;
> > - int nr_probes = 0;
> > - int stub_funcs = 0;
> > - int pos = -1;
> > + int iter_probes; /* Iterate over old probe array. */
> > + int nr_probes = 0; /* Counter for probes */
> > + int pos = -1; /* New position */
>
> New position -> Insertion position into new array
OK.
>
> >
> > if (WARN_ON(!tp_func->func))
> > return ERR_PTR(-EINVAL);
> > @@ -147,54 +147,39 @@ func_add(struct tracepoint_func **funcs, struct
> > tracepoint_func *tp_func,
> > old = *funcs;
> > if (old) {
> > /* (N -> N+1), (N != 0, 1) probes */
> > - for (nr_probes = 0; old[nr_probes].func; nr_probes++) {
> > - /* Insert before probes of lower priority */
> > - if (pos < 0 && old[nr_probes].prio < prio)
> > - pos = nr_probes;
> > - if (old[nr_probes].func == tp_func->func &&
> > - old[nr_probes].data == tp_func->data)
> > + for (iter_probes = 0; old[iter_probes].func; iter_probes++) {
> > + if (old[iter_probes].func == tp_stub_func)
> > + continue; /* Skip stub functions. */
> > + if (old[iter_probes].func == tp_func->func &&
> > + old[iter_probes].data == tp_func->data)
> > return ERR_PTR(-EEXIST);
> > - if (old[nr_probes].func == tp_stub_func)
> > - stub_funcs++;
> > + nr_probes++;
> > }
> > }
> > - /* + 2 : one for new probe, one for NULL func - stub functions */
> > - new = allocate_probes(nr_probes + 2 - stub_funcs);
> > + /* + 2 : one for new probe, one for NULL func */
> > + new = allocate_probes(nr_probes + 2);
> > if (new == NULL)
> > return ERR_PTR(-ENOMEM);
> > if (old) {
> > - if (stub_funcs) {
> > - /* Need to copy one at a time to remove stubs */
> > - int probes = 0;
> > -
> > - pos = -1;
> > - for (nr_probes = 0; old[nr_probes].func; nr_probes++) {
> > - if (old[nr_probes].func == tp_stub_func)
> > - continue;
> > - if (pos < 0 && old[nr_probes].prio < prio)
> > - pos = probes++;
> > - new[probes++] = old[nr_probes];
> > - }
> > - nr_probes = probes;
> > - if (pos < 0)
> > - pos = probes;
> > - else
> > - nr_probes--; /* Account for insertion */
> > -
> > - } else if (pos < 0) {
> > - pos = nr_probes;
> > - memcpy(new, old, nr_probes * sizeof(struct tracepoint_func));
> > - } else {
> > - /* Copy higher priority probes ahead of the new probe */
> > - memcpy(new, old, pos * sizeof(struct tracepoint_func));
> > - /* Copy the rest after it. */
> > - memcpy(new + pos + 1, old + pos,
> > - (nr_probes - pos) * sizeof(struct tracepoint_func));
> > + pos = -1;
>
> pos is already initialized to -1 at function beginning.
Ah. I noticed near the end of developing this, that we were calculating
"pos" twice. Once in the search for stub functions, and again later, where
the above assignment was necessary. I then realized that finding pos the
first time wasn't necessary and removed it, but didn't remove this second
initialization of pos.
Will remove it in v2.
>
> > + nr_probes = 0;
> > + for (iter_probes = 0; old[iter_probes].func; iter_probes++) {
> > + if (old[iter_probes].func == tp_stub_func)
> > + continue;
> > + /* Insert before probes of lower priority */
> > + if (pos < 0 && old[iter_probes].prio < prio)
> > + pos = nr_probes++;
> > + new[nr_probes++] = old[iter_probes];
> > }
> > - } else
> > + if (pos < 0)
> > + pos = nr_probes++;
> > + /* nr_probes now points to the end of the new array */
> > + } else {
> > pos = 0;
> > + nr_probes = 1; /* must point at end of array */
>
> Yep, much nicer.
>
> > + }
> > new[pos] = *tp_func;
> > - new[nr_probes + 1].func = NULL;
> > + new[nr_probes].func = NULL;
> > *funcs = new;
> > debug_print_probes(*funcs);
> > return old;
> > @@ -237,11 +222,12 @@ static void *func_remove(struct tracepoint_func **funcs,
> > /* + 1 for NULL */
> > new = allocate_probes(nr_probes - nr_del + 1);
> > if (new) {
> > - for (i = 0; old[i].func; i++)
> > - if ((old[i].func != tp_func->func
> > - || old[i].data != tp_func->data)
> > - && old[i].func != tp_stub_func)
> > + for (i = 0; old[i].func; i++) {
> > + if ((old[i].func != tp_func->func ||
> > + old[i].data != tp_func->data) &&
> > + old[i].func != tp_stub_func)
> > new[j++] = old[i];
> > + }
> > new[nr_probes - nr_del].func = NULL;
> > *funcs = new;
> > } else {
> > @@ -249,17 +235,11 @@ static void *func_remove(struct tracepoint_func **funcs,
> > * Failed to allocate, replace the old function
> > * with calls to tp_stub_func.
> > */
> > - for (i = 0; old[i].func; i++)
> > + for (i = 0; old[i].func; i++) {
> > if (old[i].func == tp_func->func &&
> > - old[i].data == tp_func->data) {
> > - old[i].func = tp_stub_func;
> > - /* Set the prio to the next event. */
> > - if (old[i + 1].func)
> > - old[i].prio =
> > - old[i + 1].prio;
> > - else
> > - old[i].prio = -1;
> > - }
> > + old[i].data == tp_func->data)
> > + WRITE_ONCE(old[i].func, tp_stub_func);
> > + }
>
> The rest looks good, thanks!
>
> You can keep my signed-off-by, and if needed may add my reviewed-by tag
> as well. ;-)
>
I'll send a v2. Thanks for looking at it.
-- Steve
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-02-04 19:23 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-04 18:27 [PATCH] tracepoints: Code clean up Steven Rostedt
2021-02-04 19:13 ` Mathieu Desnoyers
2021-02-04 19:21 ` Steven Rostedt
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.