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

[Rodrigo Campos <rodrigo@sdfg.com.ar>]

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@fb.com>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  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 :)

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



Thanks a lot, and sorry again,
Rodrigo