Messages in this thread | | | Date | Wed, 27 Jan 2021 12:54:34 -0500 | From | Steven Rostedt <> | Subject | Re: [PATCH v4] tracepoint: Do not fail unregistering a probe due to memory failure |
| |
On Wed, 27 Jan 2021 12:39:51 -0500 Steven Rostedt <rostedt@goodmis.org> wrote: > > kernel/tracepoint.c | 54 +++++++++++++++++++++++++++++++++++---------- > 1 file changed, 42 insertions(+), 12 deletions(-) > > diff --git a/kernel/tracepoint.c b/kernel/tracepoint.c > index 7261fa0f5e3c..23088f6276a4 100644 > --- a/kernel/tracepoint.c > +++ b/kernel/tracepoint.c > @@ -129,7 +129,7 @@ static struct tracepoint_func * > func_add(struct tracepoint_func **funcs, struct tracepoint_func *tp_func, > int prio) > { > - struct tracepoint_func *old, *new; > + struct tracepoint_func *old, *new, *tp_funcs; > int nr_probes = 0; > int pos = -1; > > @@ -149,10 +149,28 @@ func_add(struct tracepoint_func **funcs, struct tracepoint_func *tp_func, > return ERR_PTR(-EEXIST); > } > } > - /* + 2 : one for new probe, one for NULL func */ > - new = allocate_probes(nr_probes + 2); > - if (new == NULL) > + /* > + * The size of the tp_funcs will be the current size, plus > + * one for the new probe, one for the NULL func, and one for > + * the pointer to the "removal" array. > + * And then double the size to create the "removal" array. > + */ > + tp_funcs = allocate_probes((nr_probes + 3) * 2);
Note, I realize that this double allocation is unnecessary if we add a single probe. But to make this different for 2 or more probes, would make the logic more complex, so I just kept the logic the same for the single probe even though it's not needed.
> + if (tp_funcs == NULL) > return ERR_PTR(-ENOMEM); > + /* > + * When removing a probe and there are more probes left, > + * we cannot rely on allocation to succeed to create the new > + * RCU array. Thus, the array is doubled here, and on removal of > + * a probe with other probes still in the array, the second half > + * of the array is used. > + * > + * The first element of the array has its "func" field point to > + * the start of the other half of the array. > + */ > + tp_funcs->func = tp_funcs + nr_probes + 3; > + tp_funcs[nr_probes + 3].func = tp_funcs; > + new = tp_funcs + 1; > if (old) { > if (pos < 0) { > pos = nr_probes; > @@ -164,6 +182,14 @@ func_add(struct tracepoint_func **funcs, struct tracepoint_func *tp_func, > memcpy(new + pos + 1, old + pos, > (nr_probes - pos) * sizeof(struct tracepoint_func)); > } > + /* Make old point back to the allocated array */ > + old--; > + /* > + * If this is the second half of the array, > + * make it point back to the first half. > + */ > + if (old->func < old) > + old = old->func; > } else > pos = 0; > new[pos] = *tp_func; > @@ -202,14 +228,18 @@ static void *func_remove(struct tracepoint_func **funcs, > /* N -> 0, (N > 1) */ > *funcs = NULL; > debug_print_probes(*funcs); > + /* Set old back to what it was allocated to */ > + old--; > + if (old->func < old) > + old = old->func; > return old; > } else { > int j = 0; > - /* N -> M, (N > 1, M > 0) */ > - /* + 1 for NULL */ > - new = allocate_probes(nr_probes - nr_del + 1); > - if (new == NULL) > - return ERR_PTR(-ENOMEM); > + > + /* Use the other half of the array for the new probes */ > + new = old - 1; > + new = new->func; > + new++; > for (i = 0; old[i].func; i++) > if (old[i].func != tp_func->func > || old[i].data != tp_func->data) > @@ -218,7 +248,7 @@ static void *func_remove(struct tracepoint_func **funcs, > *funcs = new; > } > debug_print_probes(*funcs); > - return old; > + return NULL; > } > > static void tracepoint_update_call(struct tracepoint *tp, struct tracepoint_func *tp_funcs, bool sync) > @@ -309,8 +339,8 @@ static int tracepoint_remove_func(struct tracepoint *tp, > rcu_assign_pointer(tp->funcs, tp_funcs); > } else { > rcu_assign_pointer(tp->funcs, tp_funcs); > - tracepoint_update_call(tp, tp_funcs, > - tp_funcs[0].func != old[0].func); > + /* Need to sync, if going back to a single caller */ > + tracepoint_update_call(tp, tp_funcs, tp_funcs[1].func == NULL);
I may make this change a separate patch. As it changes the logic slightly unrelated to the change being fixed, and was only needed for this patch, to remove the reference to "old".
-- Steve
> } > release_probes(old); > return 0;
| |