Linux-rt-users archive on lore.kernel.org
 help / color / Atom feed
From: Tom Zanussi <zanussi@kernel.org>
To: Masami Hiramatsu <mhiramat@kernel.org>
Cc: rostedt@goodmis.org, artem.bityutskiy@linux.intel.com,
	linux-kernel@vger.kernel.org, linux-rt-users@vger.kernel.org
Subject: Re: [PATCH v2 02/12] tracing: Add get/put_event_file()
Date: Mon, 13 Jan 2020 09:15:35 -0600
Message-ID: <1578928535.31031.2.camel@kernel.org> (raw)
In-Reply-To: <20200113221559.3e101e6bbec5c2f6ae6046ba@kernel.org>

Hi Masami,

On Mon, 2020-01-13 at 22:15 +0900, Masami Hiramatsu wrote:
> Hi Tom,
> 
> On Fri, 10 Jan 2020 14:35:08 -0600
> Tom Zanussi <zanussi@kernel.org> wrote:
> 
> > Add a function to get an event file and prevent it from going away
> > on
> > module or instance removal.
> > 
> > get_event_file() will find an event file in a given instance (if
> > instance is NULL, it assumes the top trace array) and return it,
> > pinning the instance's trace array as well as the event's module,
> > if
> > applicable, so they won't go away while in use.
> > 
> > put_event_file() does the matching release.
> > 
> > Signed-off-by: Tom Zanussi <zanussi@kernel.org>
> > ---
> >  include/linux/trace_events.h |  5 +++
> >  kernel/trace/trace_events.c  | 87
> > ++++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 92 insertions(+)
> > 
> > diff --git a/include/linux/trace_events.h
> > b/include/linux/trace_events.h
> > index 4c6e15605766..bc634f4e0158 100644
> > --- a/include/linux/trace_events.h
> > +++ b/include/linux/trace_events.h
> > @@ -348,6 +348,11 @@ enum {
> >  	EVENT_FILE_FL_WAS_ENABLED_BIT,
> >  };
> >  
> > +extern struct trace_event_file *get_event_file(const char
> > *instance,
> > +					       const char *system,
> > +					       const char *event);
> > +extern void put_event_file(struct trace_event_file *file);
> > +
> >  /*
> >   * Event file flags:
> >   *  ENABLED	  - The event is enabled
> > diff --git a/kernel/trace/trace_events.c
> > b/kernel/trace/trace_events.c
> > index c6de3cebc127..184e10d3772d 100644
> > --- a/kernel/trace/trace_events.c
> > +++ b/kernel/trace/trace_events.c
> > @@ -2535,6 +2535,93 @@ find_event_file(struct trace_array *tr,
> > const char *system, const char *event)
> >  	return file;
> >  }
> >  
> > +/**
> > + * get_event_file - Find and return a trace event file
> > + * @instance: The name of the trace instance containing the event
> > + * @system: The name of the system containing the event
> > + * @event: The name of the event
> > + *
> > + * Return a trace event file given the trace instance name, trace
> > + * system, and trace event name.  If the instance name is NULL, it
> > + * refers to the top-level trace array.
> > + *
> > + * This function will look it up and return it if found, after
> > calling
> > + * trace_array_get() to prevent the instance from going away, and
> > + * increment the event's module refcount to prevent it from being
> > + * removed.
> > + *
> > + * To release the file, call put_event_file(), which will call
> > + * trace_array_put() and decrement the event's module refcount.
> > + *
> > + * Return: The trace event on success, ERR_PTR otherwise.
> > + */
> > +struct trace_event_file *get_event_file(const char *instance,
> > +					const char *system,
> > +					const char *event)
> > +{
> > +	struct trace_array *tr = top_trace_array();
> > +	struct trace_event_file *file = NULL;
> > +	int ret = -EINVAL;
> > +
> > +	if (instance) {
> > +		mutex_lock(&trace_types_lock);
> > +		tr = trace_array_find(instance);
> > +		mutex_unlock(&trace_types_lock);
> > +		if (!tr)
> > +			return ERR_PTR(ret);
> > +	}
> > +
> > +	ret = trace_array_get(tr);
> > +	if (ret)
> > +		return ERR_PTR(ret);
> 
> This seems a bit odd. I think we should introduce 
> trace_array_find_get(instance) so that we don't take care of
> loosing tr there. It should be something like
> 
> trace_array_find_get(instance) {
> 	mutex_lock(&trace_types_lock);
> 	tr = trace_array_find(instance);
> 	if (tr)
> 		tr->ref++;
> 	mutex_unlock(&trace_types_lock);
> 	return tr;
> }
> 
> Then we can write it as below;
> 
> 	struct trace_array *tr = top_trace_array();
> 
> 	if (instance) {
> 		tr = trace_array_find_get(instance);
> 		if (!tr)
> 			return -ENOENT;
> 	} else
> 		trace_array_get(tr);/* We never loose top trace_array
> */
> 	...
> 
> This will fail only if there is no instance.
> 

Yes, that's a better idea, will do that in v3.

> BTW, I think the function name should be get_trace_event_file()
> because "event_file" sounds a bit generic. (or trace_get_event_file()
> will give us a good namespace)
> 

I agree, will use trace_get_event_file().

Thanks,

Tom

> Thank you,
> 
> > +
> > +	mutex_lock(&event_mutex);
> > +
> > +	file = find_event_file(tr, system, event);
> > +	if (!file) {
> > +		trace_array_put(tr);
> > +		ret = -EINVAL;
> > +		goto out;
> > +	}
> > +
> > +	/* Don't let event modules unload while in use */
> > +	ret = try_module_get(file->event_call->mod);
> > +	if (!ret) {
> > +		trace_array_put(tr);
> > +		ret = -EBUSY;
> > +		goto out;
> > +	}
> > +
> > +	ret = 0;
> > + out:
> > +	mutex_unlock(&event_mutex);
> > +
> > +	if (ret)
> > +		file = ERR_PTR(ret);
> > +
> > +	return file;
> > +}
> > +EXPORT_SYMBOL_GPL(get_event_file);
> > +
> > +/**
> > + * put_event_file - Release a file from get_event_file()
> > + * @file: The trace event file
> > + *
> > + * If a file was retrieved using get_event_file(), this should be
> > + * called when it's no longer needed.  It will cancel the previous
> > + * trace_array_get() called by that function, and decrement the
> > + * event's module refcount.
> > + */
> > +void put_event_file(struct trace_event_file *file)
> > +{
> > +	trace_array_put(file->tr);
> > +
> > +	mutex_lock(&event_mutex);
> > +	module_put(file->event_call->mod);
> > +	mutex_unlock(&event_mutex);
> > +}
> > +EXPORT_SYMBOL_GPL(put_event_file);
> > +
> >  #ifdef CONFIG_DYNAMIC_FTRACE
> >  
> >  /* Avoid typos */
> > -- 
> > 2.14.1
> > 
> 
> 

  reply index

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-01-10 20:35 [PATCH v2 00/12] tracing: Add support for in-kernel dynamic event API Tom Zanussi
2020-01-10 20:35 ` [PATCH v2 01/12] tracing: Add trace_array_find() to find instance trace arrays Tom Zanussi
2020-01-10 20:35 ` [PATCH v2 02/12] tracing: Add get/put_event_file() Tom Zanussi
2020-01-13 13:15   ` Masami Hiramatsu
2020-01-13 15:15     ` Tom Zanussi [this message]
2020-01-10 20:35 ` [PATCH v2 03/12] tracing: Add delete_synth_event() Tom Zanussi
2020-01-10 20:35 ` [PATCH v2 04/12] tracing: Add dynamic event command creation interface Tom Zanussi
2020-01-14 12:14   ` Masami Hiramatsu
2020-01-14 15:19     ` Tom Zanussi
2020-01-10 20:35 ` [PATCH v2 05/12] tracing: Add synth_event_run_command() Tom Zanussi
2020-01-10 20:35 ` [PATCH v2 06/12] tracing: Add synthetic event command generation functions Tom Zanussi
2020-01-10 20:35 ` [PATCH v2 07/12] tracing: Add trace_synth_event() and related functions Tom Zanussi
2020-01-10 20:35 ` [PATCH v2 08/12] tracing: Add synth event generation test module Tom Zanussi
2020-01-10 20:35 ` [PATCH v2 09/12] tracing: Add trace_kprobe_run_command() Tom Zanussi
2020-01-10 20:35 ` [PATCH v2 10/12] tracing: Add kprobe event command generation functions Tom Zanussi
2020-01-10 20:35 ` [PATCH v2 11/12] tracing: Add kprobe event command generation test module Tom Zanussi
2020-01-10 20:35 ` [PATCH v2 12/12] tracing: Documentation for in-kernel synthetic event API Tom Zanussi

Reply instructions:

You may reply publically 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=1578928535.31031.2.camel@kernel.org \
    --to=zanussi@kernel.org \
    --cc=artem.bityutskiy@linux.intel.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-rt-users@vger.kernel.org \
    --cc=mhiramat@kernel.org \
    --cc=rostedt@goodmis.org \
    /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-rt-users archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-rt-users/0 linux-rt-users/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-rt-users linux-rt-users/ https://lore.kernel.org/linux-rt-users \
		linux-rt-users@vger.kernel.org
	public-inbox-index linux-rt-users

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-rt-users


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