From mboxrd@z Thu Jan 1 00:00:00 1970 From: nils.carlson@ludd.ltu.se (Nils Carlson) Date: Mon, 14 Feb 2011 20:57:13 +0100 Subject: [ltt-dev] [RFC UST PATCH] Put back urcu_ref counter in buffers.c In-Reply-To: <1297704926-17478-1-git-send-email-david.goulet@polymtl.ca> References: <1297704926-17478-1-git-send-email-david.goulet@polymtl.ca> Message-ID: <77FC3B3C-1E96-467D-90FA-08E9218AFD49@ludd.ltu.se> 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 > --- > 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 > > #include > +#include > > #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; in_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; in_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