[lttng-dev] [PATCH lttng-ust] Fix: Make deadlocks with baddr statedump unlikely

[Mathieu Desnoyers]

----- Original Message -----
> From: "Paul Woegerer" <paul_woegerer at mentor.com>
> To: lttng-dev at lists.lttng.org, "mathieu desnoyers" <mathieu.desnoyers at efficios.com>
> Sent: Monday, March 17, 2014 5:22:01 AM
> Subject: [PATCH lttng-ust] Fix: Make deadlocks with baddr statedump unlikely

Hi Paul,

A partial fix changing the code and leaving the bug in place is not
interesting for us, and will add a maintainance burden (extra delta
between current master and stable-2.4) for an experimental feature.

You might want to focus on getting the right fix in for 2.5, while we
are still in the development cycle.

Thanks,

Mathieu

> 
> Signed-off-by: Paul Woegerer <paul_woegerer at mentor.com>
> ---
>  liblttng-ust/lttng-ust-baddr.c | 95
>  ++++++++++++++++++++++++++++++++----------
>  1 file changed, 74 insertions(+), 21 deletions(-)
> 
> diff --git a/liblttng-ust/lttng-ust-baddr.c b/liblttng-ust/lttng-ust-baddr.c
> index dec7e82..0e8ff24 100644
> --- a/liblttng-ust/lttng-ust-baddr.c
> +++ b/liblttng-ust/lttng-ust-baddr.c
> @@ -39,17 +39,50 @@
>  #define TP_SESSION_CHECK
>  #include "ust_baddr_statedump.h"
>  
> +struct baddr_entry {
> +	void *base_addr_ptr;
> +	char resolved_path[PATH_MAX];
> +	int vdso;
> +};
> +
>  struct extract_data {
> -	void *owner;
>  	void *exec_baddr;	/* executable base address */
> +	struct baddr_entry *baddrs;	/* other base addresses */
> +	size_t num_baddrs;		/* number of other base addresses */
> +	size_t idx_baddrs;		/* current base addresse index */
>  };
>  
> +static
> +int add_baddr(void *base_addr_ptr,
> +	const char *resolved_path,
> +	int vdso,
> +	struct extract_data *data)
> +{
> +	struct baddr_entry *baddr;
> +
> +	if (data->num_baddrs < data->idx_baddrs + 1) {
> +		data->num_baddrs *= 2;
> +		data->baddrs = realloc(data->baddrs,
> +			data->num_baddrs * sizeof(struct baddr_entry));
> +		if (!data->baddrs)
> +			return 1;
> +	}
> +
> +	baddr = data->baddrs + data->idx_baddrs;
> +	baddr->base_addr_ptr = base_addr_ptr;
> +	strncpy(baddr->resolved_path, resolved_path, PATH_MAX - 1);
> +	baddr->vdso = vdso;
> +
> +	data->idx_baddrs += 1;
> +	return 0;
> +}
> +
>  /*
>   * Trace baddr into all sessions for which statedump is pending owned by
>   * the caller thread.
>   */
>  static
> -int trace_baddr(void *base_addr_ptr,
> +void trace_baddr(void *base_addr_ptr,
>  	const char *resolved_path,
>  	int vdso,
>  	void *owner)
> @@ -62,16 +95,6 @@ int trace_baddr(void *base_addr_ptr,
>  		sostat.st_size = 0;
>  		sostat.st_mtime = -1;
>  	}
> -	/*
> -	 * UST lock nests within dynamic loader lock.
> -	 */
> -	if (ust_lock()) {
> -		/*
> -		 * Stop iteration on headers if need to exit.
> -		 */
> -		ust_unlock();
> -		return 1;
> -	}
>  
>  	sessionsp = _lttng_get_sessions();
>  	cds_list_for_each_entry(session, sessionsp, node) {
> @@ -84,8 +107,6 @@ int trace_baddr(void *base_addr_ptr,
>  				resolved_path, sostat.st_size,
>  				sostat.st_mtime);
>  	}
> -	ust_unlock();
> -	return 0;
>  }
>  
>  static
> @@ -93,7 +114,6 @@ int extract_soinfo_events(struct dl_phdr_info *info,
> size_t size, void *_data)
>  {
>  	int j;
>  	struct extract_data *data = _data;
> -	void *owner = data->owner;
>  
>  	for (j = 0; j < info->dlpi_phnum; j++) {
>  		char resolved_path[PATH_MAX];
> @@ -138,9 +158,8 @@ int extract_soinfo_events(struct dl_phdr_info *info,
> size_t size, void *_data)
>  				vdso = 1;
>  			}
>  		}
> -		if (trace_baddr(base_addr_ptr, resolved_path, vdso, owner)) {
> +		if (add_baddr(base_addr_ptr, resolved_path, vdso, data))
>  			return 1;
> -		}
>  		/*
>  		 * We are only interested in the base address (lowest virtual
>  		 * address associated with the memory image), skip the rest
> @@ -151,9 +170,8 @@ int extract_soinfo_events(struct dl_phdr_info *info,
> size_t size, void *_data)
>  }
>  
>  static
> -void dump_exec_baddr(struct extract_data *data)
> +void dump_exec_baddr(struct extract_data *data, void *owner)
>  {
> -	void *owner = data->owner;
>  	void *base_addr_ptr;
>  	char exe_path[PATH_MAX];
>  	ssize_t exe_len;
> @@ -172,6 +190,38 @@ void dump_exec_baddr(struct extract_data *data)
>  	trace_baddr(base_addr_ptr, exe_path, 0, owner);
>  }
>  
> +static
> +void dump_baddrs(struct extract_data *data, void *owner)
> +{
> +	/* Emit tracepoints for shared objects */
> +	struct baddr_entry *baddrs = data->baddrs;
> +	size_t idx_baddrs = data->idx_baddrs;
> +	size_t i = 0;
> +
> +	/*
> +	 * UST lock nests within dynamic loader lock.
> +	 */
> +	if (ust_lock()) {
> +		/*
> +		 * Stop if need to exit.
> +		 */
> +		ust_unlock();
> +		return;
> +	}
> +
> +	while (i < idx_baddrs) {
> +		struct baddr_entry *baddr = baddrs + i;
> +		trace_baddr(baddr->base_addr_ptr,
> +			baddr->resolved_path,
> +			baddr->vdso, owner);
> +		i += 1;
> +	}
> +	/* Emit tracepoint for executable */
> +	dump_exec_baddr(data, owner);
> +
> +	ust_unlock();
> +}
> +
>  int lttng_ust_baddr_statedump(void *owner)
>  {
>  	struct extract_data data;
> @@ -179,8 +229,10 @@ int lttng_ust_baddr_statedump(void *owner)
>  	if (!getenv("LTTNG_UST_WITH_EXPERIMENTAL_BADDR_STATEDUMP"))
>  		return 0;
>  
> -	data.owner = owner;
>  	data.exec_baddr = NULL;
> +	data.num_baddrs = 32;
> +	data.baddrs = malloc(data.num_baddrs * sizeof(struct baddr_entry));
> +	data.idx_baddrs = 0;
>  	/*
>  	 * Iterate through the list of currently loaded shared objects and
>  	 * generate events for loadable segments using
> @@ -193,7 +245,8 @@ int lttng_ust_baddr_statedump(void *owner)
>  	 * deadlocks, so dump the executable outside of the phdr
>  	 * iteration.
>  	 */
> -	dump_exec_baddr(&data);
> +	dump_baddrs(&data, owner);
> +	free(data.baddrs);
>  	return 0;
>  }
>  
> --
> 1.9.0
> 
> 

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com