Re: synchronization in ltt-usertrace

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