Re: synchronization in ltt-usertrace

Hello Mathieu,

Thank you for looking into the case. I think your fix would work, but
if you disable signals around the loop, why do you need to use
atomic_cmpxchg? I think that the reader does not change the value
while the writer is up; if that's correct, there is no other
schedulable entity to change the offset value while the writer is
executing the loop body.

Also, I don't understand why the writer calls sem_post when a
corrupted subbuffer appears. I understand the writer itself frees and
uses the subbuffer. Would this mean, in essence, that the writer needs
to increment and decrement the semaphore?

Thanks very much in advance,

Naoya Maruyama

Mathieu Desnoyers <[email protected]> wrote:
> Hi Naoya,
> 
> Very good catch :)
> 
> writer_sem is a counter, always positive, which indicates the number of
> free subbuffers available for writing. Here is how it is used :
> 
> init writer_sem to the number of subbuffers (indicates the number of
> subbuffers free for writing)
> 
> in the writer :
> reserve_slot
>   new subbuffer
>     next subbuffer is not corrupted
>       wait for a free subbuffer (sem_wait(&ltt_buf->writer_sem);)
>     next subbuffer is corrupted
>       free the subbuffer ourself (sem_post(&ltt_buf->writer_sem);)
> 
> in the reader :
> ltt_buffer_put
>   if we have been pushed by the writer because it was corrupted
>     return error
>   else, we just successfully read a subbuffer
>     indicate that the subbuffer is free (sem_post)
> 
> 
> What you pointed out in ltt-usertrace ltt_reserve_slot() is correct :
> there is a case where it's not signal-safe.
> 
> To correct it, I propose to disable signals around the cmpxchg loop,
> like this :
> 
> 
> Index: ltt-usertrace-fast.h
> ===================================================================
> --- ltt-usertrace-fast.h	(revision 2076)
> +++ ltt-usertrace-fast.h	(working copy)
> @@ -366,7 +366,17 @@
>  	int consumed_old, consumed_new;
>  	int commit_count, reserve_count;
>  	int ret;
> +	sigset_t oldset, set;
>  
> +	/* sem_wait is not signal safe. Disable signals around it. */
> +
> +	/* Disable signals */
> +	ret = sigfillset(&set);
> +	if(ret) perror("LTT Error in sigfillset\n"); 
> +	
> +	ret = pthread_sigmask(SIG_BLOCK, &set, &oldset);
> +	if(ret) perror("LTT Error in pthread_sigmask\n");
> +
>  	do {
>  		offset_old = atomic_read(&ltt_buf->offset);
>  		offset_begin = offset_old;
> @@ -411,22 +421,9 @@
>  				//if((SUBBUF_TRUNC(offset_begin, ltt_buf) 
>  				//				- SUBBUF_TRUNC(atomic_read(&ltt_buf->consumed), ltt_buf))
>  				//					>= ltt_buf->alloc_size) {
> -				/* sem_wait is not signal safe. Disable signals around it. */
>  				{
> -					sigset_t oldset, set;
> -
> -					/* Disable signals */
> -					ret = sigfillset(&set);
> -					if(ret) perror("LTT Error in sigfillset\n"); 
> -					
> -					ret = pthread_sigmask(SIG_BLOCK, &set, &oldset);
> -					if(ret) perror("LTT Error in pthread_sigmask\n");
> -
>  					sem_wait(&ltt_buf->writer_sem);
>  
> -					/* Enable signals */
> -					ret = pthread_sigmask(SIG_SETMASK, &oldset, NULL);
> -					if(ret) perror("LTT Error in pthread_sigmask\n");
>  				}
>  
>  					/* go on with the write */
> @@ -466,6 +463,9 @@
>  	} while(atomic_cmpxchg(&ltt_buf->offset, offset_old, offset_end)
>  							!= offset_old);
>  
> +	/* Enable signals */
> +	ret = pthread_sigmask(SIG_SETMASK, &oldset, NULL);
> +	if(ret) perror("LTT Error in pthread_sigmask\n");
>  
>  	/* Push the reader if necessary */
>  	do {
> 
> What do you think of this fix ?
> 
> Regards,
> 
> Mathieu
> 
> 
> * Naoya Maruyama ([email protected]) wrote:
> > Hello,
> > 
> > I'm currently walking through the usertrace code, and am confused by a
> > couple of code sections of the "fast" tracer.
> > 
> > My question is the use of a semaphore in ltt_reserve_slot function in
> > ltt-usertrae/ltt/ltt-usertrace-fast.h. I don't fully understand the
> > entire code yet, but I'm thinking that it's not signal-safe in the
> > following condition. I would appreciate if anyone could tell me
> > whether my understanding is correct or not.
> > 
> > 1. Let's say the writer has just called reserve_slot and the
> > reservation starts at a new subbuffer.
> > 
> > 2. Assuming the subbuffer is not corrupted, the writer will call
> > sem_wait.
> > 
> > 3. The write successfully returns from sem_wait, but is preempted by a
> > signal after the signals being re-enabled, but before the cmpxchg.
> > 
> > 4. The handler for the signal could call reserve_slot, find the new
> > slot starts at the beginning of the next subbuffer, because the writer
> > does not yet update the value of offset field.
> > 
> > So, if the above steps happened, sem_wait would be called twice, even
> > though the writer and its signal handler would eventually use the same
> > subbuffer. If my understanding is correct, the semaphore is
> > initialized to the number of the subbuffers. If it's true, it wouldn't
> > make sense to call sem_wait multiple times for a single subbuffer,
> > right?
> > 
> > BTW, in the same function (reserve_slot in ltt-usertrace-fast.h), I
> > don't yet understand why sem_post is called when the next subbuffer is
> > corrupted, which might explain why I don't understand the call to
> > sem_wait.
> > 
> > Thanks very much in advance,
> > 
> > Naoya Maruyama
> > _______________________________________________
> > Ltt-dev mailing list
> > [email protected]
> > http://listserv.shafik.org/mailman/listinfo/ltt-dev
> > 
> 
> -- 
> OpenPGP public key:              http://krystal.dyndns.org:8080/key/compudj.gpg
> Key fingerprint:     8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68