Messages in this thread | | | Date | Wed, 23 Apr 2014 16:44:00 -0400 | From | Rik van Riel <> | Subject | Re: [PATCH RFC] sysrq: rcu-ify __handle_sysrq |
| |
On 04/23/2014 04:04 PM, Andrew Morton wrote: > On Wed, 23 Apr 2014 12:53:52 -0400 Rik van Riel <riel@redhat.com> wrote: > >> Echoing values into /proc/sysrq-trigger seems to be a popular way to >> get information out of the kernel. However, dumping information about >> thousands of processes, or hundreds of CPUs to serial console can >> result in IRQs being blocked for minutes, resulting in various kinds >> of cascade failures. >> >> The most common failure is due to interrupts being blocked for a very >> long time. This can lead to things like failed IO requests, and other >> things the system cannot easily recover from. > > I bet nobody wants that console output anyway. You do the sysrq then > run dmesg or look in /var/log/messages to see what happened. People > who are experiencing problems such as this should run `dmesg -n 1' > before writing to sysrq-trigger.
I'm not sure about that. I know of a few hundred QA people who gather the bulk of their logs through serial console, and they do appear interested in sysrq output :)
>> It also leaves sysrq-from-irq-context when the sysrq keys are pressed, >> but that is probably desired since people want that to work in situations >> where the system is already hosed. >> >> The callers of register_sysrq_key and unregister_sysrq_key appear to be >> capable of sleeping. > > unregister_sysrq_key() is basically never used - a couple of scruffy > drivers during rmmod. We hardly need any locking in there at all. I > guess using simple RCU is better than just removing it though.
Yeah, I went with the "solve the easy 90%" aspect with this patch. I am not convinced that we want to complicate the sysrq code to better support a fringe use case, but if we can fix the big without increasing the code maintenance burden in the future, why not?
>> --- a/drivers/tty/sysrq.c >> +++ b/drivers/tty/sysrq.c >> @@ -510,9 +510,8 @@ void __handle_sysrq(int key, bool check_mask) >> struct sysrq_key_op *op_p; >> int orig_log_level; >> int i; >> - unsigned long flags; >> >> - spin_lock_irqsave(&sysrq_key_table_lock, flags); >> + rcu_read_lock(); >> /* >> * Raise the apparent loglevel to maximum so that the sysrq header >> * is shown to provide the user with positive feedback. We do not >> @@ -554,7 +553,7 @@ void __handle_sysrq(int key, bool check_mask) >> printk("\n"); >> console_loglevel = orig_log_level; >> } >> - spin_unlock_irqrestore(&sysrq_key_table_lock, flags); >> + rcu_read_unlock(); >> } >> >> void handle_sysrq(int key) >> @@ -1043,16 +1042,23 @@ static int __sysrq_swap_key_ops(int key, struct sysrq_key_op *insert_op_p, >> struct sysrq_key_op *remove_op_p) >> { >> int retval; >> - unsigned long flags; >> >> - spin_lock_irqsave(&sysrq_key_table_lock, flags); >> + spin_lock(&sysrq_key_table_lock); >> if (__sysrq_get_key_op(key) == remove_op_p) { >> __sysrq_put_key_op(key, insert_op_p); >> retval = 0; >> } else { >> retval = -1; >> } >> - spin_unlock_irqrestore(&sysrq_key_table_lock, flags); >> + spin_unlock(&sysrq_key_table_lock); >> + >> + /* >> + * A concurrent __handle_sysrq eitehr got the old op or the new op. > > yuo cnat spel
I can fix that for version 2, assuming people are interested it a v2 :)
>> + * Wait for it to go away before returning, so the code for an old >> + * op is not freed (eg. on module unload) while it is in use. >> + */ >> + synchronize_rcu(); >> + >> return retval; >> } >
-- All rights reversed
| |