Re: any chance of 2.6.0-test*?

Re: any chance of 2.6.0-test*?

[Andi Kleen]

Alan Cox <alan@lxorguk.ukuu.org.uk> writes:

> On Fri, 2003-01-10 at 16:10, William Lee Irwin III wrote:
> > Any specific concerns/issues/wishlist items you want taken care of
> > before doing it or is it a "generalized comfort level" kind of thing?
> > Let me know, I'd be much obliged for specific directions to move in.
> 
> IDE is all broken still and will take at least another three months to
> fix - before we get to 'improve'. The entire tty layer locking is terminally

Can you quickly summarize what is broken with IDE ?

Are just some low level drivers broken or are there some generic
nasty problems.

If it is just some broken low level drivers I guess they can 
be marked dangerous or CONFIG_EXPERIMENTAL.

How does it differ from the code that was just merged into 2.4.21pre3
(has the later all the problems fixed?)

> broken and nobody has even started fixing it. Just try a mass of parallel
> tty/pty activity . It was problematic before, pre-empt has taken it  to dead, 
> defunct and buried. 

Can someone shortly describe what is the main problem with TTY?

>From what I can see the high level tty code mostly takes lock_kernel
before doing anything.
On reads access to file->private_data is not serialized, but it at 
least shouldn't go away because VFS takes care of struct file
reference counting.

The tty_drivers list does seem to need a spinlock, but I guess
just taking lock_kernel in tty_open would fix that for now.

[i didn't look at low level ldiscs]

Any particular test cases that break ?

If yes I would recommend to post them as scripts and their oopses so 
that people can start working on them.


The appended untested patch adds some lock_kernel()s that appear to be missing
to tty_io.c. The rest seems to already run under BKL or not access
any global data
(except tty_paranoia_check, but is probably ok with the reference counting
in the VFS)

> 
> Most of the drivers still don't build either.

In UP most did last time I tried.
On SMP a lot of problems are caused by the cli removal

My personal (i386) problem list is relatively short.
I use used 2.5.54 on my desktop without any problems (without preempt)

- BIO still oopses when XFS tries replay a log on RAID-0

-Andi

--- linux-2.5.56-work/drivers/char/tty_io.c-o	2003-01-02 05:13:12.000000000 +0100
+++ linux-2.5.56-work/drivers/char/tty_io.c	2003-01-11 13:23:15.000000000 +0100
@@ -1329,6 +1329,8 @@
 		int major, minor;
 		struct tty_driver *driver;
 
+		lock_kernel(); 
+
 		/* find a device that is not in use. */
 		retval = -1;
 		for ( major = 0 ; major < UNIX98_NR_MAJORS ; major++ ) {
@@ -1340,6 +1342,8 @@
 				if (!init_dev(device, &tty)) goto ptmx_found; /* ok! */
 			}
 		}
+
+		unlock_kernel();
 		return -EIO; /* no free ptys */
 	ptmx_found:
 		set_bit(TTY_PTY_LOCK, &tty->flags); /* LOCK THE SLAVE */
@@ -1357,6 +1361,8 @@
 #endif  /* CONFIG_UNIX_98_PTYS */
 	}
 
+	lock_kernel();
+
 	retval = init_dev(device, &tty);
 	if (retval)
 		return retval;
@@ -1389,6 +1395,8 @@
 #endif
 
 		release_dev(filp);
+
+		unlock_kernel(); 
 		if (retval != -ERESTARTSYS)
 			return retval;
 		if (signal_pending(current))
@@ -1397,6 +1405,7 @@
 		/*
 		 * Need to reset f_op in case a hangup happened.
 		 */
+		lock_kernel();
 		filp->f_op = &tty_fops;
 		goto retry_open;
 	}
@@ -1424,6 +1433,7 @@
 			nr_warns++;
 		}
 	}
+	unlock_kernel();
 	return 0;
 }
 
@@ -1444,8 +1454,13 @@
 	if (tty_paranoia_check(tty, filp->f_dentry->d_inode->i_rdev, "tty_poll"))
 		return 0;
 
-	if (tty->ldisc.poll)
-		return (tty->ldisc.poll)(tty, filp, wait);
+	if (tty->ldisc.poll) { 
+		int ret;
+		lock_kernel();
+		ret = (tty->ldisc.poll)(tty, filp, wait);
+		unlock_kernel();
+		return ret;
+	}
 	return 0;
 }
 

Re: any chance of 2.6.0-test*?

[Russell King]

> --- linux-2.5.56-work/drivers/char/tty_io.c-o	2003-01-02 05:13:12.000000000 +0100
> +++ linux-2.5.56-work/drivers/char/tty_io.c	2003-01-11 13:23:15.000000000 +0100
> @@ -1329,6 +1329,8 @@
>  		int major, minor;
>  		struct tty_driver *driver;
>  
> +		lock_kernel(); 
> +

Deadlock.  chrdev_open() calls lock_kernel() and then the fops->open
method, which is tty_open().

>  		/* find a device that is not in use. */
>  		retval = -1;
>  		for ( major = 0 ; major < UNIX98_NR_MAJORS ; major++ ) {
> @@ -1340,6 +1342,8 @@
>  				if (!init_dev(device, &tty)) goto ptmx_found; /* ok! */
>  			}
>  		}
> +
> +		unlock_kernel();
>  		return -EIO; /* no free ptys */
>  	ptmx_found:
>  		set_bit(TTY_PTY_LOCK, &tty->flags); /* LOCK THE SLAVE */
> @@ -1357,6 +1361,8 @@
>  #endif  /* CONFIG_UNIX_98_PTYS */
>  	}
>  
> +	lock_kernel();
> +

Deadlock.  See chrdev_open() note above.

>  	retval = init_dev(device, &tty);
>  	if (retval)
>  		return retval;
> @@ -1389,6 +1395,8 @@
>  #endif
>  
>  		release_dev(filp);
> +
> +		unlock_kernel(); 
>  		if (retval != -ERESTARTSYS)
>  			return retval;
>  		if (signal_pending(current))
> @@ -1397,6 +1405,7 @@
>  		/*
>  		 * Need to reset f_op in case a hangup happened.
>  		 */
> +		lock_kernel();

Deadlock.  See chrdev_open() note above.

>  		filp->f_op = &tty_fops;
>  		goto retry_open;
>  	}
> @@ -1424,6 +1433,7 @@
>  			nr_warns++;
>  		}
>  	}
> +	unlock_kernel();
>  	return 0;
>  }
>  
> @@ -1444,8 +1454,13 @@
>  	if (tty_paranoia_check(tty, filp->f_dentry->d_inode->i_rdev, "tty_poll"))
>  		return 0;
>  
> -	if (tty->ldisc.poll)
> -		return (tty->ldisc.poll)(tty, filp, wait);
> +	if (tty->ldisc.poll) { 
> +		int ret;
> +		lock_kernel();
> +		ret = (tty->ldisc.poll)(tty, filp, wait);
> +		unlock_kernel();
> +		return ret;
> +	}

This one needs deeper review.

-- 
Russell King (rmk@arm.linux.org.uk)                The developer of ARM Linux
             http://www.arm.linux.org.uk/personal/aboutme.html

Re: any chance of 2.6.0-test*?

[Andi Kleen]

On Sat, Jan 11, 2003 at 02:01:51PM +0100, Russell King wrote:
> > --- linux-2.5.56-work/drivers/char/tty_io.c-o	2003-01-02 05:13:12.000000000 +0100
> > +++ linux-2.5.56-work/drivers/char/tty_io.c	2003-01-11 13:23:15.000000000 +0100
> > @@ -1329,6 +1329,8 @@
> >  		int major, minor;
> >  		struct tty_driver *driver;
> >  
> > +		lock_kernel(); 
> > +
> 
> Deadlock.  chrdev_open() calls lock_kernel() and then the fops->open
> method, which is tty_open().

No problem, lock_kernel is recursive and dropped on schedule.

It is very very hard to get a BKL deadlock.

> This one needs deeper review.

I agree, but one has to start somewhere. Please submit any fixes,
perhaps we can take then close these issues for good.

Was looking at n_tty.c now, looks like it has some more race 
problems.


-Andi

Re: any chance of 2.6.0-test*?

[Andi Kleen]

> The main problems are
>   - Incorrect locking all over the place

Hmm bad. Is it that hard to fix ?

>   - Incorrect timings on some phases

Can't you just take out the timings in that case? 
My (not very informed) understanding is: 
everything should work with the BIOS timings and generic IDE,
having own timings is just an optimization to squeeze out a bit
more speed.

>   - ISAPnP IDE doesn't work right now

Hardly a release show stopper I guess

(ISA support in general being rather broken, but I doubt many people care
still) 

>   - Flaws in error recovery paths in certain situations
>   - Lots of random oopses on boot/remove that were apparently
>     introduced by the kobject/sysfs people and need chasing
>     down. (There are some non sysfs ones mostly fixed)

I guess the kobject/sysfs stuff could be ripped out if it doesn't
work - it is probably not a "must have" feature.


>   - ide-scsi needs some cleanup to fix switchover ide-cd/scsi
>     (We can't dump ide-scsi)
>   - Unregister path has races which cause all the long
>     standing problems with pcmcia and prevents pci unreg

Can't you just disable module unloading for the release ?
(not nice, but has worked for other subsystems like IPv6 too in the past)
Also 2.4 didn't have much problems without modular IDE, 2.6 will
be probably able to tolerate it too.

>   - IDE raid hasn't been ported to 2.5 at all yet

Vendor problem ?
> 
> > > broken and nobody has even started fixing it. Just try a mass of parallel
> > > tty/pty activity . It was problematic before, pre-empt has taken it  to dead, 
> > > defunct and buried. 
> > 
> > Can someone shortly describe what is the main problem with TTY?
> > 
> > >From what I can see the high level tty code mostly takes lock_kernel
> > before doing anything.
> 
> Which works really well with all the IRQ paths on it

Then 2.0/2.2/2.4 would have been racy too :-) Apparently they worked though.

High level tty code doesn't touch any ttys as far as I can see.

It is done in n_tty.c, which apparently needs a bit of work
(found some races together with Rik there)

> > On reads access to file->private_data is not serialized, but it at 
> > least shouldn't go away because VFS takes care of struct file
> > reference counting.
> 
> The refcounting bugs are/were in the ldisc stuff. I thought the
> bluetooth folks (Max and co) fixed that one

I was thinking about races with tty_release (= own tty_struct suddenly
going away). VFS would prevent that.


-Andi

Re: any chance of 2.6.0-test*?

[Alan Cox]

On Sat, 2003-01-11 at 12:27, Andi Kleen wrote:
> Can you quickly summarize what is broken with IDE ?
> Are just some low level drivers broken or are there some generic
> nasty problems.
> 
> If it is just some broken low level drivers I guess they can 
> be marked dangerous or CONFIG_EXPERIMENTAL.

Low level drivers are basically sorted.

The main problems are
  - Incorrect locking all over the place
  - Incorrect timings on some phases
  - Some ioctls can cause crashes due to locking
  - ISAPnP IDE doesn't work right now
  - Flaws in error recovery paths in certain situations
  - Lots of random oopses on boot/remove that were apparently
    introduced by the kobject/sysfs people and need chasing
    down. (There are some non sysfs ones mostly fixed)
  - ide-scsi needs some cleanup to fix switchover ide-cd/scsi
    (We can't dump ide-scsi)
  - Unregister path has races which cause all the long
    standing problems with pcmcia and prevents pci unreg
  - PCI IDE driver registration needs busy checks
  - PCI layer needs some stuff from 2.4
  - PCI layer in 2.4/2.5 needs an IRQ bug fixing
  - ACPI doesn't seem to handle compatibility IRQ mode
  - We don't handle a few errata (MWDMA on 450NX for example)
  - IDE raid hasn't been ported to 2.5 at all yet

Thats off the top of my head right now.

> How does it differ from the code that was just merged into 2.4.21pre3
> (has the later all the problems fixed?)

No, although some don't show up in the same ways in 2.4 - eg the pcmcia
unload race is rare in 2.4 for other reasons. Endianism should all
be cured in 2.4, and I sent that to Linus. The PCI i/o assignment code
in 2.4 is done. I hope to have some of the locking and also the timing
path work Ross Biro has done in for 2.4.22 proper

> > broken and nobody has even started fixing it. Just try a mass of parallel
> > tty/pty activity . It was problematic before, pre-empt has taken it  to dead, 
> > defunct and buried. 
> 
> Can someone shortly describe what is the main problem with TTY?
> 
> >From what I can see the high level tty code mostly takes lock_kernel
> before doing anything.

Which works really well with all the IRQ paths on it

> On reads access to file->private_data is not serialized, but it at 
> least shouldn't go away because VFS takes care of struct file
> reference counting.

The refcounting bugs are/were in the ldisc stuff. I thought the
bluetooth folks (Max and co) fixed that one

If we can lock_kernel the tty layer for now (I'm bothered about
the ldisc end of tht which is IRQ context) then great, tty scaling
is suddenelly a 2.7 problem.

Re: any chance of 2.6.0-test*?

[Andi Kleen]

On Sat, Jan 11, 2003 at 04:31:00PM +0100, Alan Cox wrote:
> > >   - ide-scsi needs some cleanup to fix switchover ide-cd/scsi
> > >     (We can't dump ide-scsi)
> > >   - Unregister path has races which cause all the long
> > >     standing problems with pcmcia and prevents pci unreg
> > 
> > Can't you just disable module unloading for the release ?
> 
> Only if I can also nail shut your PCMCIA slot, disallow SATA and remove
> some ioctls people use for docking.

Hmm? Didn't that all work in 2.4 with monolithic IDE ?

-Andi

Re: any chance of 2.6.0-test*?

[Alan Cox]

On Sat, 2003-01-11 at 14:06, Andi Kleen wrote:
> > The main problems are
> >   - Incorrect locking all over the place
> 
> Hmm bad. Is it that hard to fix ?

Very. Just read the settings/proc code for an example

> >   - Incorrect timings on some phases
> 
> Can't you just take out the timings in that case? 
> My (not very informed) understanding is: 
> everything should work with the BIOS timings and generic IDE,
> having own timings is just an optimization to squeeze out a bit
> more speed.

These are timing for phases not drive timings. Drive timings from
the BIOS are also frequently questionable. These have to be fixed
and as boxes get faster it becomes more important they are

> >   - Lots of random oopses on boot/remove that were apparently
> >     introduced by the kobject/sysfs people and need chasing
> >     down. (There are some non sysfs ones mostly fixed)
> 
> I guess the kobject/sysfs stuff could be ripped out if it doesn't
> work - it is probably not a "must have" feature.

Without a doubt. 

> >   - ide-scsi needs some cleanup to fix switchover ide-cd/scsi
> >     (We can't dump ide-scsi)
> >   - Unregister path has races which cause all the long
> >     standing problems with pcmcia and prevents pci unreg
> 
> Can't you just disable module unloading for the release ?

Only if I can also nail shut your PCMCIA slot, disallow SATA and remove
some ioctls people use for docking.

> >   - IDE raid hasn't been ported to 2.5 at all yet
> 
> Vendor problem ?

It needs a rewrite to the new bio layer or rewriting to use the device
mapper, which is a distinct option here.


> > Which works really well with all the IRQ paths on it
> 
> Then 2.0/2.2/2.4 would have been racy too :-) Apparently they worked though.

Long ago lock_kernel had meaning against IRQ paths. TTY never caught up.

Re: any chance of 2.6.0-test*?

[Alan Cox]

On Sat, 2003-01-11 at 15:25, Andi Kleen wrote:
> On Sat, Jan 11, 2003 at 04:31:00PM +0100, Alan Cox wrote:
> > > >   - ide-scsi needs some cleanup to fix switchover ide-cd/scsi
> > > >     (We can't dump ide-scsi)
> > > >   - Unregister path has races which cause all the long
> > > >     standing problems with pcmcia and prevents pci unreg
> > > 
> > > Can't you just disable module unloading for the release ?
> > 
> > Only if I can also nail shut your PCMCIA slot, disallow SATA and remove
> > some ioctls people use for docking.
> 
> Hmm? Didn't that all work in 2.4 with monolithic IDE ?

The pcmcia stuff crashes erratically with the old IDE the bugs have not
changed but the problem now shows up far more often. With a 2Gb type II
PCMCIA IDE disk I get a crash about 1 in 3 ejects now.

Fixing the tty layer was Re: any chance of 2.6.0-test*?

[Andi Kleen]

[forgot to cc linux-kernel in the first try, sorry if you see it twice]

Greg KH <greg@kroah.com> writes:

> On Fri, Jan 10, 2003 at 05:19:08PM +0000, Alan Cox wrote:
> > The entire tty layer locking is terminally broken and nobody has even
> > started fixing it. Just try a mass of parallel tty/pty activity . It
> > was problematic before, pre-empt has taken it  to dead, defunct and
> > buried. 
> 
> I've looked into this, and wow, it's not a simple fix :(

it has to be fixed for 2.6, no argument.

I took a look at it. I think the easiest strategy would be:

- Make sure all the process context code holds BKL
(most of it does, but not all - sometimes it is buggy like in 
disassociate_tty) 
I have some patches for that for tty_io.c at least

The local_irq_save in there are buggy, they need to take 
a lock.

- Audit the data structures that are touched by interrupts
and add spinlocks.
At least for n_tty.c probably just tty->read_lock needs to be 
extended.
Perhaps some can be just "fixed" by ignoring latency and pushing
softirq functions into keventd
(modern CPUs should be fast enough for that) 

- Possibly disable module unloading for ldiscs (seems to be rather broken,
although Rusty's new unload algorithm may avoid the worst - not completely
sure)

Probably all doable with some concentrated effort.

Anyone interested in helping ?

-Andi

Re: any chance of 2.6.0-test*?

[Paul Mackerras]

Alan Cox writes:

> The pcmcia stuff crashes erratically with the old IDE the bugs have not
> changed but the problem now shows up far more often. With a 2Gb type II
> PCMCIA IDE disk I get a crash about 1 in 3 ejects now.

I'm using the patch below, which makes sure that ide_release (in
ide-cs.c) runs in process context not interrupt context.  The specific
problem I saw was that ide_unregister calls various devfs routines
that don't like being called in interrupt context, but there may well
be other thing ide_unregister does that aren't safe in interrupt
context.

I am also hitting another problem on my powerbook that I would like
your advice on.  The thing is that my powerbook (a 500MHz G4 titanium
powerbook) has 3 IDE interfaces, but only the first two have anything
attached.  The pmac IDE driver goes through and initializes all three,
including setting hwif->selectproc for each one.  Then, when I plug in
a CF card in the pcmcia/cardbus slot, ide_register_hw goes and looks
for a free ide_hwifs[] entry.  Slot 2 appears to be available because
there are no devices attached, and so it uses that.  In the process of
setting up ide_hwifs[2], it sets hwif->io_ports but it doesn't reset
selectproc (or hwif->OUTB etc., AFAICS).  The result is that it calls
the pmac IDE selectproc, which crashes (since we munged hwif->io_ports
without telling it).

I have "fixed" the problem for now by adding a call to
init_hwif_data(index) in ide_register_hw just before the first
memcpy.  I'd be interested to know what the right fix is. :)

Regards,
Paul.

diff -urN linux-2.4/drivers/ide/legacy/ide-cs.c pmac/drivers/ide/legacy/ide-cs.c
--- linux-2.4/drivers/ide/legacy/ide-cs.c	2002-12-19 11:50:35.000000000 +1100
+++ pmac/drivers/ide/legacy/ide-cs.c	2003-01-13 14:02:31.000000000 +1100
@@ -92,10 +92,11 @@
     int		ndev;
     dev_node_t	node;
     int		hd;
+    struct tq_struct rel_task;
 } ide_info_t;
 
 static void ide_config(dev_link_t *link);
-static void ide_release(u_long arg);
+static void ide_release(void *arg);
 static int ide_event(event_t event, int priority,
 		     event_callback_args_t *args);
 
@@ -136,9 +137,8 @@
     if (!info) return NULL;
     memset(info, 0, sizeof(*info));
     link = &info->link; link->priv = info;
+    INIT_TQUEUE(&info->rel_task, ide_release, link);
 
-    link->release.function = &ide_release;
-    link->release.data = (u_long)link;
     link->io.Attributes1 = IO_DATA_PATH_WIDTH_AUTO;
     link->io.Attributes2 = IO_DATA_PATH_WIDTH_8;
     link->io.IOAddrLines = 3;
@@ -187,6 +187,7 @@
 static void ide_detach(dev_link_t *link)
 {
     dev_link_t **linkp;
+    ide_info_t *info = link->priv;
     int ret;
 
     DEBUG(0, "ide_detach(0x%p)\n", link);
@@ -197,9 +198,10 @@
     if (*linkp == NULL)
 	return;
 
-    del_timer(&link->release);
-    if (link->state & DEV_CONFIG)
-	ide_release((u_long)link);
+    if (link->state & DEV_CONFIG) {
+	schedule_task(&info->rel_task);
+	flush_scheduled_tasks();
+    }
     
     if (link->handle) {
 	ret = CardServices(DeregisterClient, link->handle);
@@ -209,7 +211,7 @@
     
     /* Unlink, free device structure */
     *linkp = link->next;
-    kfree(link->priv);
+    kfree(info);
     
 } /* ide_detach */
 
@@ -381,7 +383,7 @@
 cs_failed:
     cs_error(link->handle, last_fn, last_ret);
 failed:
-    ide_release((u_long)link);
+    ide_release(link);
 
 } /* ide_config */
 
@@ -393,12 +395,15 @@
     
 ======================================================================*/
 
-void ide_release(u_long arg)
+static void ide_release(void *arg)
 {
-    dev_link_t *link = (dev_link_t *)arg;
+    dev_link_t *link = arg;
     ide_info_t *info = link->priv;
     
-    DEBUG(0, "ide_release(0x%p)\n", link);
+    if (!(link->state & DEV_CONFIG))
+	return;
+
+    DEBUG(0, "ide_do_release(0x%p)\n", link);
 
     if (info->ndev) {
         /* FIXME: if this fails we need to queue the cleanup somehow
@@ -435,6 +440,7 @@
 	      event_callback_args_t *args)
 {
     dev_link_t *link = args->client_data;
+    ide_info_t *info = link->priv;
 
     DEBUG(1, "ide_event(0x%06x)\n", event);
     
@@ -442,7 +448,7 @@
     case CS_EVENT_CARD_REMOVAL:
 	link->state &= ~DEV_PRESENT;
 	if (link->state & DEV_CONFIG)
-	    mod_timer(&link->release, jiffies + HZ/20);
+	    schedule_task(&info->rel_task);
 	break;
     case CS_EVENT_CARD_INSERTION:
 	link->state |= DEV_PRESENT | DEV_CONFIG_PENDING;

Re: Fixing the tty layer was Re: any chance of 2.6.0-test*?

[Dipankar Sarma]

On Sun, Jan 12, 2003 at 09:59:15AM +0000, Andi Kleen wrote:
> > I've looked into this, and wow, it's not a simple fix :(

Oh, yes, I have spent hours and hours trying to untangle tty locking
and it isn't simple.

> 
> it has to be fixed for 2.6, no argument.
> 
> I took a look at it. I think the easiest strategy would be:
> 
> - Make sure all the process context code holds BKL
> (most of it does, but not all - sometimes it is buggy like in 
> disassociate_tty) 
> I have some patches for that for tty_io.c at least

What does that BKL protect ? I can't seem to ever figure our if
all the races are plugged or not.

> 
> The local_irq_save in there are buggy, they need to take 
> a lock.

Also a locking model w.r.t. the serial drivers ?

> 
> - Audit the data structures that are touched by interrupts
> and add spinlocks.
> At least for n_tty.c probably just tty->read_lock needs to be 
> extended.
> Perhaps some can be just "fixed" by ignoring latency and pushing
> softirq functions into keventd
> (modern CPUs should be fast enough for that) 
> 
> - Possibly disable module unloading for ldiscs (seems to be rather broken,
> although Rusty's new unload algorithm may avoid the worst - not completely
> sure)
> 
> Probably all doable with some concentrated effort.
> 
> Anyone interested in helping ?

Yes, I would like to help out. I was hoping to help rewrite the whole
thing in 2.7, but it needs help *now*. Perhaps I can take your list
of things to do and fix them as a starting point ?

Thanks
Dipankar

Re: Fixing the tty layer was Re: any chance of 2.6.0-test*?

[Greg KH]

On Sun, Jan 12, 2003 at 10:56:37AM +0100, Andi Kleen wrote:
> 
> - Possibly disable module unloading for ldiscs (seems to be rather broken,
> although Rusty's new unload algorithm may avoid the worst - not completely
> sure)

Max has a patch for this, to add struct module * to a ldisc.  Don't know
if it made it in or not.

> Probably all doable with some concentrated effort.
> 
> Anyone interested in helping ?

I am.

thanks,

greg k-h

Re: Fixing the tty layer was Re: any chance of 2.6.0-test*?

[Andi Kleen]

On Mon, Jan 13, 2003 at 07:41:31AM +0100, Dipankar Sarma wrote:
> On Sun, Jan 12, 2003 at 09:59:15AM +0000, Andi Kleen wrote:
> > > I've looked into this, and wow, it's not a simple fix :(
> 
> Oh, yes, I have spent hours and hours trying to untangle tty locking
> and it isn't simple.

Oops. Could you quickly summarize your findings so far ?

> 
> > 
> > it has to be fixed for 2.6, no argument.
> > 
> > I took a look at it. I think the easiest strategy would be:
> > 
> > - Make sure all the process context code holds BKL
> > (most of it does, but not all - sometimes it is buggy like in 
> > disassociate_tty) 
> > I have some patches for that for tty_io.c at least
> 
> What does that BKL protect ? I can't seem to ever figure our if
> all the races are plugged or not.

Well, one has to start somewhere. Just starting by plugging most of the
obvious races, then the more subtle ones can be attacked later.

The idea of the BKL was to protect the protect context code against
itself (code lock) and also the few global data structures that 
are only accessed from process context (like the tty drivers list)


> 
> > 
> > The local_irq_save in there are buggy, they need to take 
> > a lock.
> 
> Also a locking model w.r.t. the serial drivers ?

It would be better to encapsulate the locking in the ndisc/tty layer
IMHO, otherwise too much code needs to be audited.

I have not studied the dependencies to low level serial in too close
detail, so I am not sure how difficult that is.

> 
> > 
> > - Audit the data structures that are touched by interrupts
> > and add spinlocks.
> > At least for n_tty.c probably just tty->read_lock needs to be 
> > extended.
> > Perhaps some can be just "fixed" by ignoring latency and pushing
> > softirq functions into keventd
> > (modern CPUs should be fast enough for that) 
> > 
> > - Possibly disable module unloading for ldiscs (seems to be rather broken,
> > although Rusty's new unload algorithm may avoid the worst - not completely
> > sure)
> > 
> > Probably all doable with some concentrated effort.
> > 
> > Anyone interested in helping ?
> 
> Yes, I would like to help out. I was hoping to help rewrite the whole
> thing in 2.7, but it needs help *now*. Perhaps I can take your list
> of things to do and fix them as a starting point ?

I attached my current patch, it isn't too well tested however and needs
more work.

Mostly just adds lock_kernel()s to the high level code so far and a few comments.

-Andi


diff -u linux-2.5.56-work/drivers/char/tty_io.c-o linux-2.5.56-work/drivers/char/tty_io.c
--- linux-2.5.56-work/drivers/char/tty_io.c-o	2003-01-02 05:13:12.000000000 +0100
+++ linux-2.5.56-work/drivers/char/tty_io.c	2003-01-12 12:47:19.000000000 +0100
@@ -114,7 +114,12 @@
 #define CHECK_TTY_COUNT 1
 
 struct termios tty_std_termios;		/* for the benefit of tty drivers  */
+
+/* protected by the BKL */
 LIST_HEAD(tty_drivers);			/* linked list of tty drivers */
+
+/* broken locking correctly (FIXME).
+   Due to static allocation races are relatively harmless. */
 struct tty_ldisc ldiscs[NR_LDISCS];	/* line disc dispatch table	*/
 
 #ifdef CONFIG_UNIX98_PTYS
@@ -281,6 +286,9 @@
 		sprintf(modname, "tty-ldisc-%d", ldisc);
 		request_module (modname);
 	}
+
+	/* module unload race with LDISCs here */
+
 	if (!(ldiscs[ldisc].flags & LDISC_FLAG_DEFINED))
 		return -EINVAL;
 
@@ -322,6 +330,7 @@
 
 /*
  * This routine returns a tty driver structure, given a device number
+ * Called with BKL held.
  */
 struct tty_driver *get_tty_driver(kdev_t device)
 {
@@ -350,14 +359,21 @@
  */
 int tty_check_change(struct tty_struct * tty)
 {
-	if (current->tty != tty)
+	lock_kernel(); 
+	if (current->tty != tty) { 
+		unlock_kernel();
 		return 0;
+	}
 	if (tty->pgrp <= 0) {
+		unlock_kernel();
 		printk(KERN_WARNING "tty_check_change: tty->pgrp <= 0!\n");
 		return 0;
 	}
-	if (current->pgrp == tty->pgrp)
+	if (current->pgrp == tty->pgrp) { 
+		unlock_kernel();
 		return 0;
+	}
+	unlock_kernel(); 
 	if (is_ignored(SIGTTOU))
 		return 0;
 	if (is_orphaned_pgrp(current->pgrp))
@@ -568,17 +584,19 @@
  *
  * The argument on_exit is set to 1 if called when a process is
  * exiting; it is 0 if called by the ioctl TIOCNOTTY.
+ * 
+ * Can be called without BKL from do_exit.
  */
 void disassociate_ctty(int on_exit)
 {
-	struct tty_struct *tty = current->tty;
+	struct tty_struct *tty;
 	struct task_struct *p;
 	struct list_head *l;
 	struct pid *pid;
 	int tty_pgrp = -1;
 
 	lock_kernel();
-
+	tty = current->tty;
 	if (tty) {
 		tty_pgrp = tty->pgrp;
 		if (on_exit && tty->driver.type != TTY_DRIVER_TYPE_PTY)
@@ -1284,6 +1302,8 @@
  *
  * The termios state of a pty is reset on first open so that
  * settings don't persist across reuse.
+ * 
+ * Called with BKL hold.
  */
 static int tty_open(struct inode * inode, struct file * filp)
 {
@@ -1340,6 +1360,7 @@
 				if (!init_dev(device, &tty)) goto ptmx_found; /* ok! */
 			}
 		}
+
 		return -EIO; /* no free ptys */
 	ptmx_found:
 		set_bit(TTY_PTY_LOCK, &tty->flags); /* LOCK THE SLAVE */
@@ -1389,6 +1410,7 @@
 #endif
 
 		release_dev(filp);
+
 		if (retval != -ERESTARTSYS)
 			return retval;
 		if (signal_pending(current))
@@ -1444,8 +1466,13 @@
 	if (tty_paranoia_check(tty, filp->f_dentry->d_inode->i_rdev, "tty_poll"))
 		return 0;
 
-	if (tty->ldisc.poll)
-		return (tty->ldisc.poll)(tty, filp, wait);
+	if (tty->ldisc.poll) { 
+		int ret;
+		lock_kernel();
+		ret = (tty->ldisc.poll)(tty, filp, wait);
+		unlock_kernel();
+		return ret;
+	}
 	return 0;
 }
 
@@ -1669,6 +1697,8 @@
 
 /*
  * Split this up, as gcc can choke on it otherwise..
+ * 
+ * Called with BKL hold.
  */
 int tty_ioctl(struct inode * inode, struct file * file,
 	      unsigned int cmd, unsigned long arg)
@@ -1919,7 +1949,7 @@
 		fp = tty->flip.flag_buf + TTY_FLIPBUF_SIZE;
 		tty->flip.buf_num = 0;
 
-		local_irq_save(flags); // FIXME: is this safe?
+		spin_lock_irqsave(&tty->read_lock, flags); 
 		tty->flip.char_buf_ptr = tty->flip.char_buf;
 		tty->flip.flag_buf_ptr = tty->flip.flag_buf;
 	} else {
@@ -1927,13 +1957,14 @@
 		fp = tty->flip.flag_buf;
 		tty->flip.buf_num = 1;
 
-		local_irq_save(flags); // FIXME: is this safe?
+		spin_lock_irqsave(&tty->read_lock, flags); 
 		tty->flip.char_buf_ptr = tty->flip.char_buf + TTY_FLIPBUF_SIZE;
 		tty->flip.flag_buf_ptr = tty->flip.flag_buf + TTY_FLIPBUF_SIZE;
 	}
 	count = tty->flip.count;
 	tty->flip.count = 0;
-	local_irq_restore(flags); // FIXME: is this safe?
+	spin_unlock_irqrestore(&tty->read_lock, flags);  /* need more locking elsewhere */
+
 	
 	tty->ldisc.receive_buf(tty, cp, fp, count);
 }
@@ -2092,12 +2123,16 @@
 	int error;
         int i;
 
+	lock_kernel();
+
 	if (driver->flags & TTY_DRIVER_INSTALLED)
 		return 0;
 
 	error = register_chrdev(driver->major, driver->name, &tty_fops);
-	if (error < 0)
+	if (error < 0) { 
+		unlock_kernel();
 		return error;
+	}
 	else if(driver->major == 0)
 		driver->major = error;
 
@@ -2111,6 +2146,7 @@
 		    tty_register_device(driver, driver->minor_start + i);
 	}
 	proc_tty_register_driver(driver);
+	unlock_kernel();
 	return error;
 }
 
@@ -2125,9 +2161,12 @@
 	struct termios *tp;
 	const char *othername = NULL;
 	
-	if (*driver->refcount)
+	if (*driver->refcount)  /* needs to be atomic */ 
 		return -EBUSY;
 
+
+	lock_kernel();
+
 	list_for_each_entry(p, &tty_drivers, tty_drivers) {
 		if (p == driver)
 			found++;
@@ -2135,13 +2174,17 @@
 			othername = p->name;
 	}
 	
-	if (!found)
+	if (!found) {
+		unlock_kernel();
 		return -ENOENT;
+	}
 
 	if (othername == NULL) {
 		retval = unregister_chrdev(driver->major, driver->name);
-		if (retval)
+		if (retval) {
+			unlock_kernel();
 			return retval;
+		}
 	} else
 		register_chrdev(driver->major, othername, &tty_fops);
 
@@ -2166,6 +2209,7 @@
 		tty_unregister_device(driver, driver->minor_start + i);
 	}
 	proc_tty_unregister_driver(driver);
+	unlock_kernel();
 	return 0;
 }
 
diff -u linux-2.5.56-work/fs/proc/proc_tty.c-o linux-2.5.56-work/fs/proc/proc_tty.c
--- linux-2.5.56-work/fs/proc/proc_tty.c-o	2002-10-19 04:32:32.000000000 +0200
+++ linux-2.5.56-work/fs/proc/proc_tty.c	2003-01-12 15:54:52.000000000 +0100
@@ -12,6 +12,7 @@
 #include <linux/proc_fs.h>
 #include <linux/stat.h>
 #include <linux/tty.h>
+#include <linux/smp_lock.h>
 #include <asm/bitops.h>
 
 extern struct tty_ldisc ldiscs[];
@@ -39,6 +40,8 @@
 	char	range[20], deftype[20];
 	char	*type;
 
+	lock_kernel();
+
 	list_for_each_entry(p, &tty_drivers, tty_drivers) {
 		if (p->num > 1)
 			sprintf(range, "%d-%d", p->minor_start,
@@ -88,6 +91,9 @@
 			len = 0;
 		}
 	}
+
+	unlock_kernel();
+
 	if (!p)
 		*eof = 1;
 	if (off >= len+begin)

Re: Fixing the tty layer was Re: any chance of 2.6.0-test*?

[Dipankar Sarma]

On Mon, Jan 13, 2003 at 08:25:39AM +0100, Andi Kleen wrote:
> > Oh, yes, I have spent hours and hours trying to untangle tty locking
> > and it isn't simple.
> 
> Oops. Could you quickly summarize your findings so far ?

I only found more confusions - I can't figure how tty_files list
is locked - sure files_lock is supposed to protect it but there
are deletions done without any lock. Another thing that needs
looking into is to avoid or reduce use of the tasklist_lock there.

> > What does that BKL protect ? I can't seem to ever figure our if
> > all the races are plugged or not.
> 
> Well, one has to start somewhere. Just starting by plugging most of the
> obvious races, then the more subtle ones can be attacked later.
> 
> The idea of the BKL was to protect the protect context code against
> itself (code lock) and also the few global data structures that 
> are only accessed from process context (like the tty drivers list)

In that case would it not be better to replace all BKLs by a single tty
lock ?

> 
> I attached my current patch, it isn't too well tested however and needs
> more work.
> 
> Mostly just adds lock_kernel()s to the high level code so far and a few comments.

Cool, I will start off by testing this stuff.

Thanks
Dipankar

Re: Fixing the tty layer was Re: any chance of 2.6.0-test*?

[Russell King]

On Mon, Jan 13, 2003 at 01:42:33PM +0530, Dipankar Sarma wrote:
> > The idea of the BKL was to protect the protect context code against
> > itself (code lock) and also the few global data structures that 
> > are only accessed from process context (like the tty drivers list)
> 
> In that case would it not be better to replace all BKLs by a single tty
> lock ?

No.  The tty layer relies on being able to safely reschedule with the
BKL held.  If you replace it with a "tty lock" you need to find all
those schedule() points throughout _every_ tty line discipline and
tty driver and release that lock.

Basically, the tty later was written upon the assumption that there
would be only _one_ thread of execution running tty code at any one
time, and we only reschedule when we explicitly want to (which was
the general kernel coding rule before we got spinlocks etc.)  Every
point where a reschedule is possible, state checks are (should be)
made to prevent races.

When analysing the tty layer, you have to think not "what data does
this protect" but "what code are we protecting".  Note that you must
apply the same approach towards what were the global-cli points.

I don't think its the BKL points you have to worry about; they've
stayed the same over many kernel versions.  The places that need
deeper consideration are where the global-cli was replaced with the
local-cli.  Obviously the latter is not a direct subsitute for the
former.

-- 
Russell King (rmk@arm.linux.org.uk)                The developer of ARM Linux
             http://www.arm.linux.org.uk/personal/aboutme.html

Re: Fixing the tty layer was Re: any chance of 2.6.0-test*?

[Dipankar Sarma]

On Mon, Jan 13, 2003 at 09:15:26AM +0000, Russell King wrote:
> > In that case would it not be better to replace all BKLs by a single tty
> > lock ?
> 
> No.  The tty layer relies on being able to safely reschedule with the
> BKL held.  If you replace it with a "tty lock" you need to find all
> those schedule() points throughout _every_ tty line discipline and
> tty driver and release that lock.

Yes, I get it now from this and Andi's mail. I hadn't thought about
that "special feature" of BKL :)

> 
> Basically, the tty later was written upon the assumption that there
> would be only _one_ thread of execution running tty code at any one
> time, and we only reschedule when we explicitly want to (which was
> the general kernel coding rule before we got spinlocks etc.)  Every
> point where a reschedule is possible, state checks are (should be)
> made to prevent races.

Hmm.. This understanding would make it easier for me to go take another look
at the tty layer.

> 
> When analysing the tty layer, you have to think not "what data does
> this protect" but "what code are we protecting".  Note that you must
> apply the same approach towards what were the global-cli points.
> 
> I don't think its the BKL points you have to worry about; they've
> stayed the same over many kernel versions.  The places that need
> deeper consideration are where the global-cli was replaced with the
> local-cli.  Obviously the latter is not a direct subsitute for the
> former.

BKL confused me here because I wasn't sure whether BKL was implicitly
protecting the tty driver code against anything else apart from itself.

Thanks
Dipankar

Re: any chance of 2.6.0-test*?

[Alan Cox]

On Mon, 2003-01-13 at 03:33, Paul Mackerras wrote:
> I'm using the patch below, which makes sure that ide_release (in
> ide-cs.c) runs in process context not interrupt context.  The specific
> problem I saw was that ide_unregister calls various devfs routines
> that don't like being called in interrupt context, but there may well
> be other thing ide_unregister does that aren't safe in interrupt
> context.

The ide release code isnt safe in any context.

> I have "fixed" the problem for now by adding a call to
> init_hwif_data(index) in ide_register_hw just before the first
> memcpy.  I'd be interested to know what the right fix is. :)

The code is currently written on the basis that people don't mangle
free interfaces (the free up code restores stuff which I grant is
kind of ass backwards). It also needs serious review and 2.5 testing
before I'd want to move it to the right spot.


Also note that freeing the IDE can fail. If it fails then the code
should probably be a lot smarter. Right now it 'loses' the interface.
Really it should set a timer and try again. It might also want to
add a null iops (out does nothing in returns FFFFFFFF) to stop
further I/O cycles.

Re: any chance of 2.6.0-test*?

[Benjamin Herrenschmidt]

On Mon, 2003-01-13 at 15:59, Alan Cox wrote:
> On Mon, 2003-01-13 at 03:33, Paul Mackerras wrote:
> > I'm using the patch below, which makes sure that ide_release (in
> > ide-cs.c) runs in process context not interrupt context.  The specific
> > problem I saw was that ide_unregister calls various devfs routines
> > that don't like being called in interrupt context, but there may well
> > be other thing ide_unregister does that aren't safe in interrupt
> > context.
> 
> The ide release code isnt safe in any context.

Yup, though Paul's patch is a first step in the right way as I don't
think anybody sane plan to fix the IDE release code to make it interrupt
safe ;) At least I don't...
 
> > I have "fixed" the problem for now by adding a call to
> > init_hwif_data(index) in ide_register_hw just before the first
> > memcpy.  I'd be interested to know what the right fix is. :)
> 
> The code is currently written on the basis that people don't mangle
> free interfaces (the free up code restores stuff which I grant is
> kind of ass backwards). It also needs serious review and 2.5 testing
> before I'd want to move it to the right spot.

The code is indeed strangely backward, I had to deal with that in 2.4
for the PowerBook hotswap CD bay and will soon have to review that for
2.4.21-pre & 2.5.

> Also note that freeing the IDE can fail. If it fails then the code
> should probably be a lot smarter. Right now it 'loses' the interface.
> Really it should set a timer and try again. It might also want to
> add a null iops (out does nothing in returns FFFFFFFF) to stop
> further I/O cycles.

Yup, this have been a problem for me too, as ide_unregister for example
fails with a mounted fs. So the user had effectively removed the drive
from the bay, but I couldn't free the interface... nasty. Especially if
the user then plugs some different IDE device in the bay, the kernel
will get completely confused.

ide-cs problem (was Re: any chance of 2.6.0-test*?)

[Paul Mackerras]

Alan Cox writes:

> The ide release code isnt safe in any context.

Do you mean ide_release in ide-cs.c, or ide_unregister in ide.c?

> The code is currently written on the basis that people don't mangle
> free interfaces (the free up code restores stuff which I grant is

Well, the problem is that there is no way to say that there is a hwif
present without any drives.  The low-level driver (i.e. pmac.c) has to
leave hwif->present = 0, because if you set hwif->present = 1 then the
ide probe code won't go looking for drives.  The probe code then only
sets hwif->present = 1 if it finds a drive.  If hwif->present gets
left at 0 then it is liable to be taken over for a pcmcia card.

What would be good is a way to say "I know this hwif is definitely
present, please probe for devices attached to it, but don't decide
it's not present if you don't find any devices".  I would like pmac.c
to be able to set hwif->present = 1 but still have the probe code go
looking for devices.

Regards,
Paul.

Re: any chance of 2.6.0-test*?

[Pavel Machek]

Hi!

> >   - Flaws in error recovery paths in certain situations
> >   - Lots of random oopses on boot/remove that were apparently
> >     introduced by the kobject/sysfs people and need chasing
> >     down. (There are some non sysfs ones mostly fixed)
> 
> I guess the kobject/sysfs stuff could be ripped out if it doesn't
> work - it is probably not a "must have" feature.

sysfs is  needed to properly flush caches on powerdown and for
S3/S4 suspend/resume to work.
								Pavel
-- 
Worst form of spam? Adding advertisment signatures ala sourceforge.net.
What goes next? Inserting advertisment *into* email?