Avoid unexpected concurrent access (#94)

In file {chardev,chardev2,sleep}.c, the variable to determine
the exclusive access was of integer type, which led to race
condition.

This patch rewrote the above with atomic CAS respectively
to eliminate the race.

Close #93

diff --git a/examples/chardev.c b/examples/chardev.c
index ecbb1433ececb922d023ea6e7ae1e11ff2f7770f..c571673a213977ea4c2e2273e5748b629c274d08 100644 (file)
--- a/examples/chardev.c
+++ b/examples/chardev.c
@@ -27,8 +27,8 @@ static ssize_t device_write(struct file *, const char __user *, size_t,
 /* Global variables are declared as static, so are global within the file. */
 
 static int major; /* major number assigned to our device driver */
-static int open_device_cnt = 0; /* Is device open?
-                                 * Used to prevent multiple access to device */
+/* Is device open? Used to prevent multiple access to device */
+static atomic_t already_open = ATOMIC_INIT(0);
 static char msg[BUF_LEN]; /* The msg the device will give when asked */
 static char *msg_ptr;
 
@@ -78,10 +78,9 @@ static int device_open(struct inode *inode, struct file *file)
 {
     static int counter = 0;
 
-    if (open_device_cnt)
+    if (atomic_cmpxchg(&already_open, 0, 1))
         return -EBUSY;
 
-    open_device_cnt++;
     sprintf(msg, "I already told you %d times Hello world!\n", counter++);
     msg_ptr = msg;
     try_module_get(THIS_MODULE);
@@ -92,7 +91,7 @@ static int device_open(struct inode *inode, struct file *file)
 /* Called when a process closes the device file. */
 static int device_release(struct inode *inode, struct file *file)
 {
-    open_device_cnt--; /* We're now ready for our next caller */
+    atomic_set(&already_open, 0); /* We're now ready for our next caller */
 
     /* Decrement the usage count, or else once you opened the file, you will
      * never get get rid of the module.

diff --git a/examples/chardev2.c b/examples/chardev2.c
index bb98633872997f903145b1f59b5ff5df83d14233..817be6c121796b7eeb0fc20752e8b60b2529282a 100644 (file)
--- a/examples/chardev2.c
+++ b/examples/chardev2.c
@@ -20,7 +20,7 @@
 /* Is the device open right now? Used to prevent concurrent access into
  * the same device
  */
-static int open_device_cnt = 0;
+static atomic_t already_open = ATOMIC_INIT(0);
 
 /* The message the device will give when asked */
 static char message[BUF_LEN];
@@ -38,10 +38,9 @@ static int device_open(struct inode *inode, struct file *file)
     pr_info("device_open(%p)\n", file);
 
     /* We don't want to talk to two processes at the same time. */
-    if (open_device_cnt)
+    if (atomic_cmpxchg(&already_open, 0, 1))
         return -EBUSY;
 
-    open_device_cnt++;
     /* Initialize the message */
     message_ptr = message;
     try_module_get(THIS_MODULE);
@@ -53,7 +52,7 @@ static int device_release(struct inode *inode, struct file *file)
     pr_info("device_release(%p,%p)\n", inode, file);
 
     /* We're now ready for our next caller */
-    open_device_cnt--;
+    atomic_set(&already_open, 0);
 
     module_put(THIS_MODULE);
     return SUCCESS;

diff --git a/examples/sleep.c b/examples/sleep.c
index f4a953ef53558fc69fed661159f29ce0157ecf8a..d642b1ed2f5d26b07db79da3676aa95393cf4087 100644 (file)
--- a/examples/sleep.c
+++ b/examples/sleep.c
@@ -77,7 +77,7 @@ static ssize_t module_input(struct file *file, /* The file itself */
 }
 
 /* 1 if the file is currently open by somebody */
-static int already_open = 0;
+static atomic_t already_open = ATOMIC_INIT(0);
 
 /* Queue of processes who want our file */
 static DECLARE_WAIT_QUEUE_HEAD(waitq);
@@ -90,7 +90,7 @@ static int module_open(struct inode *inode, struct file *file)
      * we should fail with -EAGAIN, meaning "you will have to try again",
      * instead of blocking a process which would rather stay awake.
      */
-    if ((file->f_flags & O_NONBLOCK) && already_open)
+    if ((file->f_flags & O_NONBLOCK) && atomic_read(&already_open))
         return -EAGAIN;
 
     /* This is the correct place for try_module_get(THIS_MODULE) because if
@@ -99,8 +99,7 @@ static int module_open(struct inode *inode, struct file *file)
      */
     try_module_get(THIS_MODULE);
 
-    /* If the file is already open, wait until it is not. */
-    while (already_open) {
+    while (atomic_cmpxchg(&already_open, 0, 1)) {
         int i, is_sig = 0;
 
         /* This function puts the current process, including any system
@@ -110,7 +109,7 @@ static int module_open(struct inode *inode, struct file *file)
          * is closed) or when a signal, such as Ctrl-C, is sent
          * to the process
          */
-        wait_event_interruptible(waitq, !already_open);
+        wait_event_interruptible(waitq, !atomic_read(&already_open));
 
         /* If we woke up because we got a signal we're not blocking,
          * return -EINTR (fail the system call).  This allows processes
@@ -133,10 +132,6 @@ static int module_open(struct inode *inode, struct file *file)
         }
     }
 
-    /* If we got here, already_open must be zero. */
-
-    /* Open the file */
-    already_open = 1;
     return 0; /* Allow the access */
 }
 
@@ -148,7 +143,7 @@ static int module_close(struct inode *inode, struct file *file)
      * the other processes will be called when already_open is back to one,
      * so they'll go back to sleep.
      */
-    already_open = 0;
+    atomic_set(&already_open, 0);
 
     /* Wake up all the processes in waitq, so if anybody is waiting for the
      * file, they can have it.

diff --git a/lkmpg.tex b/lkmpg.tex
index 3c67bcbbd85c4ffd67d3393fa462905961723159..5dbd0cab0089f39af88db04ac5839261febc8172 100644 (file)
--- a/lkmpg.tex
+++ b/lkmpg.tex
@@ -912,7 +912,7 @@ It is important to keep the counter accurate; if you ever do lose track of the c
 This is bound to happen to you sooner or later during a module's development.
 
 \subsection{chardev.c}
-\label{sec:org7ce767e}
+\label{sec:chardev_c}
 The next code sample creates a char driver named \verb|chardev|.
 You can cat its device file.
 
@@ -925,6 +925,13 @@ We do not support writing to the file (like \sh|echo "hi" > /dev/hello|), but ca
 Don't worry if you don't see what we do with the data we read into the buffer; we don't do much with it.
 We simply read in the data and print a message acknowledging that we received it.
 
+In the multiple-threaded environment, without any protection, concurrent access to the same memory may lead to the race condition, and will not preserve the performance.
+In the kernel module, this problem may happen due to multiple instances accessing the shared resources.
+Therefore, a solution is to enforce the exclusive access.
+We use atomic Compare-And-Swap (CAS), the single atomic operation, to determine whether the file is currently open by someone.
+CAS compares the contents of a memory loaction with the expected value and, only if they are the same, modifies the contents of that memory location to the desired value.
+See more concurrency details in the \ref{sec:synchronization} section.
+
 \samplec{examples/chardev.c}
 
 \subsection{Writing Modules for Multiple Kernel Versions}
@@ -1158,6 +1165,9 @@ In the example below, the header file is chardev.h and the program which uses it
 If you want to use ioctls in your own kernel modules, it is best to receive an official ioctl assignment, so if you accidentally get somebody else's ioctls, or if they get yours, you'll know something is wrong.
 For more information, consult the kernel source tree at \src{Documentation/userspace-api/ioctl/ioctl-number.rst}.
 
+Also, we need to be careful that concurrent access to the shared resources will lead to the race condition.
+The solution is using atomic Compare-And-Swap (CAS), which we mentioned at \ref{sec:chardev_c} section, to enforce the exclusive access.
+
 \samplec{examples/chardev2.c}
 
 \samplec{examples/chardev.h}
@@ -1460,6 +1470,16 @@ An example is shown below.
 
 \samplec{examples/example_atomic.c}
 
+Before the C11 standard adopts the built-in atomic types, the kernel already provided a small set of atomic types by using a bunch of tricky architecture-specific codes.
+Implementing the atomic types by C11 atomics may allow the kernel to throw away the architecture-specific codes and letting the kernel code be more friendly to the people who understand the standard.
+But there are some problems, such as the memory model of the kernel doesn't match the model formed by the C11 atomics.
+For further details, see:
+\begin{itemize}
+ \item \href{https://www.kernel.org/doc/Documentation/atomic_t.txt}{kernel documentation of atomic types}
+ \item \href{https://lwn.net/Articles/691128/}{Time to move to C11 atomics?}
+ \item \href{https://lwn.net/Articles/698315/}{Atomic usage patterns in the kernel}
+\end{itemize}
+
 % FIXME: we should rewrite this section
 \section{Replacing Print Macros}
 \label{sec:print_macros}