Linux-Trace-Devel Archive on lore.kernel.org
 help / color / Atom feed
From: Tzvetomir Stoyanov <tz.stoyanov@gmail.com>
To: Steven Rostedt <rostedt@goodmis.org>
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 18:32:25 +0300
Message-ID: <CAPpZLN4cC4Z7eYP=VTj09XRhQ_jaswcgYXa1MEnvs7RJRVBCeA@mail.gmail.com> (raw)
In-Reply-To: <20210330105250.0a7105a1@gandalf.local.home>

On Tue, Mar 30, 2021 at 5:52 PM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> 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);
>

If the user takes care of the state, instead of the library, this use
case could look like:

   tracefs_function_filter(instance, NULL, NULL, TRACEFS_FL_RESET |
TRACEFS_FL_CONTINUE);
/* or add a wrapper tracefs_function_filter_reset() */

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

   tracefs_function_filter(instance, NULL, NULL, 0);
/* or add a wrapper tracefs_function_filter_commit() */


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



-- 
Tzvetomir (Ceco) Stoyanov
VMware Open Source Technology Center

  parent 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
2021-03-30 15:14       ` Steven Rostedt
2021-03-30 15:32       ` Tzvetomir Stoyanov [this message]
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='CAPpZLN4cC4Z7eYP=VTj09XRhQ_jaswcgYXa1MEnvs7RJRVBCeA@mail.gmail.com' \
    --to=tz.stoyanov@gmail.com \
    --cc=linux-trace-devel@vger.kernel.org \
    --cc=rostedt@goodmis.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