Re: synchronization in ltt-usertrace

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