On Son, 2002-12-01 at 20:40, Linus Torvalds wrote:
> I'm following the DRM kernel changes, and over the last few days code
> appeared that I really don't think should be in the kernel, and that I
> really don't want to merge.
>
> The problem appears to be that the DRM people are used to using semaphores
> to protect kernel data structures. That is WRONG. It leads to all kinds of
> stupid problems, like realizing that you cannot use semaphores from
> interrupts, so when interrupts need to use the data structures the code
> tries to do all kinds of strange stuff, like using task-queues to do the
> work that should have been done in the interrupt.
Actually, I did that because I thought send_sig_info() or kfree() might
not be interrupt safe.
> The fact is, that for data structure integrity, you should just use a
> spinlock. And if the data structure needs to be accessed from interrupt
> context, just use the irq-safe versions of the spinlock. Please don't
> start using things like task-queues just because the locking was done
> wrong in the first place.
>
> Spinlocks are faster and simpler than semaphores, and optimize away on UP.
> They also end up being a lot more straightforward to use - they have no
> subtle issues. Yes, they require that you do all user-level accesses and
> any allocations that can sleep outside the spinlocked region, but that's a
> _goodness_, not a problem: it just forces the user to not make the
> critical region bigger than it need be.
Does that also apply to kmalloc()/kfree(), or are they safe?
> So _please_, whoever did this (Michel?):
>
> - remove "dev->vbl_sem", and replace it with a "dev->vbl_lock" spinlock
> - use "spin_lock_irqsave()/spun_unlock_irqrestore()" to protect the vbl
> list.
> - rename "vbl_immediate_bh()" as "vbl_send_signals()", and just call it
> directly from the interrupt handler rather than playing all the games
> with task-queues.
> - remove all the vbl_tq stuff.
How does the attached patch look?
> Quite frankly, I'd rather get rid of some of the old TQ stuff in DRM
> (called "work queues" in modern 2.5.x), and I definitely do not want to
> see _more_ of it. I suspect that the only reason the new code uses them is
> that it was was what the DRM code already did from before.
Yes, I had asked about this a while ago but not gotten much feedback so
I copied some similar code from the DRM. Thanks for your feedback, I
appreciate it.
--
Earthling Michel Dänzer (MrCooper)/ Debian GNU/Linux (powerpc) developer
XFree86 and DRI project member / CS student, Free Software enthusiast
Index: programs/Xserver/hw/xfree86/os-support/linux/drm/kernel/drmP.h
===================================================================
RCS file: /cvsroot/dri/xc/xc/programs/Xserver/hw/xfree86/os-support/linux/drm/kernel/drmP.h,v
retrieving revision 1.53
diff -p -u -r1.53 drmP.h
--- programs/Xserver/hw/xfree86/os-support/linux/drm/kernel/drmP.h 30 Nov 2002 14:24:06 -0000 1.53
+++ programs/Xserver/hw/xfree86/os-support/linux/drm/kernel/drmP.h 1 Dec 2002 21:25:57 -0000
@@ -591,8 +600,7 @@ typedef struct drm_device {
#if __HAVE_VBL_IRQ
wait_queue_head_t vbl_queue;
atomic_t vbl_received;
- struct tq_struct vbl_tq;
- struct semaphore vbl_sem;
+ spinlock_t vbl_lock;
drm_vbl_sig_t vbl_sigs;
#endif
cycles_t ctx_start;
@@ -834,7 +845,7 @@ extern void DRM(driver_irq_unin
extern int DRM(wait_vblank)(struct inode *inode, struct file *filp,
unsigned int cmd, unsigned long arg);
extern int DRM(vblank_wait)(drm_device_t *dev, unsigned int *vbl_seq);
-extern void DRM(vbl_immediate_bh)( void *arg );
+extern void DRM(vbl_send_signals)( drm_device_t *dev );
#endif
#if __HAVE_DMA_IRQ_BH
extern void DRM(dma_immediate_bh)( void *dev );
Index: programs/Xserver/hw/xfree86/os-support/linux/drm/kernel/drm_dma.h
===================================================================
RCS file: /cvsroot/dri/xc/xc/programs/Xserver/hw/xfree86/os-support/linux/drm/kernel/drm_dma.h,v
retrieving revision 1.8
diff -p -u -r1.8 drm_dma.h
--- programs/Xserver/hw/xfree86/os-support/linux/drm/kernel/drm_dma.h 30 Nov 2002 14:24:07 -0000 1.8
+++ programs/Xserver/hw/xfree86/os-support/linux/drm/kernel/drm_dma.h 1 Dec 2002 21:25:59 -0000
@@ -509,6 +509,9 @@ int DRM(dma_get_buffers)(drm_device_t *d
int DRM(irq_install)( drm_device_t *dev, int irq )
{
int ret;
+#if __HAVE_VBL_IRQ
+ unsigned long flags;
+#endif
if ( !irq )
return -EINVAL;
@@ -541,16 +544,11 @@ int DRM(irq_install)( drm_device_t *dev,
#if __HAVE_VBL_IRQ
init_waitqueue_head(&dev->vbl_queue);
- sema_init( &dev->vbl_sem, 0 );
+ spin_lock_irqsave( &dev->vbl_lock, flags );
INIT_LIST_HEAD( &dev->vbl_sigs.head );
- up( &dev->vbl_sem );
-
- INIT_LIST_HEAD( &dev->vbl_tq.list );
- dev->vbl_tq.sync = 0;
- dev->vbl_tq.routine = DRM(vbl_immediate_bh);
- dev->vbl_tq.data = dev;
+ spin_unlock_irqrestore( &dev->vbl_lock, flags );
#endif
/* Before installing handler */
@@ -642,6 +640,7 @@ int DRM(wait_vblank)( DRM_IOCTL_ARGS )
flags = vblwait.request.type & _DRM_VBLANK_FLAGS_MASK;
if ( flags & _DRM_VBLANK_SIGNAL ) {
+ unsigned long flags;
drm_vbl_sig_t *vbl_sig = DRM_MALLOC( sizeof( drm_vbl_sig_t ) );
if ( !vbl_sig )
@@ -656,11 +655,11 @@ int DRM(wait_vblank)( DRM_IOCTL_ARGS )
vblwait.reply.sequence = atomic_read( &dev->vbl_received );
/* Hook signal entry into list */
- down( &dev->vbl_sem );
+ spin_lock_irqsave( &dev->vbl_lock, flags );
list_add_tail( (struct list_head *) vbl_sig, &dev->vbl_sigs.head );
- up( &dev->vbl_sem );
+ spin_unlock_irqrestore( &dev->vbl_lock, flags );
} else {
ret = DRM(vblank_wait)( dev, &vblwait.request.sequence );
@@ -675,14 +674,14 @@ int DRM(wait_vblank)( DRM_IOCTL_ARGS )
return ret;
}
-void DRM(vbl_immediate_bh)( void *arg )
+void DRM(vbl_send_signals)( drm_device_t *dev )
{
- drm_device_t *dev = (drm_device_t *) arg;
struct list_head *entry, *tmp;
drm_vbl_sig_t *vbl_sig;
unsigned int vbl_seq = atomic_read( &dev->vbl_received );
+ unsigned long flags;
- down( &dev->vbl_sem );
+ spin_lock_irqsave( &dev->vbl_lock, flags );
list_for_each_safe( entry, tmp, &dev->vbl_sigs.head ) {
@@ -699,7 +698,7 @@ void DRM(vbl_immediate_bh)( void *arg )
}
}
- up( &dev->vbl_sem );
+ spin_unlock_irqrestore( &dev->vbl_lock, flags );
}
#endif /* __HAVE_VBL_IRQ */
Index: programs/Xserver/hw/xfree86/os-support/shared/drm/kernel/mga_irq.c
===================================================================
RCS file: /cvsroot/dri/xc/xc/programs/Xserver/hw/xfree86/os-support/shared/drm/kernel/mga_irq.c,v
retrieving revision 1.2
diff -p -u -r1.2 mga_irq.c
--- programs/Xserver/hw/xfree86/os-support/shared/drm/kernel/mga_irq.c 30 Nov 2002 14:24:07 -0000 1.2
+++ programs/Xserver/hw/xfree86/os-support/shared/drm/kernel/mga_irq.c 1 Dec 2002 21:25:59 -0000
@@ -50,10 +50,7 @@ void mga_dma_service( DRM_IRQ_ARGS )
MGA_WRITE( MGA_ICLEAR, MGA_VLINEICLR );
atomic_inc(&dev->vbl_received);
DRM_WAKEUP(&dev->vbl_queue);
-
- /* kick off bottom half for signals */
- queue_task(&dev->vbl_tq, &tq_immediate);
- mark_bh(IMMEDIATE_BH);
+ DRM(vbl_send_signals)( dev );
}
}
Index: programs/Xserver/hw/xfree86/os-support/shared/drm/kernel/r128_irq.c
===================================================================
RCS file: /cvsroot/dri/xc/xc/programs/Xserver/hw/xfree86/os-support/shared/drm/kernel/r128_irq.c,v
retrieving revision 1.2
diff -p -u -r1.2 r128_irq.c
--- programs/Xserver/hw/xfree86/os-support/shared/drm/kernel/r128_irq.c 30 Nov 2002 14:24:07 -0000 1.2
+++ programs/Xserver/hw/xfree86/os-support/shared/drm/kernel/r128_irq.c 1 Dec 2002 21:25:59 -0000
@@ -50,10 +50,7 @@ void r128_dma_service( DRM_IRQ_ARGS )
R128_WRITE( R128_GEN_INT_STATUS, R128_CRTC_VBLANK_INT_AK );
atomic_inc(&dev->vbl_received);
DRM_WAKEUP(&dev->vbl_queue);
-
- /* kick off bottom half for signals */
- queue_task(&dev->vbl_tq, &tq_immediate);
- mark_bh(IMMEDIATE_BH);
+ DRM(vbl_send_signals)( dev );
}
}
Index: programs/Xserver/hw/xfree86/os-support/shared/drm/kernel/radeon_irq.c
===================================================================
RCS file: /cvsroot/dri/xc/xc/programs/Xserver/hw/xfree86/os-support/shared/drm/kernel/radeon_irq.c,v
retrieving revision 1.9
diff -p -u -r1.9 radeon_irq.c
--- programs/Xserver/hw/xfree86/os-support/shared/drm/kernel/radeon_irq.c 30 Nov 2002 14:24:07 -0000 1.9
+++ programs/Xserver/hw/xfree86/os-support/shared/drm/kernel/radeon_irq.c 1 Dec 2002 21:25:59 -0000
@@ -74,10 +74,7 @@ void DRM(dma_service)( DRM_IRQ_ARGS )
if (stat & RADEON_CRTC_VBLANK_STAT) {
atomic_inc(&dev->vbl_received);
DRM_WAKEUP(&dev->vbl_queue);
-
- /* kick off bottom half for signals */
- queue_task(&dev->vbl_tq, &tq_immediate);
- mark_bh(IMMEDIATE_BH);
+ DRM(vbl_send_signals)( dev );
}
/* Acknowledge all the bits in GEN_INT_STATUS -- seem to get
