Received-SPF: pass (google.com: domain of linux-kernel+bounces-198531-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Date: Mon, 3 Jun 2024 10:33:16 +0900
From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
To: Steven Rostedt <rostedt@goodmis.org>
Cc: linux-kernel@vger.kernel.org, linux-trace-kernel@vger.kernel.org, Masami
 Hiramatsu <mhiramat@kernel.org>, Mark Rutland <mark.rutland@arm.com>,
 Mathieu Desnoyers <mathieu.desnoyers@efficios.com>, Andrew Morton
 <akpm@linux-foundation.org>, Alexei Starovoitov
 <alexei.starovoitov@gmail.com>, Florent Revest <revest@chromium.org>,
 Martin KaFai Lau <martin.lau@linux.dev>, bpf <bpf@vger.kernel.org>, Sven
 Schnelle <svens@linux.ibm.com>, Alexei Starovoitov <ast@kernel.org>, Jiri
 Olsa <jolsa@kernel.org>, Arnaldo Carvalho de Melo <acme@kernel.org>, Daniel
 Borkmann <daniel@iogearbox.net>, Alan Maguire <alan.maguire@oracle.com>,
 Peter Zijlstra <peterz@infradead.org>, Thomas Gleixner
 <tglx@linutronix.de>, Guo Ren <guoren@kernel.org>
Subject: Re: [PATCH v2 10/27] ftrace: Add subops logic to allow one ops to
 manage many
Message-Id: <20240603103316.3af9dea3214a5d2bde721cd8@kernel.org>
In-Reply-To: <20240602033832.709653366@goodmis.org>
References: <20240602033744.563858532@goodmis.org>
	<20240602033832.709653366@goodmis.org>
Precedence: bulk
Mime-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit

Hi Steve,

On Sat, 01 Jun 2024 23:37:54 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>

I think this is a new patch, correct? I'm a bit confused.

And I have some comments below;
[..]
> @@ -3164,6 +3166,392 @@ int ftrace_shutdown(struct ftrace_ops *ops, int command)
>  	return 0;
>  }
>  
> +/* Simply make a copy of @src and return it */
> +static struct ftrace_hash *copy_hash(struct ftrace_hash *src)
> +{
> +	if (!src || src == EMPTY_HASH)
> +		return EMPTY_HASH;
> +
> +	return alloc_and_copy_ftrace_hash(src->size_bits, src);
> +}
> +
> +/*
> + * Append @new_hash entries to @hash:
> + *
> + *  If @hash is the EMPTY_HASH then it traces all functions and nothing
> + *  needs to be done.
> + *
> + *  If @new_hash is the EMPTY_HASH, then make *hash the EMPTY_HASH so
> + *  that it traces everything.

This lacks the most important comment, this function is only for
filter_hash, not for notrace_hash. :)

> + *
> + *  Otherwise, go through all of @new_hash and add anything that @hash
> + *  doesn't already have, to @hash.
> + */
> +static int append_hash(struct ftrace_hash **hash, struct ftrace_hash *new_hash)
> +{
> +	struct ftrace_func_entry *entry;
> +	int size;
> +	int i;
> +
> +	/* An empty hash does everything */
> +	if (!*hash || *hash == EMPTY_HASH)
> +		return 0;
> +
> +	/* If new_hash has everything make hash have everything */
> +	if (!new_hash || new_hash == EMPTY_HASH) {
> +		free_ftrace_hash(*hash);
> +		*hash = EMPTY_HASH;
> +		return 0;
> +	}
> +
> +	size = 1 << new_hash->size_bits;
> +	for (i = 0; i < size; i++) {
> +		hlist_for_each_entry(entry, &new_hash->buckets[i], hlist) {
> +			/* Only add if not already in hash */
> +			if (!__ftrace_lookup_ip(*hash, entry->ip) &&
> +			    add_hash_entry(*hash, entry->ip) == NULL)
> +				return -ENOMEM;
> +		}
> +	}
> +	return 0;
> +}
> +
> +/* Add to @hash only those that are in both @new_hash1 and @new_hash2 */

Ditto, this is only for the notrace_hash.

> +static int intersect_hash(struct ftrace_hash **hash, struct ftrace_hash *new_hash1,
> +			  struct ftrace_hash *new_hash2)
> +{
> +	struct ftrace_func_entry *entry;
> +	int size;
> +	int i;
> +
> +	/*
> +	 * If new_hash1 or new_hash2 is the EMPTY_HASH then make the hash
> +	 * empty as well as empty for notrace means none are notraced.
> +	 */
> +	if (!new_hash1 || new_hash1 == EMPTY_HASH ||
> +	    !new_hash2 || new_hash2 == EMPTY_HASH) {
> +		free_ftrace_hash(*hash);
> +		*hash = EMPTY_HASH;
> +		return 0;
> +	}
> +
> +	size = 1 << new_hash1->size_bits;
> +	for (i = 0; i < size; i++) {
> +		hlist_for_each_entry(entry, &new_hash1->buckets[i], hlist) {
> +			/* Only add if in both @new_hash1 and @new_hash2 */
> +			if (__ftrace_lookup_ip(new_hash2, entry->ip) &&
> +			    add_hash_entry(*hash, entry->ip) == NULL)
> +				return -ENOMEM;
> +		}
> +	}
> +	return 0;
> +}
> +
> +/* Return a new hash that has a union of all @ops->filter_hash entries */
> +static struct ftrace_hash *append_hashes(struct ftrace_ops *ops)
> +{
> +	struct ftrace_hash *new_hash;
> +	struct ftrace_ops *subops;
> +	int ret;
> +
> +	new_hash = alloc_ftrace_hash(ops->func_hash->filter_hash->size_bits);
> +	if (!new_hash)
> +		return NULL;
> +
> +	list_for_each_entry(subops, &ops->subop_list, list) {
> +		ret = append_hash(&new_hash, subops->func_hash->filter_hash);
> +		if (ret < 0) {
> +			free_ftrace_hash(new_hash);
> +			return NULL;
> +		}
> +		/* Nothing more to do if new_hash is empty */
> +		if (new_hash == EMPTY_HASH)
> +			break;
> +	}
> +	return new_hash;
> +}
> +
> +/* Make @ops trace evenything except what all its subops do not trace */
> +static struct ftrace_hash *intersect_hashes(struct ftrace_ops *ops)
> +{
> +	struct ftrace_hash *new_hash = NULL;
> +	struct ftrace_ops *subops;
> +	int size_bits;
> +	int ret;
> +
> +	list_for_each_entry(subops, &ops->subop_list, list) {
> +		struct ftrace_hash *next_hash;
> +
> +		if (!new_hash) {
> +			size_bits = subops->func_hash->notrace_hash->size_bits;
> +			new_hash = alloc_and_copy_ftrace_hash(size_bits, ops->func_hash->notrace_hash);
> +			if (!new_hash)
> +				return NULL;

If the first subops has EMPTY_HASH, this allocates small empty hash (!= EMPTY_HASH)
on `new_hash`.

> +			continue;
> +		}
> +		size_bits = new_hash->size_bits;
> +		next_hash = new_hash;

And it is assigned to `next_hash`.

> +		new_hash = alloc_ftrace_hash(size_bits);
> +		ret = intersect_hash(&new_hash, next_hash, subops->func_hash->notrace_hash);

Since the `next_hash` != EMPTY_HASH but it is empty, this keeps `new_hash`
empty but allocated.

> +		free_ftrace_hash(next_hash);
> +		if (ret < 0) {
> +			free_ftrace_hash(new_hash);
> +			return NULL;
> +		}
> +		/* Nothing more to do if new_hash is empty */
> +		if (new_hash == EMPTY_HASH)

Since `new_hash` is empty but != EMPTY_HASH, this does not pass. Keep looping on.

> +			break;
> +	}
> +	return new_hash;

And this will return empty but not EMPTY_HASH hash.


So, we need;

#define FTRACE_EMPTY_HASH_OR_NULL(hash)	(!(hash) || (hash) == EMPTY_HASH)

if (FTRACE_EMPTY_HASH_OR_NULL(subops->func_hash->notrace_hash)) {
	free_ftrace_hash(new_hash);
	new_hash = EMPTY_HASH;
	break;
}

at the beginning of the loop.
Also, at the end of the loop,

if (ftrace_hash_empty(new_hash)) {
	free_ftrace_hash(new_hash);
	new_hash = EMPTY_HASH;
	break;
}

> +}
> +
> +/* Returns 0 on equal or non-zero on non-equal */
> +static int compare_ops(struct ftrace_hash *A, struct ftrace_hash *B)

nit: Isn't it better to be `bool hash_equal()` and return true if A == B ?

Thank you,

> +{
> +	struct ftrace_func_entry *entry;
> +	int size;
> +	int i;
> +
> +	if (!A || A == EMPTY_HASH)
> +		return !(!B || B == EMPTY_HASH);
> +
> +	if (!B || B == EMPTY_HASH)
> +		return !(!A || A == EMPTY_HASH);
> +
> +	if (A->count != B->count)
> +		return 1;
> +
> +	size = 1 << A->size_bits;
> +	for (i = 0; i < size; i++) {
> +		hlist_for_each_entry(entry, &A->buckets[i], hlist) {
> +			if (!__ftrace_lookup_ip(B, entry->ip))
> +				return 1;
> +		}
> +	}
> +
> +	return 0;
> +}
> +


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>