[ltt-dev] [RFC UST PATCH] Put back urcu_ref counter in buffers.c

[nils.carlson@ludd.ltu.se (Nils Carlson)]

Hi David,

Your patch says what it does, but nothing about why. Please elaborate.

/Nils
On Feb 14, 2011, at 6:35 PM, David Goulet wrote:

> Signed-off-by: David Goulet <david.goulet at polymtl.ca>
> ---
> libust/buffers.c |  133 +++++++++++++++++++++++++++++++++ 
> +-------------------
> 1 files changed, 85 insertions(+), 48 deletions(-)
>
> diff --git a/libust/buffers.c b/libust/buffers.c
> index 4e8004c..a82538b 100644
> --- a/libust/buffers.c
> +++ b/libust/buffers.c
> @@ -28,6 +28,7 @@
> #include <stdlib.h>
>
> #include <ust/clock.h>
> +#include <urcu/urcu_ref.h>
>
> #include "buffers.h"
> #include "channels.h"
> @@ -150,6 +151,18 @@ static void ltt_buffer_begin(struct ust_buffer  
> *buf,
> 	ltt_write_trace_header(channel->trace, header);
> }
>
> +static int unmap_buf_structs(struct ust_channel *chan)
> +{
> +	int i;
> +
> +	for (i=0; i < chan->n_cpus; i++) {
> +		if (shmdt(chan->buf[i]) < 0) {
> +			PERROR("shmdt");
> +		}
> +	}
> +	return 0;
> +}
> +
> static int map_buf_data(struct ust_buffer *buf, size_t *size)
> {
> 	void *ptr;
> @@ -208,11 +221,17 @@ static int open_buf(struct ust_channel *chan,  
> int cpu)
> 	if (result < 0)
> 		return -1;
>
> +	urcu_ref_init(&chan->buf[cpu]->urcu_ref);
> +
> 	buf->commit_count =
> 		zmalloc(sizeof(*buf->commit_count) * n_subbufs);
> 	if (!buf->commit_count)
> 		goto unmap_buf;
>
> +	urcu_ref_get(&trace->urcu_ref);
> +	urcu_ref_get(&trace->ltt_transport_urcu_ref);
> +	urcu_ref_get(&chan->urcu_ref);
> +
> 	result = pipe(fds);
> 	if (result < 0) {
> 		PERROR("pipe");
> @@ -222,7 +241,9 @@ static int open_buf(struct ust_channel *chan,  
> int cpu)
> 	buf->data_ready_fd_write = fds[1];
>
> 	buf->cpu = cpu;
> +
> 	buf->chan = chan;
> +	urcu_ref_get(&chan->urcu_ref);
>
> 	uatomic_set(&buf->offset, ltt_subbuffer_header_size());
> 	uatomic_set(&buf->consumed, 0);
> @@ -254,13 +275,17 @@ unmap_buf:
> 	return -1;
> }
>
> +static void release_channel(struct urcu_ref *);
> static void ltt_relay_print_buffer_errors(struct ust_channel *chan,  
> int cpu);
>
> -static void close_buf(struct ust_buffer *buf)
> +static void destroy_buf(struct ust_buffer *buf)
> {
> +	int result;
> 	struct ust_channel *chan = buf->chan;
> 	int cpu = buf->cpu;
> -	int result;
> +
> +	/* Print possible buffer errors */
> +	ltt_relay_print_buffer_errors(chan, cpu);
>
> 	result = shmdt(buf->buf_data);
> 	if (result < 0) {
> @@ -279,10 +304,61 @@ static void close_buf(struct ust_buffer *buf)
> 		PERROR("close");
> 	}
>
> -	/* FIXME: This spews out errors, are they real?:
> -	 * ltt_relay_print_buffer_errors(chan, cpu); */
> +	urcu_ref_put(&chan->urcu_ref, release_channel);
> +	urcu_ref_put(&chan->trace->urcu_ref, ltt_release_trace);
> }
>
> +static void remove_buf(struct urcu_ref *urcu_ref)
> +{
> +	struct ust_buffer *buf = _ust_container_of(urcu_ref, struct  
> ust_buffer, urcu_ref);
> +	destroy_buf(buf);
> +}
> +
> +static void close_buf(struct ust_buffer *buf)
> +{
> +	urcu_ref_put(&buf->urcu_ref, remove_buf);
> +}
> +
> +static void destroy_channel(struct urcu_ref *urcu_ref)
> +{
> +	struct ust_channel *chan = _ust_container_of(urcu_ref, struct  
> ust_channel, urcu_ref);
> +
> +	free(chan);
> +}
> +
> +static void release_channel(struct urcu_ref *urcu_ref)
> +{
> +	struct ust_channel *chan = _ust_container_of(urcu_ref,
> +			struct ust_channel, urcu_ref);
> +
> +	free(chan->buf);
> +}
> +
> +static void close_channel(struct ust_channel *chan)
> +{
> +	int i;
> +	if(!chan)
> +		return;
> +
> +	pthread_mutex_lock(&ust_buffers_channels_mutex);
> +	for(i=0; i<chan->n_cpus; i++) {
> +		/* FIXME: if we make it here, then all buffers were necessarily  
> allocated. Moreover, we don't
> +		 * initialize to NULL so we cannot use this check. Should we? */
> +		//ust//		if (chan->buf[i])
> +		close_buf(chan->buf[i]);
> +	}
> +
> +	cds_list_del(&chan->list);
> +	urcu_ref_put(&chan->urcu_ref, destroy_channel);
> +	pthread_mutex_unlock(&ust_buffers_channels_mutex);
> +}
> +
> +static void remove_channel(struct ust_channel *chan)
> +{
> +	close_channel(chan);
> +	unmap_buf_structs(chan);
> +	urcu_ref_put(&chan->urcu_ref, release_channel);
> +}
>
> static int open_channel(struct ust_channel *chan, size_t subbuf_size,
> 			size_t subbuf_cnt)
> @@ -308,6 +384,8 @@ static int open_channel(struct ust_channel  
> *chan, size_t subbuf_size,
> 	chan->subbuf_size_order = get_count_order(subbuf_size);
> 	chan->alloc_size = subbuf_size * subbuf_cnt;
>
> +	urcu_ref_init(&chan->urcu_ref);
> +
> 	pthread_mutex_lock(&ust_buffers_channels_mutex);
> 	for (i=0; i < chan->n_cpus; i++) {
> 		result = open_buf(chan, i);
> @@ -327,29 +405,11 @@ error:
> 		do {} while(0);
> 	}
>
> +	urcu_ref_put(&chan->urcu_ref, destroy_channel);
> 	pthread_mutex_unlock(&ust_buffers_channels_mutex);
> 	return -1;
> }
>
> -static void close_channel(struct ust_channel *chan)
> -{
> -	int i;
> -	if(!chan)
> -		return;
> -
> -	pthread_mutex_lock(&ust_buffers_channels_mutex);
> -	for(i=0; i<chan->n_cpus; i++) {
> -	/* FIXME: if we make it here, then all buffers were necessarily  
> allocated. Moreover, we don't
> -	 * initialize to NULL so we cannot use this check. Should we? */
> -//ust//		if (chan->buf[i])
> -			close_buf(chan->buf[i]);
> -	}
> -
> -	cds_list_del(&chan->list);
> -
> -	pthread_mutex_unlock(&ust_buffers_channels_mutex);
> -}
> -
> static void ltt_force_switch(struct ust_buffer *buf,
> 		enum force_switch_mode mode);
>
> @@ -646,18 +706,6 @@ static int map_buf_structs(struct ust_channel  
> *chan)
> 	return -1;
> }
>
> -static int unmap_buf_structs(struct ust_channel *chan)
> -{
> -	int i;
> -
> -	for (i=0; i < chan->n_cpus; i++) {
> -		if (shmdt(chan->buf[i]) < 0) {
> -			PERROR("shmdt");
> -		}
> -	}
> -	return 0;
> -}
> -
> /*
>  * Create channel.
>  */
> @@ -667,6 +715,8 @@ static int create_channel(const char  
> *trace_name, struct ust_trace *trace,
> {
> 	int i, result;
>
> +	urcu_ref_init(&chan->urcu_ref);
> +
> 	chan->trace = trace;
> 	chan->overwrite = overwrite;
> 	chan->n_subbufs_order = get_count_order(n_subbufs);
> @@ -714,19 +764,6 @@ error:
> 	return -1;
> }
>
> -
> -static void remove_channel(struct ust_channel *chan)
> -{
> -	close_channel(chan);
> -
> -	unmap_buf_structs(chan);
> -
> -	free(chan->buf_struct_shmids);
> -
> -	free(chan->buf);
> -
> -}
> -
> static void ltt_relay_async_wakeup_chan(struct ust_channel  
> *ltt_channel)
> {
> //ust//	unsigned int i;
> -- 
> 1.7.4
>
>
> _______________________________________________
> ltt-dev mailing list
> ltt-dev at lists.casi.polymtl.ca
> http://lists.casi.polymtl.ca/cgi-bin/mailman/listinfo/ltt-dev