Full preemption issues

Dear Linus,

As you know we at MontaVista are working on a fully preemptable kernel. In this work we have come up with a couple of issues we would like your comments on.

First, as you know, we have added code to the spinlock macros to count up and down a preemption lock counter. We would like to not do this if the macro also turns off local interrupts. The issue here is that in some places in the code, spin_lock_irq() or spin_lock_irqsave() is called but spin_unlock_irq() or spin_lock_irqrestore() is not. This, of course, confuses the preemption count. Attached is a patch that addresses this issue. At this time we are not asking you to apply this patch, but to indicate if we are moving in an acceptable direction.

The second issue resolves around the naming conventions used in the kernel. We want to extend this work to include the SMP kernel, but to do this we need to have several levels of names for the spinlock macros. We note that the kernel uses "_" and "__" prefixes in some macros, but can not, by inspection, figure out when to uses these prefixes. Could you explain this convention or is this wisdom written somewhere?

To clarify the intent here is a bit of proto code:

#ifdef CONFIG_PREEMPT
#define preempt_lock() ... definition...
#define preempt_unlock() ...definition...
#else
#define preempt_lock()
#define preempt_unlock()
#endif

#ifdef CONFIG_SMP
#define _spin_lock(x) __spin_lock(x) /* __spin_lock(x) to be todays SMP definition */
#define _spin_unlock(x) __spin_unlock(x) /* __spin_unlock(x) to be todays SMP definition */
#else
#define _spin_lock()
#define _spin_unlock()
#endif

#define spin_lock(x) do{ preempt_lock(); _spin_lock(x);} while (0) #define spin_unlock(x) do{ _spin_unlock(x); preempt_unlock();} while (0)

George

diff -urP -X patch.exclude linux-2.4.0-test6-kb-p-r-i-6-1.4/drivers/ide/ide.c linux/drivers/ide/ide.c
@@ -1354,7 +1354,7 @@
 		 */
 		if (masked_irq && hwif->irq != masked_irq)
 			disable_irq_nosync(hwif->irq);
-		spin_unlock(&io_request_lock);
+		spin_unlock_noirqrestore(&io_request_lock);
 		ide__sti();	/* allow other IRQs while we start this request */
 		startstop = start_request(drive);
 		spin_lock_irq(&io_request_lock);

@@ -1438,7 +1438,7 @@
 			 * the handler() function, which means we need to globally
 			 * mask the specific IRQ:
 			 */
-			spin_unlock(&io_request_lock);
+			spin_unlock_noirqrestore(&io_request_lock);
 			hwif  = HWIF(drive);
 #if DISABLE_IRQ_NOSYNC
 			disable_irq_nosync(hwif->irq);

@@ -1599,7 +1599,7 @@
 	}
 	hwgroup->handler = NULL;
 	del_timer(&hwgroup->timer);
-	spin_unlock(&io_request_lock);
+	spin_unlock_noirqrestore(&io_request_lock);
 
 	if (drive->unmask)
 		ide__sti();	/* local CPU only */

diff -urP -X patch.exclude linux-2.4.0-test6-kb-p-r-i-6-1.4/include/linux/wait.h linux/include/linux/wait.h
@@ -76,6 +76,9 @@
 # define wq_write_lock_irqsave write_lock_irqsave
 # define wq_write_unlock_irqrestore write_unlock_irqrestore
 # define wq_write_unlock write_unlock
+# define wq_read_unlock_noirqrestore read_unlock_noirqrestore
+# define wq_write_unlock_noirqrestore write_unlock_noirqrestore
+# define wq_write_unlock_noirq write_unlock_noirq
 #else
 # define wq_lock_t spinlock_t
 # define WAITQUEUE_RW_LOCK_UNLOCKED SPIN_LOCK_UNLOCKED

@@ -88,6 +91,9 @@
 # define wq_write_lock_irqsave spin_lock_irqsave
 # define wq_write_unlock_irqrestore spin_unlock_irqrestore
 # define wq_write_unlock spin_unlock
+# define wq_read_unlock_noirqrestore spin_unlock_noirqrestore
+# define wq_write_unlock_noirqrestore spin_unlock_noirqrestore
+# define wq_write_unlock_noirq spin_unlock_noirq
 #endif
 
 struct __wait_queue_head {

diff -urP -X patch.exclude linux-2.4.0-test6-kb-p-r-i-6-1.4/kernel/sched.c linux/kernel/sched.c
@@ -838,7 +838,7 @@
 #define	SLEEP_ON_HEAD					\
 	wq_write_lock_irqsave(&q->lock,flags);		\
 	__add_wait_queue(q, &wait);			\
-	wq_write_unlock(&q->lock);
+	wq_write_unlock_noirqrestore(&q->lock);
 
 #define	SLEEP_ON_TAIL						\
 	wq_write_lock_irq(&q->lock);				\

Looks entirely sane to me.

The "wisdom" is not written down anywhere, and is more a convention than anything else. The convention is that a prepended "__" means that "this is an internal routine, and you can use it, but you should damn well know what you're doing if you do". For example, the most common use is for routines that need external locking - the version that does its own locking and is thus "safe" to use in normal circumstances has the regular name, and the version of the routine that does no locking and depends on the caller to lock for it has the "__" version.

Your proto code does not break this convention in any way..

Linus

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
Please read the FAQ at http://www.tux.org/lkml/