Linux-Trace-Devel Archive on lore.kernel.org
 help / color / Atom feed
From: Steven Rostedt <rostedt@goodmis.org>
To: Tzvetomir Stoyanov <tz.stoyanov@gmail.com>
Cc: Linux Trace Devel <linux-trace-devel@vger.kernel.org>,
	Sameeruddin shaik <sameeruddin.shaik8@gmail.com>
Subject: Re: [PATCH 13/13 v2] libtracefs: Add CONTINUE to tracefs_function_filter()
Date: Tue, 30 Mar 2021 10:52:50 -0400
Message-ID: <20210330105250.0a7105a1@gandalf.local.home> (raw)
In-Reply-To: <CAPpZLN57JrvSNTCGJTe7A9_x9Z4rSsH54jQu5C8wqodii=np4w@mail.gmail.com>

On Tue, 30 Mar 2021 17:29:57 +0300
Tzvetomir Stoyanov <tz.stoyanov@gmail.com> wrote:

> On Tue, Mar 30, 2021 at 3:54 AM Steven Rostedt <rostedt@goodmis.org> wrote:
> >
> > From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
> >
> > Add the TRACEFS_FL_CONTINUE flag to the tracefs_function_filter() API that
> > will allow it to return without closing the set_ftrace_filter file. When
> > the set_ftrace_filter file is closed, all the changes to it take place,
> > but not before hand. In the case that multiple modules need to be set in
> > one activation, the tracefs_function_filter() would need to be called
> > multiple times without closing the file descriptor.
> >
> > Note, the next call to tracefs_function_filter() after it was called with
> > the CONTINUE flag set, the RESET flag is ignored, as the RESET flag only
> > takes effect on opening the file. The next call to
> > tracefs_function_filter() after it was called with the CONTINUE flag (on
> > the same instance) does not reopen the file, and thus will not reset the
> > file.  
> 
> I think it is better to not maintain a state in the API context. Let's
> make it simple and allow the user to take care of the calling order
> and flags.
>    If RESET is set - close the file (if it is opened already) and open
> it with the TRUNC flag.

In the use cases I've played with, it was simple to do:

	for (i = 0; filters[i]; i++)
		tracefs_function_filter(instance, filters[i], module,
				TRACEFS_FL_RESET | TRACEFS_FL_CONTINUE);

And not worry about keeping track of the RESET flag. This is the reason I
did it this way. Otherwise I would need to be:

	int reset = TRACEFS_FL_RESET;
	for (i = 0; filters[i]; i++) {
		tracefs_function_filter(instance, filters[i], module,
				reset | TRACEFS_FL_CONTINUE);
		reset = 0;
	}

Or something else. But I'm looking to simplify the most common case.

And closing the file can cause issues. If we were to go this way, I would
make it that if the file is open and RESET is set, it would return an error
EBUSY.

But no, closing the file just because RESET is set is dangerous.

>    If CONTINUE is set - do not close the file.
> Also, allow calling the API with no filters and any combination of
> flags, just to have resting and committing currently written filters.
> For example:
>  tracefs_function_filter(NULL, NULL, NULL, TRACEFS_FL_RESET, NULL); //
> Close the file (if it is opened already), open it with the TRUNC flag
> and close it.
>  tracefs_function_filter(NULL, NULL, NULL, TRACEFS_FL_CONTINUE, NULL);
> //  Open the file with APPEND (if it is not opened already) and do not
> close it.
>  tracefs_function_filter(NULL, NULL, NULL, TRACEFS_FL_RESET |
> TRACEFS_FL_CONTINUE, NULL);  // Close the file (if it is opened
> already), open it with the TRUNC flag and do not close it.

Yes, I had already planned on adding a patch to allow NULL filter when
these flags are set. I just didn't get there yet.

-- Steve

> 
> 
> >
> > If the file is opened, calling tracefs_function_filter() with no filters
> > and the continue flag not set, will simply close the set_ftrace_filter
> > file.
> >
> > Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> > ---

  reply index

Thread overview: 27+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-03-30  0:51 [PATCH 00/13 v2] libtracefs: Add tracefs_function_filter() Steven Rostedt
2021-03-30  0:51 ` [PATCH 01/13 v2] libtracefs: An API to set the filtering of functions Steven Rostedt
2021-03-30  0:51 ` [PATCH 02/13 v2] libtracefs: Document function_filter API Steven Rostedt
2021-03-30  0:51 ` [PATCH 03/13 v2] libtracefs: Fix notations of tracefs_function_filter() and module parameter Steven Rostedt
2021-03-30  0:51 ` [PATCH 04/13 v2] libtracefs: Move opening of file out of controlled_write() Steven Rostedt
2021-03-30  0:51 ` [PATCH 05/13 v2] libtracefs: Add add_errors() helper function for control_write() Steven Rostedt
2021-03-30  0:51 ` [PATCH 06/13 v2] libtracefs: Add checking of available_filter_functions to tracefs_function_filter() Steven Rostedt
2021-03-30  0:51 ` [PATCH 07/13 v2] libtracefs: Add write_filter() helper function Steven Rostedt
2021-03-30  0:51 ` [PATCH 08/13 v2] libtracefs: Allow for setting filters with regex expressions Steven Rostedt
2021-04-01 16:33   ` sameeruddin shaik
2021-03-31 16:39     ` Steven Rostedt
2021-04-02  1:59       ` sameeruddin shaik
2021-04-01  2:41         ` Steven Rostedt
2021-03-30  0:51 ` [PATCH 09/13 v2] libtracefs: Add indexing to set functions in tracefs_function_filter() Steven Rostedt
2021-03-30  0:51 ` [PATCH 10/13 v2] libtracefs: Pass in reset via flags to tracefs_function_filter() Steven Rostedt
2021-03-30 14:29   ` Tzvetomir Stoyanov
2021-03-30 14:53     ` Steven Rostedt
2021-03-30  0:51 ` [PATCH 11/13 v2] libtracefs: Add pthread_mutex_lock() around tracefs_function_filter() Steven Rostedt
2021-03-30  0:51 ` [PATCH 12/13 v2] libtracefs: Move struct tracefs_instance to tracefs-local.h Steven Rostedt
2021-03-30  0:51 ` [PATCH 13/13 v2] libtracefs: Add CONTINUE to tracefs_function_filter() Steven Rostedt
2021-03-30 14:29   ` Tzvetomir Stoyanov
2021-03-30 14:52     ` Steven Rostedt [this message]
2021-03-30 15:14       ` Steven Rostedt
2021-03-30 15:32       ` Tzvetomir Stoyanov
2021-03-30 16:03         ` Steven Rostedt
2021-03-30  1:03 ` [RFC][PATCH 14/13 v2] libtracefs: Just past one filter in for tracefs_function_filter() Steven Rostedt
2021-03-30 14:31   ` Tzvetomir Stoyanov

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=20210330105250.0a7105a1@gandalf.local.home \
    --to=rostedt@goodmis.org \
    --cc=linux-trace-devel@vger.kernel.org \
    --cc=sameeruddin.shaik8@gmail.com \
    --cc=tz.stoyanov@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