Re: LiS ldl driver with 2.6 kernels

Hai,

No, the driver needs to be reworked to not use SPLSTR or SPLX at
all.  SPLSTR and SPLX are non-MP safe SVR 4 functions and they
are not doing what the original author thinks they are.

On an SVR 4 UP system, SPLSTR simply disables interrupts,
however, LiS's SPLSTR takes a global (recursive) spin lock
(lis_spl_lock) and disables _local_ (I repeat _LOCAL_)
interrupts.  Therefore, SPLSTR only ensure mutual exclusion with
other critical sections bracketed by SPLSTR/SPLX and has no
effect on interrupt service routines or bottom half procedures.
Because lis_spl_lock is not known outside of streams, on an MP
machine, SPLSTR has no locking effect on any other part of the
kernel.

As far as the irq portion of SPLSTR, there is no part of this
code that executes within an interrupt service procedure and all
other interrupt service procedures have no knowledge of
lis_spl_lock, so it is pointless to suppress local interrupts.
The spin itself would be sufficient, and that spin could be
something like ldl_spin_lock if one wants to be that lazy with
lock heirarchies.

I think you have to remember that this driver was originally
written on linux 2.0 and 2.2 that were UP only (no MP), where
SPLSTR/SPLX was simply cli()/sti().

Therefore, all occurences of SPLSTR can be replaced by
spin_lock(&ldl_spin_lock) and SPLX by
spin_unlock(&ldl_spin_lock).

This exposes a race.  The write side of the queue take
write_lock(&pt->lock) while the receive callback takes
read_lock(&pt->lock).  This is not protected by bottom half
suppression.  I have upgraded write_lock(&pt->lock) to
write_lock_bh(&pt->lock).  The read_lock is executed at bottom
half anyway.

So, what I did was: (First hunk gets rid of the unnecessary
local_irq_disable, the rest get rid of the race on pt->lock.)

Index: ldl.c
===================================================================
RCS file: /u2/cvsroot/strxns/src/drivers/ldl.c,v
retrieving revision 0.9.2.16
diff -r0.9.2.16 ldl.c
446a447,454
> #ifdef LIS
> #undef SPLSTR
> #undef SPLX
> STATIC spinlock_t ldl_spin_lock = SPIN_LOCK_UNLOCKED;
> #define SPLSTR(__psw) while(0){ (void)(__psw); spin_lock(&ldl_spin_lock);   }
> #define SPLX(__psw)   while(0){ (void)(__psw); spin_unlock(&ldl_spin_lock); }
> #endif
> 
736c744
< 		write_lock(&pt->lock);
---
> 		write_lock_bh(&pt->lock);
739c747
< 		write_unlock(&pt->lock);
---
> 		write_unlock_bh(&pt->lock);
770c778
< 	write_lock(&pt->lock);
---
> 	write_lock_bh(&pt->lock);
773c781
< 		write_unlock(&pt->lock);
---
> 		write_unlock_bh(&pt->lock);
798c806
< 				write_unlock(&pt->lock);
---
> 				write_unlock_bh(&pt->lock);
807c815
< 		write_unlock(&pt->lock);
---
> 		write_unlock_bh(&pt->lock);

If someone wants to go through an improve the granularity over
one big ldl_spin_lock, please go ahead.


--brian


On Mon, 04 Apr 2005, Hai Zaar wrote:

> > > 1) Calling sap_create() under lock. The problem is that sap_create()
> > > might invoke dev_add_pack() that can sleep, since kernel 2.5.70. I
> > > choose to remove the call to dev_add_pack() from sap_create() and make
> 
> > dev_add_pack does not sleep.
> 
> I mixed up the reason for the change, because I've done several
> changes at once. I still think the patch is right. dev_add_pack()
> definitly does not sleep, but still, it cannot be called with IRQs
> disabled, the reason is the following : dev_add_pack() uses bh-safe
> spinlocks, spin_unlock_bh() in turn uses local_bh_enable() which
> complains loudly when IRQs are disabled (At least in miy 2.6.7
> kernel). And in ldl drivers IRQs are disabled after SPLSTR is done,
> because LiS uses irq-safe spinlocks for it. So I still think that my
> fixes for dev_add_pack apply.
> 
> 
> 
> > > 4) Doing spin_unlock_bh() while holding irq-disabling lock. I choose
> > > to change the locking to be irq-safe, that does not have that problem.
> > > Due to my limited understaing of Linux kernel locking rules, I'm not
> > > completely sure it is correct. I thing that disabling local irqs
> > > guarantees that no bh processing is done, but I might be wrong.
> 
> > irq locking is too severe.
> 
> I'm well aware of that. Still, the change to use IRQ safe locking is
> donw for the same reason as the previous change. spin_unlock_bh()
> complains when called with IRQs disabled. But the actual performance
> hit of converting to spinlock_irqsafe will be minimal, because SPLSTR
> had already disabled IRQs by the time we'd want to lock.
> 
> -- 
> Zaar
> _______________________________________________
> Linux-streams mailing list
> [email protected]
> http://gsyc.escet.urjc.es/mailman/listinfo/linux-streams

-- 
Brian F. G. Bidulock    ¦ The reasonable man adapts himself to the ¦
[email protected]    ¦ world; the unreasonable one persists in  ¦
http://www.openss7.org/ ¦ trying  to adapt the  world  to himself. ¦
                        ¦ Therefore  all  progress  depends on the ¦
                        ¦ unreasonable man. -- George Bernard Shaw ¦