Linux-Trace-Devel Archive on lore.kernel.org
 help / color / Atom feed
From: Steven Rostedt <rostedt@goodmis.org>
To: linux-trace-devel@vger.kernel.org
Cc: Sameeruddin shaik <sameeruddin.shaik8@gmail.com>
Subject: Re: [PATCH 0/7] libtracfes: Add tracefs_function_filter()
Date: Tue, 23 Mar 2021 08:52:20 -0400
Message-ID: <20210323085220.6e20d125@gandalf.local.home> (raw)
In-Reply-To: <20210323012755.155237800@goodmis.org>

On Mon, 22 Mar 2021 21:27:55 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> This adds a new API tracefs_function_filter() as described in:
> 
>  https://bugzilla.kernel.org/show_bug.cgi?id=210643
> 
> It will use regular expressions against available_filter_functions (or even
> the kernel glob expression) to enable the function by the index method if it
> is supported. If it is not supported, it will go back to the writing of the
> filter strings directly into the set_ftrace_filter file.
> 
> 

Playing with the interface some more, I feel it's not quite adequate.

The returning the negative number of filters that failed, isn't very
useful. Having the errs array that points to those filters gives us that
information.

Also, because of the way that file works in the kernel, we need to be able
to call this function several times without closing the file. That's
because the actions take place when the file is closed, *not* when a write
is made. Having the interface return errors without closing the file would
allow the user to fix the failed filters and try again.

I plan on committing this patch series, so any new changes will be done on
top of them. But here's what I think needs to be done to make the interface
more usable.

 - Create a tracefs_function_filter_commit(instance) API that will close
   the file and commit the changes.

 - Have the tracefs_function_filter() open the set_ftrace_filter file if it
   is not already opened. This will allow for the function to be called
   multiple times. The file descriptor will be part of the instance
   descriptor or a global variable for the top level instance. The opening
   of the files will need to be protected by a pthread mutex.
   Note, the "reset" parameter is only applicable if the file is not
   already opened.

 - On success, return 0 and tracefs_function_filter_commit() must be called.

 - If the file descriptor is opened and an error happens, return 1, and the
   tracefs_function_filter_commit() still needs to be called, but the user
   can call tracefs_function_filter() again to try to fix the problem.

 - If an error is found before the file descriptor is opened, then
   tracefs_function_filter_commit() does not need to be called.

That is:

	ret = tracefs_function_filter(...);
	if (ret >= 0)
		tracefs_function_filter_commit();

 - Now for errs, it will be allocated if a problem happened on a filter,
   and may be set if the return value is non zero (1 or -1). The user can
   use it to know if the problem happened on a filter as well as find out
   which filter had the problem.

 - We can have a tracefs_read_function_filter() that returns an array of
   strings which ends with a NULL pointer (and needs to be freed with
   tracefs_list_free(), and have this:

   save_filters = tracefs_read_function_filter(instance);

   ret = tracefs_function_filter(instance, filters, NULL, true, &errs);
   if (ret > 0 && errs) {
	/* Modified but failed */
	int i, j;
	for (i = 0; filters[i]; i++)
		;
	for (j = 0; errs[i]; j++)
		;
	if (i == j)
		/* all filters failed! Put back the original */
		tracefs_function_filter(instance, save_filters, NULL, false, NULL);
   }
   if (ret >= 0)
	tracefs_function_filter_commit(instance);

Another reason to have it not close the file is because we only support
passing in one module at a time. If the user wants to enable functions in
two modules, they would need to call this twice, and we want them to be
able to do so and have both changes take affect at the same time.

Thoughts?

-- Steve

  parent reply index

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-03-23  1:27 Steven Rostedt
2021-03-23  1:27 ` [PATCH 1/7] libtracefs: An API to set the filtering of functions Steven Rostedt
2021-03-23  1:27 ` [PATCH 2/7] libtracefs: Move opening of file out of controlled_write() Steven Rostedt
2021-03-23  1:27 ` [PATCH 3/7] libtracefs: Add add_errors() helper function for control_write() Steven Rostedt
2021-03-23  1:27 ` [PATCH 4/7] libtracefs: Add checking of available_filter_functions to tracefs_function_filter() Steven Rostedt
2021-03-23  1:28 ` [PATCH 5/7] libtracefs: Add write_filter() helper function Steven Rostedt
2021-03-23  1:28 ` [PATCH 6/7] libtracefs: Allow for setting filters with regex expressions Steven Rostedt
2021-03-23  1:28 ` [PATCH 7/7] libtracefs: Add indexing to set functions in tracefs_function_filter() Steven Rostedt
2021-03-23 12:52 ` Steven Rostedt [this message]
2021-03-24 10:13   ` [PATCH 0/7] libtracfes: Add tracefs_function_filter() Sameeruddin Shaik
2021-03-24 14:08     ` Steven Rostedt
2021-03-26  0:25   ` sameeruddin shaik
2021-03-25  0:35     ` Steven Rostedt

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210323085220.6e20d125@gandalf.local.home \
    --to=rostedt@goodmis.org \
    --cc=linux-trace-devel@vger.kernel.org \
    --cc=sameeruddin.shaik8@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-Trace-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-trace-devel/0 linux-trace-devel/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-trace-devel linux-trace-devel/ https://lore.kernel.org/linux-trace-devel \
		linux-trace-devel@vger.kernel.org
	public-inbox-index linux-trace-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-trace-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git