Re: LiS ldl driver with 2.6 kernels

Hai,

On Sun, 03 Apr 2005, Hai Zaar wrote:

>     Hello!
>     I'm using the ldl driver module in LiS-2.18 on 2.6.7 Linux kernel
> and I've had various warnings in the kernel log due to improper
> locking in the driver. The problems are easiliy reproducible using
> ldltest that is supplied with LiS. I have fixed all of those problems,
> but due to my limited undestanding of both LiS and kernel locking and
> internals, I'd be glad if someone who actually understands both
> reviews my patches.
> 
>     The locking fixes patch, fixes the following problems:
> 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.

> sap_create() return pointer to struct packet_type to the caller, that
> will invoke dev_add_pack() after unlocking. It seem that attaching
> packet device later cannot do any harm.
> 2) Calling sleeping dev_remove_pack() in sap_destory(), while
> sap_destroy() holds lock. The fix is to delay calling
> dev_remove_pack() until unlock is done. During the call sap_destroy()
> dl->state is either DL_UNBIND_PENDING  or DL_SUBS_UNBIND_PENDING so
> delaying unregistration seems to be safe to me.
> 3) Calling sap_destroy_all() while holding lock. The change to to make
> dl->state to be DL_UNBIND_PENDING, unlock, do the call and re-lock.
> I'm not completely sure if it is safe, I was just blindnly copying the
> code that already existed for one of the invocations of
> sap_destroy_all().

Simple enough to remove locks around this function.  The locks are not
protecting this.

> 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.

>  
>     Another small off-subject patch is attached, it fixes problematic
> uses of HZ. We're running here wigth HZ==2000, thus 1000/HZ is always
> zero (each timer tick is less than 1msec apart from the previous one).
> I've converted all users of HZ to jiffie/ms/jiffie conversion routines
> of the kernell

I already have the 2.16.18 time patch included in 2.18.1.

> 
>     Regards,
>        Zaar
> 
> P.S. It's kind of strange that nobody noticed those problems because
> they appeared during 2.5.70, when the networking code was coverted
> from using brlocks to RCU. It was the change that made dev_add_pack()

dev_add_pack does not sleep.

> and dev_remove_pack() to be sleeping functions. Maybe the problem was
> raised on this list, but the web archive of it is unavailable at the
> present.

The change was panic code for sleeping allocations while locks held.


I  have patched as follows:


Index: /home/brian/os8/strxns/src/drivers/ldl.c
===================================================================
RCS file: /u2/cvsroot/strxns/src/drivers/ldl.c,v
retrieving revision 0.9.2.16
diff -u -r0.9.2.16 ldl.c
--- /home/brian/os8/strxns/src/drivers/ldl.c	1 Apr 2005 06:23:27 -0000	0.9.2.16
+++ /home/brian/os8/strxns/src/drivers/ldl.c	4 Apr 2005 00:54:13 -0000
@@ -938,7 +938,9 @@
 	 *  should come from this endpoint.
 	 */
 	if (dl->dlstate == DL_IDLE) {
+		SPLX(psw);
 		sap_destroy_all(dl);
+		SPLSTR(psw);
 		dl->dlstate = DL_UNBOUND;
 	}
 	if (dl->dlstate == DL_UNBOUND) {
@@ -4282,7 +4284,9 @@
 	}
 
 	if (dl->dlstate == DL_IDLE) {
+		SPLX(psw);
 		sap_destroy_all(dl);
+		SPLSTR(psw);
 		dl->dlstate = DL_UNBOUND;
 	}
 	if (dl->dlstate != DL_UNATTACHED) {


-- 
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 ¦