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

Author: linD026

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.

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;

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.

lkmpg.tex

@@ -912,7 +912,7 @@ \subsection{Unregistering A Device}
 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 @@ \subsection{chardev.c}
 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 @@ \section{Talking To Device Files}
 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 @@ \subsection{Atomic operations}
 
 \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}