Messages in this thread | | | Date | Tue, 22 Oct 2019 22:52:53 -0400 | From | Steven Rostedt <> | Subject | Re: [PATCH 4/5] tracing: Handle the trace array ref counter in new functions |
| |
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
| |