Re: [PATCH 4/5] tracing: Handle the trace array ref counter in new functions

[Steven Rostedt <rostedt@goodmis.org>]

On Wed, 16 Oct 2019 16:42:02 -0700
Divya Indi <divya.indi@oracle.com> wrote:

> Hi Steve,
> 
> Thanks again for taking the time to review and providing feedback. Please find my comments inline.
> 
> On 10/15/19 4:04 PM, Steven Rostedt wrote:
> > Sorry for taking so long to getting to these patches.
> >
> > On Wed, 14 Aug 2019 10:55:26 -0700
> > Divya Indi <divya.indi@oracle.com> wrote:
> >  
> >> For functions returning a trace array Eg: trace_array_create(), we need to
> >> increment the reference counter associated with the trace array to ensure it
> >> does not get freed when in use.
> >>
> >> Once we are done using the trace array, we need to call
> >> trace_array_put() to make sure we are not holding a reference to it
> >> anymore and the instance/trace array can be removed when required.  
> > I think it would be more in line with other parts of the kernel if we
> > don't need to do the trace_array_put() before calling
> > trace_array_destroy().  
> 
> The reason we went with this approach is
> 
> instance_mkdir -          ref_ctr = 0  // Does not return a trace array ptr.
> trace_array_create -      ref_ctr = 1  // Since this returns a trace array ptr.
> trace_array_lookup -      ref_ctr = 1  // Since this returns a trace array ptr.
> 
> if we make trace_array_destroy to expect ref_ctr to be 1, we risk destroying the trace array while in use.
> 
> We could make it -
> 
> instance_mkdir - 	ref_ctr = 1
> trace_array_create -    ref_ctr = 2
> trace_array_lookup -    ref_ctr = 2+  // depending on no of lookups
> 
> but, we'd still need the trace_array_put() (?)
> 
> We can also have one function doing create (if does not exist) or lookup (if exists), but that would require
> some redundant code since instance_mkdir needs to return -EXIST when a trace array already exists.
> 
> Let me know your thoughts on this.
> 

Can't we just move the trace_array_put() in the instance_rmdir()?

static int instance_rmdir(const char *name)
{
	struct trace_array *tr;
	int ret;

	mutex_lock(&event_mutex);
	mutex_lock(&trace_types_lock);

	ret = -ENODEV;
	list_for_each_entry(tr, &ftrace_trace_arrays, list) {
		if (tr->name && strcmp(tr->name, name) == 0) {
			__trace_array_put(tr);
			ret = __remove_instance(tr);
			if (ret)
				tr->ref++;
			break;
		}
	}

	mutex_unlock(&trace_types_lock);
	mutex_unlock(&event_mutex);

	return ret;
}

-- Steve