Received-SPF: pass (google.com: domain of linux-kernel+bounces-198073-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Sun, 2 Jun 2024 11:40:32 +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, 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 10/20] function_graph: Have the instances use their own
 ftrace_ops for filtering
Message-Id: <20240602114032.fefbbbfdc8e743b3a148a919@kernel.org>
In-Reply-To: <20240531184910.799635e8@rorschach.local.home>
References: <20240525023652.903909489@goodmis.org>
	<20240525023742.786834257@goodmis.org>
	<20240530223057.21c2a779@rorschach.local.home>
	<20240531121241.c586189caad8d31d597f614d@kernel.org>
	<20240531020346.6c13e2d4@rorschach.local.home>
	<20240531235023.a0b2b207362eba2f8b5c16f7@kernel.org>
	<20240531184910.799635e8@rorschach.local.home>
Precedence: bulk
Mime-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit

On Fri, 31 May 2024 18:49:10 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Fri, 31 May 2024 23:50:23 +0900
> Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote:
> 
> > So is it similar to the fprobe/kprobe, use shared signle ftrace_ops,
> > but keep each fgraph has own hash table?
> 
> Sort of.
> 
> I created helper functions in ftrace that lets you have a "manager
> ftrace_ops" that will be used to assign to ftrace (with the function
> that will demultiplex), and then you can have "subops" that can be
> assigned that is per user. Here's a glimpse of the code:
> 
> /**
>  * ftrace_startup_subops - enable tracing for subops of an ops
>  * @ops: Manager ops (used to pick all the functions of its subops)
>  * @subops: A new ops to add to @ops
>  * @command: Extra commands to use to enable tracing
>  *
>  * The @ops is a manager @ops that has the filter that includes all the functions
>  * that its list of subops are tracing. Adding a new @subops will add the
>  * functions of @subops to @ops.
>  */
> int ftrace_startup_subops(struct ftrace_ops *ops, struct ftrace_ops *subops, int command)
> {
> 	struct ftrace_hash *filter_hash;
> 	struct ftrace_hash *notrace_hash;
> 	struct ftrace_hash *save_filter_hash;
> 	struct ftrace_hash *save_notrace_hash;
> 	int size_bits;
> 	int ret;
> 
> 	if (unlikely(ftrace_disabled))
> 		return -ENODEV;
> 
> 	ftrace_ops_init(ops);
> 	ftrace_ops_init(subops);
> 
> 	/* Make everything canonical (Just in case!) */
> 	if (!ops->func_hash->filter_hash)
> 		ops->func_hash->filter_hash = EMPTY_HASH;
> 	if (!ops->func_hash->notrace_hash)
> 		ops->func_hash->notrace_hash = EMPTY_HASH;
> 	if (!subops->func_hash->filter_hash)
> 		subops->func_hash->filter_hash = EMPTY_HASH;
> 	if (!subops->func_hash->notrace_hash)
> 		subops->func_hash->notrace_hash = EMPTY_HASH;
> 
> 	/* For the first subops to ops just enable it normally */
> 	if (list_empty(&ops->subop_list)) {

May above ftrace_ops_init() clear this list up always?

> 		/* Just use the subops hashes */
> 		filter_hash = copy_hash(subops->func_hash->filter_hash);
> 		notrace_hash = copy_hash(subops->func_hash->notrace_hash);
> 		if (!filter_hash || !notrace_hash) {
> 			free_ftrace_hash(filter_hash);
> 			free_ftrace_hash(notrace_hash);
> 			return -ENOMEM;
> 		}
> 
> 		save_filter_hash = ops->func_hash->filter_hash;
> 		save_notrace_hash = ops->func_hash->notrace_hash;
> 
> 		ops->func_hash->filter_hash = filter_hash;
> 		ops->func_hash->notrace_hash = notrace_hash;
> 		list_add(&subops->list, &ops->subop_list);
> 		ret = ftrace_startup(ops, command);
> 		if (ret < 0) {
> 			list_del(&subops->list);
> 			ops->func_hash->filter_hash = save_filter_hash;
> 			ops->func_hash->notrace_hash = save_notrace_hash;
> 			free_ftrace_hash(filter_hash);
> 			free_ftrace_hash(notrace_hash);
> 		} else {
> 			free_ftrace_hash(save_filter_hash);
> 			free_ftrace_hash(save_notrace_hash);
> 			subops->flags |= FTRACE_OPS_FL_ENABLED;
> 		}
> 		return ret;
> 	}
> 
> 	/*
> 	 * Here there's already something attached. Here are the rules:
> 	 *   o If either filter_hash is empty then the final stays empty
> 	 *      o Otherwise, the final is a superset of both hashes
> 	 *   o If either notrace_hash is empty then the final stays empty
> 	 *      o Otherwise, the final is an intersection between the hashes

Yeah, filter_hash |= subops_filter_hash and notrace_hash &= subops_notrace_hash.
The complicated point is filter's EMPTY_HASH means FULLSET_HASH. :)

> 	 */
> 	if (ops->func_hash->filter_hash == EMPTY_HASH ||
> 	    subops->func_hash->filter_hash == EMPTY_HASH) {
> 		filter_hash = EMPTY_HASH;
> 	} else {
> 		size_bits = max(ops->func_hash->filter_hash->size_bits,
> 				subops->func_hash->filter_hash->size_bits);

Don't we need to expand the size_bits? In the worst case, both hash does not
share any entry, then it should be expanded.

> 		filter_hash = alloc_and_copy_ftrace_hash(size_bits, ops->func_hash->filter_hash);
> 		if (!filter_hash)
> 			return -ENOMEM;
> 		ret = append_hash(&filter_hash, subops->func_hash->filter_hash);
> 		if (ret < 0) {
> 			free_ftrace_hash(filter_hash);
> 			return ret;
> 		}
> 	}
> 
> 	if (ops->func_hash->notrace_hash == EMPTY_HASH ||
> 	    subops->func_hash->notrace_hash == EMPTY_HASH) {
> 		notrace_hash = EMPTY_HASH;
> 	} else {
> 		size_bits = max(ops->func_hash->filter_hash->size_bits,
> 				subops->func_hash->filter_hash->size_bits);
> 		notrace_hash = alloc_ftrace_hash(size_bits);
> 		if (!notrace_hash) {
> 			free_ftrace_hash(filter_hash);
> 			return -ENOMEM;
> 		}
> 
> 		ret = intersect_hash(&notrace_hash, ops->func_hash->filter_hash,
> 				     subops->func_hash->filter_hash);
> 		if (ret < 0) {
> 			free_ftrace_hash(filter_hash);
> 			free_ftrace_hash(notrace_hash);
> 			return ret;
> 		}
> 	}
> 
> 	list_add(&subops->list, &ops->subop_list);
> 
> 	ret = ftrace_update_ops(ops, filter_hash, notrace_hash);
> 	free_ftrace_hash(filter_hash);
> 	free_ftrace_hash(notrace_hash);
> 	if (ret < 0)
> 		list_del(&subops->list);
> 	return ret;
> }
> 
> /**
>  * ftrace_shutdown_subops - Remove a subops from a manager ops
>  * @ops: A manager ops to remove @subops from
>  * @subops: The subops to remove from @ops
>  * @command: Any extra command flags to add to modifying the text
>  *
>  * Removes the functions being traced by the @subops from @ops. Note, it
>  * will not affect functions that are being traced by other subops that
>  * still exist in @ops.
>  *
>  * If the last subops is removed from @ops, then @ops is shutdown normally.
>  */
> int ftrace_shutdown_subops(struct ftrace_ops *ops, struct ftrace_ops *subops, int command)
> {
> 	struct ftrace_hash *filter_hash;
> 	struct ftrace_hash *notrace_hash;
> 	int ret;
> 
> 	if (unlikely(ftrace_disabled))
> 		return -ENODEV;
> 
> 	list_del(&subops->list);
> 
> 	if (list_empty(&ops->subop_list)) {
> 		/* Last one, just disable the current ops */
> 
> 		ret = ftrace_shutdown(ops, command);
> 		if (ret < 0) {
> 			list_add(&subops->list, &ops->subop_list);
> 			return ret;
> 		}
> 
> 		subops->flags &= ~FTRACE_OPS_FL_ENABLED;
> 
> 		free_ftrace_hash(ops->func_hash->filter_hash);
> 		free_ftrace_hash(ops->func_hash->notrace_hash);
> 		ops->func_hash->filter_hash = EMPTY_HASH;
> 		ops->func_hash->notrace_hash = EMPTY_HASH;
> 
> 		return 0;
> 	}
> 
> 	/* Rebuild the hashes without subops */
> 	filter_hash = append_hashes(ops);
> 	notrace_hash = intersect_hashes(ops);
> 	if (!filter_hash || !notrace_hash) {
> 		free_ftrace_hash(filter_hash);
> 		free_ftrace_hash(notrace_hash);
> 		list_add(&subops->list, &ops->subop_list);
> 		return -ENOMEM;
> 	}
> 
> 	ret = ftrace_update_ops(ops, filter_hash, notrace_hash);
> 	if (ret < 0)
> 		list_add(&subops->list, &ops->subop_list);
> 	free_ftrace_hash(filter_hash);
> 	free_ftrace_hash(notrace_hash);
> 	return ret;
> }

OK, so if the list_is_singlar(ops->subop_list), ftrace_graph_enter_ops() is
called and if not, ftrace_graph_enter() is called, right?

Thank you,

> 
> 
> > 
> > > This removes the need to touch the architecture code. It can also be
> > > used by fprobes to handle the attachments to functions for several
> > > different sets of callbacks.
> > > 
> > > I'll send out patches soon.  
> > 
> > OK, I'll wait for that.
> 
> I'm just cleaning it up. I'll post it tomorrow (your today).
> 
> -- Steve


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