Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH 4/5] tracing: Handle the trace array ref counter in new
 functions
To:     Steven Rostedt <rostedt@goodmis.org>
Cc:     linux-kernel@vger.kernel.org, Joe Jin <joe.jin@oracle.com>,
        Srinivas Eeda <srinivas.eeda@oracle.com>,
        Aruna Ramakrishna <aruna.ramakrishna@oracle.com>
References: <1565805327-579-1-git-send-email-divya.indi@oracle.com>
 <1565805327-579-5-git-send-email-divya.indi@oracle.com>
 <20191015190436.65c8c7a3@gandalf.local.home>
 <4cad186e-ba8b-8e1a-731b-4350a095ba5a@oracle.com>
 <20191022225253.4086195c@oasis.local.home>
From:   Divya Indi <divya.indi@oracle.com>
Message-ID: <2b08751a-4028-2130-9a70-c2aa2d76a31c@oracle.com>
Date:   Wed, 23 Oct 2019 15:57:49 -0700
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.14; rv:60.0)
 Gecko/20100101 Thunderbird/60.9.0
MIME-Version: 1.0
In-Reply-To: <20191022225253.4086195c@oasis.local.home>
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit
Content-Language: en-US
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Steven,                                                                         
                                                                                 
A few clarifications on this discussion on reference counter -                   
                                                                                 
1) We will still need to export trace_array_put() to be used for every           
trace_array_get_by_name() OR trace_array_create() + trace_array_get().              
                                                                                 
How else will we reduce the reference counter [For eg: When multiple modules     
lookup the same trace array (say, reference counter = 4)]?                       
                                                                                 
2) tr = trace_array_create("my_tr");                                             
   trace_array_get(tr);                                                          
                                                                                 
Both of these functions will iterate through the list of trace arrays to verify  
whether the trace array exists (redundant, but more intuitive? Does this seem    
acceptable?)                                                                     
                                                                                 
To avoid iterating twice, we went with increasing ref_ctr in trace_array_create. 
This necessitated a trace_array_put() in instance_mkdir (Or as suggested below,
we can do this trace_array_put() in instance_rmdir().)                        
                                                                                                               
                                                                                 
3) A summary of suggested changes (Let me know if this looks good) -                                              
                                                                                 
tr = trace_array_get_by_name("foo-bar"); // ref_ctr++.                           
                                                                                 
if (!tr)                                                                         
{                                                                                
        // instance_mkdir also causes ref_ctr = 1                                
        tr = trace_array_create("foo-bar"); // ref_ctr = 1                       
        trace_array_get(tr); // ref_ctr++                                        
}                                                                                
                                                                                 
trace_array_printk(.....);                                                       
trace_array_set_clr_event(......);                                               
...                                                                              
...                                                                              
...                                                                              
// Done using the trace array.                                                   
trace_array_put(tr); // ref_ctr--                                                
...                                                                              
...                                                                              
...                                                                              
// We can now remove the trace array via trace_array_destroy or instance_rmdir()
trace_array_destroy(tr); // ref_ctr > 1 returns -EBUSY.
                                                       
                          
Thanks,                                                                          
Divya 

On 10/22/19 7:52 PM, Steven Rostedt wrote:
> 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