On Wed, 16 Oct 2019 16:42:02 -0700
Divya Indi <divya.i...@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.i...@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
