Re: [PATCH 18/21] perf top: Convert to hist_entry_iter

[Namhyung Kim]

On Fri, 7 Feb 2014 13:26:01 +0000, Rodrigo Campos wrote:
> On Fri, Feb 07, 2014 at 10:35:20AM +0900, Namhyung Kim wrote:
>> Reuse hist_entry_iter__add() function to share the similar code with
>> perf report.  Note that it needs to be called with hists.lock so tweak
>> some internal functions not to deadlock or hold the lock too long.
>> 
>> Tested-by: Arun Sharma <asharma@xxxxxx>
>> Signed-off-by: Namhyung Kim <namhyung@xxxxxxxxxx>
>> ---
>>  tools/perf/builtin-top.c | 78 ++++++++++++++++++++++++++----------------------
>>  1 file changed, 43 insertions(+), 35 deletions(-)
>> 
>> diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
>> index 40e430530168..e7d67421eb0f 100644
>> --- a/tools/perf/builtin-top.c
>> +++ b/tools/perf/builtin-top.c
>> @@ -194,6 +194,12 @@ static void perf_top__record_precise_ip(struct perf_top *top,
>>  
>>  	pthread_mutex_unlock(&notes->lock);
>>  
>> +	/*
>> +	 * This function is now called with he->hists->lock held.
>> +	 * Release it before going to sleep.
>> +	 */
>> +	pthread_mutex_unlock(&he->hists->lock);
>> +
>>  	if (err == -ERANGE && !he->ms.map->erange_warned)
>>  		ui__warn_map_erange(he->ms.map, sym, ip);
>>  	else if (err == -ENOMEM) {
>> @@ -201,6 +207,8 @@ static void perf_top__record_precise_ip(struct perf_top *top,
>>  		       sym->name);
>>  		sleep(1);
>>  	}
>> +
>> +	pthread_mutex_lock(&he->hists->lock);
>>  }
>
>
> I've seen Jiri's comment on this on a previous version of the patch, and the
> comment now really helps. Thanks :)
>
> But it still feels weird for me unlocking and then re-aquiring the lock at the
> end. Maybe is error prone too, as it's easy to add a "return" in between where
> we don't have yet aquired the lock and everything breaks, for example. I would
> document that this function must always return with that lock acquired. But
> maybe this is too personal :)

Yeah, it's a bit sad to require such weird locking behavior.  And I
agree that adding more comments will help preventing those possible
mistakes.

>
> And just curious, isn't it possible that the caller releases the lock before
> calling the function and aquires it after ? This function could then aquire +
> release the lock and be done with it. Or do you need that atomicity and know
> that nobody else have taken the lock between the caller took it and this
> function was called ? Although I'm not sure if this is better... Maybe is the
> same at the end, as the caller should (maybe) always unlock before calling.
>
> Also, I realize this is shared code so it might not make any sense or even
> complicate things a lot more in some other places where this ends-up being
> called. Sorry in advance if this is the case, I really don't know about perf :S

Don't say sorry. :)

The perf_top__record_precise_ip() is called in hist_iter_cb() callback
function so that we cannot control how it'll be called since it's now in
the generic code.  So it acquires the lock before calling the generic
code and then release-and-reacquire in the callback function.

And the hist lock is needed during inserting an entry since display
thread can switch to another tree anytime.

Thanks,
Namhyung
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/