any chance of 2.6.0-test*?
Subsystems (2)
A message that is not in the archive (sent off-list or not yet received): [email protected]
A message that is not in the archive (sent off-list or not yet received): [email protected]
Quoted 7 lines · Alan Cox
Quoted 3 lines · William Lee Irwin III
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?)
Quoted 3 lines
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?
Quoted 1 line
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)
Quoted 2 lines
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
drivers/char/tty_io.c renamed from drivers/char/tty_io.c-o +17 -2
@@ -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 ; }
drivers/char/tty_io.c line 1333
Quoted 8 lines
drivers/char/tty_io.c renamed from drivers/char/tty_io.c-o +2
-0@@ -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().
Quoted 13 lines
drivers/char/tty_io.c renamed from drivers/char/tty_io.c-o +2
-0/* 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 */
drivers/char/tty_io.c line 1365
Quoted 5 lines
drivers/char/tty_io.c renamed from drivers/char/tty_io.c-o +2
-0@@ -1357,6 +1361,8 @@#endif /* CONFIG_UNIX_98_PTYS */ } +lock_kernel (); +
Deadlock. See chrdev_open() note above.
Quoted 13 lines
drivers/char/tty_io.c renamed from drivers/char/tty_io.c-o +2
-0retval =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))
drivers/char/tty_io.c line 1408
Quoted 4 lines
drivers/char/tty_io.c renamed from drivers/char/tty_io.c-o +1
-0@@ -1397,6 +1405,7 @@/* * Need to reset f_op in case a hangup happened. */ +lock_kernel ();
Deadlock. See chrdev_open() note above.
Quoted 14 lines
drivers/char/tty_io.c renamed from drivers/char/tty_io.c-o +1
-0filp->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 ;
drivers/char/tty_io.c line 1463
Quoted 10 lines
drivers/char/tty_io.c renamed from drivers/char/tty_io.c-o +7
-2-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.
Signature · 2 lines
Russell King ([email protected]) The developer of ARM Linux http://www.arm.linux.org.uk/personal/aboutme.html
Quoted 11 lines · Russell King
drivers/char/tty_io.c line 1333
Quoted 8 lines
drivers/char/tty_io.c renamed from drivers/char/tty_io.c-o +2
-0@@ -1329,6 +1329,8 @@int major, minor;struct tty_driver *driver; +lock_kernel (); +Deadlock.
chrdev_open()callslock_kernel()and then thefops->openmethod, which istty_open().
No problem, lock_kernel is recursive and dropped on schedule.
It is very very hard to get a BKL deadlock.
Quoted 1 line
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
Quoted 6 lines · Andi Kleen
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.
Quoted 2 lines
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
Quoted 8 lines
Quoted 3 lines
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?
Quoted 1 line
From what I can see the high level tty code mostly takes
lock_kernelbefore doing anything.
Which works really well with all the IRQ paths on it
Quoted 3 lines
On reads access to
file->private_datais 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.
Quoted 2 lines
The main problems are
- Incorrect locking all over the place
Hmm bad. Is it that hard to fix ?
Quoted 1 line
- 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.
Quoted 1 line
- 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)
Quoted 4 lines
- 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.
Quoted 4 lines
- 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.
Quoted 1 line
- IDE raid hasn't been ported to 2.5 at all yet
Vendor problem ?
Quoted 11 lines
Quoted 9 lines
Quoted 4 lines
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?
Quoted 1 line
From what I can see the high level tty code mostly takes
lock_kernelbefore 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)
Quoted 6 lines
Quoted 3 lines
On reads access to
file->private_datais 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
Quoted 4 lines · Andi Kleen
Quoted 2 lines
The main problems are
- Incorrect locking all over the placeHmm bad. Is it that hard to fix ?
Very. Just read the settings/proc code for an example
Quoted 7 lines
Quoted 1 line
- 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
Quoted 6 lines
Quoted 3 lines
- 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.
Quoted 6 lines
Quoted 4 lines
- 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 unregCan'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.
Quoted 3 lines
Quoted 1 line
- 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.
Quoted 3 lines
Quoted 1 line
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.
Quoted 9 lines · Alan Cox
Quoted 6 lines
Quoted 4 lines
- 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 unregCan'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
Quoted 6 lines · Andi Kleen
Quoted 5 lines · Alan Cox
Quoted 5 lines
Quoted 4 lines
- 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
Quoted 6 lines
Quoted 4 lines
Quoted 1 line
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.
Quoted 3 lines · Alan Cox
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.
drivers/ide/legacy/ide-cs.c +18 -12
@@ -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 ;
Quoted 6 lines · Paul Mackerras
I'm using the patch below, which makes sure that
ide_release(inide-cs.c) runs in process context not interrupt context. The specific problem I saw was thatide_unregistercalls various devfs routines that don't like being called in interrupt context, but there may well be other thingide_unregisterdoes that aren't safe in interrupt context.
The ide release code isnt safe in any context.
Quoted 3 lines
I have "fixed" the problem for now by adding a call to
init_hwif_data(index)inide_register_hwjust 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.
Quoted 9 lines · Alan Cox
Quoted 6 lines · Paul Mackerras
I'm using the patch below, which makes sure that
ide_release(inide-cs.c) runs in process context not interrupt context. The specific problem I saw was thatide_unregistercalls various devfs routines that don't like being called in interrupt context, but there may well be other thingide_unregisterdoes 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...
Quoted 8 lines
Quoted 3 lines
I have "fixed" the problem for now by adding a call to
init_hwif_data(index)inide_register_hwjust 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.
Quoted 5 lines
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*?)
In reply to Alan CoxQuoted 1 line · Alan Cox
The ide release code isnt safe in any context.
Do you mean ide_release in ide-cs.c, or ide_unregister in ide.c?
Quoted 2 lines
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.
Hi!
Quoted 7 lines
Quoted 4 lines
- 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
Signature · 2 lines
Worst form of spam? Adding advertisment signatures ala sourceforge.net. What goes next? Inserting advertisment *into* email?
A message that is not in the archive (sent off-list or not yet received): [email protected]
Fixing the tty layer was Re: any chance of 2.6.0-test*?
[forgot to cc linux-kernel in the first try, sorry if you see it twice]
Quoted 7 lines · Greg KH
Quoted 4 lines · Alan Cox
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: Fixing the tty layer was Re: any chance of 2.6.0-test*?
Quoted 1 line · Andi Kleen
Quoted 1 line
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.
Quoted 9 lines
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 fortty_io.cat least
What does that BKL protect ? I can't seem to ever figure our if all the races are plugged or not.
Quoted 3 lines
The
local_irq_savein there are buggy, they need to take a lock.
Also a locking model w.r.t. the serial drivers ?
Quoted 9 lines
- Audit the data structures that are touched by interrupts and add spinlocks.
At least forn_tty.cprobably justtty->read_lockneeds 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)
Quoted 7 lines
- 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*?
Quoted 5 lines · Dipankar Sarma
Quoted 1 line · Andi Kleen
Quoted 1 line
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 ?
Quoted 6 lines
Quoted 6 lines
it has to be fixed for 2.6, no argument.
I took a look at it. I think the easiest strategy would be:
Quoted 7 lines
Quoted 4 lines
- 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 fortty_io.cat leastWhat 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)
Quoted 6 lines
Quoted 4 lines
The
local_irq_savein 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.
Quoted 14 lines
Quoted 14 lines
- Audit the data structures that are touched by interrupts and add spinlocks.
At least forn_tty.cprobably justtty->read_lockneeds 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)
Quoted 7 lines
Quoted 3 lines
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
drivers/char/tty_io.c renamed from drivers/char/tty_io.c-o +56 -13
@@ -114,7 +114,12 @@#define CHECK_TTY_COUNT 1struct 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 ; }
fs/proc/proc_tty.c renamed from fs/proc/proc_tty.c-o +6 -0
@@ -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*?
Quoted 4 lines · Andi Kleen
Quoted 2 lines
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.
Quoted 9 lines
Quoted 2 lines
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 ?
Quoted 5 lines
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*?
Quoted 6 lines · Dipankar Sarma
Quoted 3 lines
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.
Signature · 2 lines
Russell King ([email protected]) 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*?
Quoted 7 lines · Russell King
Quoted 2 lines
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 :)
Quoted 7 lines
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.
Quoted 10 lines
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: Fixing the tty layer was Re: any chance of 2.6.0-test*?
In reply to Andi KleenQuoted 4 lines · Andi Kleen
- 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.
Quoted 3 lines
Probably all doable with some concentrated effort.
Anyone interested in helping ?
I am.
thanks,
greg k-h