Re: DataObjectLockFree: fix facts in doxygen

From 66f5a06aefee62eac2d7ef267c2f7db07280f4be Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?S=C3=A9bastien=20Barth=C3=A9l=C3=A9my?=
 <[email protected]>
Date: Tue, 11 Dec 2012 15:01:10 +0100
Subject: [PATCH 1/2] DataObjectLockFree: fix doxygen

to make clear that this port is only thread-safe with a single
concurrent writer.
---
 rtt/base/DataObjectLockFree.hpp |   33 +++++++++++++++++++++------------
 1 file changed, 21 insertions(+), 12 deletions(-)

diff --git a/rtt/base/DataObjectLockFree.hpp b/rtt/base/DataObjectLockFree.hpp
index d857255..4ac5ae2 100644
--- a/rtt/base/DataObjectLockFree.hpp
+++ b/rtt/base/DataObjectLockFree.hpp
@@ -46,31 +46,41 @@ namespace RTT
 { namespace base {
 
     /**
-     * @brief This DataObject is a Lock-Free implementation,
-     * such that reads and writes can happen concurrently without priority
-     * inversions.
+     * @brief This DataObject is a Lock-Free implementation, such that
+     * several reads and a single write can happen concurrently without
+     * priority inversions.
      *
-     * When there are more writes than reads, the last write will
-     * be returned. The internal buffer can get full if too many
-     * concurrent reads are taking to long. In that case, each new
-     * read will read the element the previous read returned.
+     * The freshest value (the one from the last write) is returned upon
+     * read.
+     *
+     * The internal buffer can get full if too many concurrent reads are
+     * taking too long. In that case, each new read will read the element
+     * the previous read returned instead of the freshest one, until one
+     * or two buffer slots are freed up.
+     *
+     * Readers are garanteed to get the freshest value as long as there are
+     * at most max_threads concurrent reads.
+     *
+     * The internal buffer can get full if too many concurrent reads are
+     * taking to long. In such a case, the write occurs anyway and late
+     * readers can get corrupted values.
      *
      * @verbatim
      * The following Truth table applies when a Low Priority thread is
      * preempted by a High Priority thread :
      *
      *   L\H | Set | Get |
-     *   Set | Ok  | Ok  |
+     *   Set | NA  | Ok  |
      *   Get | Ok  | Ok  |
      *
      * legend : L : Low Priority thread
      *          H : High Priority thread
      *          Blk: Blocks High Priority thread (bad!)
-     *          internal::NA : Not allowed !
+     *          internal::NA : Not allowed!
      * @endverbatim
      * Further, multiple reads may occur before, during and after
-     * a write operation simultaneously. The buffer needs readers+2*writers
-     * elements to be guaranteed non blocking.
+     * a write operation simultaneously. The buffer needs readers+2
+     * elements to be guaranteed thread-safe.
      * @ingroup PortBuffers
      */
     template<class T>
@@ -228,4 +238,3 @@ namespace RTT
 }}
 
 #endif
-
-- 
1.7.9.5

From 478e55abaddaacd726aad581ed9c6d6901ab0d7b Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?S=C3=A9bastien=20Barth=C3=A9l=C3=A9my?=
 <[email protected]>
Date: Wed, 12 Dec 2012 12:38:19 +0100
Subject: [PATCH 2/2] DataObjectLockFree: improve comments

---
 rtt/base/DataObjectLockFree.hpp |   26 ++++++++++++++++++--------
 1 file changed, 18 insertions(+), 8 deletions(-)

diff --git a/rtt/base/DataObjectLockFree.hpp b/rtt/base/DataObjectLockFree.hpp
index 4ac5ae2..e19591c 100644
--- a/rtt/base/DataObjectLockFree.hpp
+++ b/rtt/base/DataObjectLockFree.hpp
@@ -176,13 +176,23 @@ namespace RTT
             PtrType reading;
             // loop to combine Read/Modify of counter
             // This avoids a race condition where read_ptr
-            // could become write_ptr ( then we would read corrupted data).
+            // could become write_ptr (then we would read corrupted data).
             do {
                 reading = read_ptr;            // copy buffer location
-                oro_atomic_inc(&reading->counter); // lock buffer, no more writes
+                // Beware: if we get preempted by a write here, read_ptr and
+                // write_ptr will change behind our back.
+                // Worst, if BUF_LEN-1 writes occur, write_ptr will point to
+                // the same buffer element as the reading variable. A later
+                // write could then corrupt the data while we are reading it.
+                oro_atomic_inc(&reading->counter);
+                // we locked the buffer element, new writes will not set
+                // write_ptr to this element.
+
                 // XXX smp_mb
-                if ( reading != read_ptr )     // if read_ptr changed,
-                    oro_atomic_dec(&reading->counter); // better to start over.
+                if ( reading != read_ptr )
+                    // if read_ptr changed, better to start over to avoid
+                    // corruption.
+                    oro_atomic_dec(&reading->counter);
                 else
                     break;
             } while ( true );
@@ -204,12 +214,12 @@ namespace RTT
              * This method can not be called concurrently (only one
              * producer). With a minimum of 3 buffers, if the
              * write_ptr+1 field is not occupied, it will remain so
-             * because the read_ptr is at write_ptr-1 (and can
-             * not increment the counter on write_ptr+1). Hence, no
+             * because the read_ptr is pointing elsewhere and can
+             * only be changed by a write (this very method). Hence, no
              * locking is needed.
              */
             // writeout in any case
-            write_ptr->data = push;
+            write_ptr->data = push; // takes some time
             PtrType wrote_ptr = write_ptr;
             // if next field is occupied (by read_ptr or counter),
             // go to next and check again...
@@ -217,7 +227,7 @@ namespace RTT
                 {
                     write_ptr = write_ptr->next;
                     if (write_ptr == wrote_ptr)
-                        return; // nothing found, to many readers !
+                        return; // nothing found, too many readers !
                 }
 
             // we will be able to move, so replace read_ptr
-- 
1.7.9.5