Re: [PATCH v4] tracepoint: Do not fail unregistering a probe due to memory failure

[Steven Rostedt]

On Wed, 27 Jan 2021 12:39:51 -0500
Steven Rostedt <rostedt@xxxxxxxxxxx> 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;