Re: synchronization in ltt-usertrace

I made a mistake in my attempt to correct the ltt-usertrace-fast tracing:
there was still the following race window : if a signal came between the
offset read and the signal disabling, it could cause a second pass in
the cmpxchg loop which would overwrite the old mask value. When
reenabling signals, it would put back a signal mask that has signals
disabled, which is utterly wrong.

The following patch addresses this issue. It will be in the newer
ltt-usertrace version.

Mathieu


Index: ltt-usertrace-fast.h
===================================================================
--- ltt-usertrace-fast.h	(revision 2355)
+++ ltt-usertrace-fast.h	(working copy)
@@ -367,6 +367,7 @@
 	int commit_count, reserve_count;
 	int ret;
 	sigset_t oldset, set;
+	int signals_disabled = 0;
 
 	do {
 		offset_old = atomic_read(&ltt_buf->offset);
@@ -418,12 +419,14 @@
 					 * Signals are kept disabled to make sure we win the cmpxchg. */
 
 					/* 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");
-
+					if(!signals_disabled) {
+						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");
+						signals_disabled = 1;
+					}
 					sem_wait(&ltt_buf->writer_sem);
 
 				}
@@ -547,7 +550,7 @@
 		 * sem_wait does in fact win the cmpxchg for the offset. We only call
 		 * these system calls on buffer boundaries because of their performance
 		 * cost. */
-		if(reserve_commit_diff == 0) {
+		if(signals_disabled) {
 			ret = pthread_sigmask(SIG_SETMASK, &oldset, NULL);
 			if(ret) perror("LTT Error in pthread_sigmask\n");
 		}


* Mathieu Desnoyers ([email protected]) wrote:
> Hrm, my solution works, but adds systems calls on the fast path, which
> is precisely what this function shouldn't do.
> 
> Here is a new approach : I only disable signals and sem_wait in :
> 
> > reserve_slot
> >   new subbuffer
> >     next subbuffer is not corrupted
> 
> within the loop. Later, in the if(begin_switch) outside the cmpxchg
> loop, I reenable them. It makes sure no signal will come in my way while
> still not doing a system call on the fast path.
> 
> Here is the patch, applied on top of the previous one :
> 
> 
> Index: ltt/ltt-usertrace-fast.h
> ===================================================================
> --- ltt/ltt-usertrace-fast.h	(revision 2354)
> +++ ltt/ltt-usertrace-fast.h	(working copy)
> @@ -368,15 +368,6 @@
>  	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;
> @@ -416,16 +407,26 @@
>  																												 ltt_buf)])
>  				- atomic_read(&ltt_buf->commit_count[SUBBUF_INDEX(offset_begin,
>  																						 ltt_buf)]);
> +
>  			if(reserve_commit_diff == 0) {
>  				/* Next buffer not corrupted. */
>  				//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.
> +					 * Signals are kept disabled to make sure we win the cmpxchg. */
> +
> +					/* 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);
>  
>  				}
> -
>  					/* go on with the write */
>  
>  				//} else {
> @@ -435,7 +436,8 @@
>  			} else {
>  				/* Next subbuffer corrupted. Force pushing reader even in normal
>  				 * mode. It's safe to write in this new subbuffer. */
> -				sem_post(&ltt_buf->writer_sem);
> +				/* No sem_post is required because we fall through without doing a
> +				 * sem_wait. */
>  			}
>  			size = ltt_get_header_size(trace, ltt_buf->start + offset_begin,
>  					before_hdr_pad, after_hdr_pad, header_size) + data_size;
> @@ -443,6 +445,10 @@
>  				/* Event too big for subbuffers, report error, don't complete 
>  				 * the sub-buffer switch. */
>  				atomic_inc(&ltt_buf->events_lost);
> +				if(reserve_commit_diff == 0) {
> +					ret = pthread_sigmask(SIG_SETMASK, &oldset, NULL);
> +					if(ret) perror("LTT Error in pthread_sigmask\n");
> +				}
>  				return NULL;
>  			} else {
>  				/* We just made a successful buffer switch and the event fits in the
> @@ -463,10 +469,6 @@
>  	} 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 {
>  		consumed_old = atomic_read(&ltt_buf->consumed);
> @@ -541,6 +543,14 @@
>  	}
>  
>  	if(begin_switch) {
> +		/* Enable signals : this is what guaranteed that same reserve which did the
> +		 * sem_wait does in fact win the cmpxchg for the offset. We only call
> +		 * these system calls on buffer boundaries because of their performance
> +		 * cost. */
> +		if(reserve_commit_diff == 0) {
> +			ret = pthread_sigmask(SIG_SETMASK, &oldset, NULL);
> +			if(ret) perror("LTT Error in pthread_sigmask\n");
> +		}
>  		/* New sub-buffer */
>  		/* This code can be executed unordered : writers may already have written
>  			 to the sub-buffer before this code gets executed, caution. */
> 
> 
> Regards,
> 
> Mathieu
> 
> 
> 
> * 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 
> > _______________________________________________
> > 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 
> _______________________________________________
> Ltt-dev mailing list
> [email protected]
> http://listserv.shafik.org/mailman/listinfo/ltt-dev
> 

-- 
Mathieu Desnoyers
Computer Engineering Ph.D. Candidate, École Polytechnique de Montréal
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68