Received-SPF: pass (google.com: domain of linux-kernel+bounces-198541-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Sun, 2 Jun 2024 22:06:13 -0400
From: Steven Rostedt <rostedt@goodmis.org>
To: "Masami Hiramatsu (Google)" <mhiramat@kernel.org>
Cc: linux-kernel@vger.kernel.org, linux-trace-kernel@vger.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: <20240602220613.3f9eac04@gandalf.local.home>
In-Reply-To: <20240603103316.3af9dea3214a5d2bde721cd8@kernel.org>
References: <20240602033744.563858532@goodmis.org>
	<20240602033832.709653366@goodmis.org>
	<20240603103316.3af9dea3214a5d2bde721cd8@kernel.org>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit

On Mon, 3 Jun 2024 10:33:16 +0900
Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote:

> 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.

Ah, good catch!

I originally started writing this code and updating an old commit (one that
I did at VMware), and then split it out. But that keeps the original
authorship (rebase, copy the commit twice, and fix it up).

Will fix.

> 
> 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. :)

I did purposely leave it out, because it describes what it does. It's not
that it's only for filter_hash and not for notrace_hash, but it's more that
filter_hash only needs this and notrace_hash only needs the intersection
function ;-)

But, sure, to get rid of confusion, I'll add a comment saying such.

> 
> > + *
> > + *  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`.

Could we just change the above to be: ?

			new_hash = ftrace_hash_empty(ops->func_hash->notrace_hash) ? EMPTY_HASH :
				alloc_and_copy_ftrace_hash(size_bits, ops->func_hash->notrace_hash);
			if (!new_hash)
				return NULL;  


> 
> > +			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 ?

Sure. I guess I was thinking too much of strcmp() logic :-p

> 
> Thank you,

Thanks for the review.

-- Steve

> 
> > +{
> > +	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;
> > +}
> > +  
> 
>