On Thursday 05 January 2006 00:25, [email protected] wrote:
> The patch titled
>
> uml: SIGWINCH handling cleanup
>
> has been added to the -mm tree. Its filename is
>
> uml-sigwinch-handling-cleanup.patch
Please hold, should be fixed.
> From: Jeff Dike <[email protected]>
>
> Code cleanup - unregister_winch and winch_cleanup had some duplicate code.
> This is now abstracted out into free_winch.
Meanwhile, the whole content of the new free_winch(), including some syscalls
on the host, and various other stuff, is brought back under the
winch_handler_lock.
And this without notice, which means that possibly the author didn't notice
this. A note "I brought things back under the lock because it was dumb" would
have made me happier...
I had carefully brought that stuff out keeping only the list access under the
lock, probably while fixing some "scheduling while atomic" warnings - once
the element is out of the list it's unreachable thus (IMHO) safely
accessible.
I'm thinking to some scenarios to verify if this is as true as it seems...
So, list_del should be brought out from free_winch, which would then become
callable without the spinlock held.
> +static void free_winch(struct winch *winch)
> +{
> + list_del(&winch->list);
> +
> + if(winch->pid != -1)
> + os_kill_process(winch->pid, 1);
> + if(winch->fd != -1)
> + os_close_file(winch->fd);
> +
> + free_irq(WINCH_IRQ, winch);
> + kfree(winch);
> +}
> +
> static void unregister_winch(struct tty_struct *tty)
> {
> struct list_head *ele;
> - struct winch *winch, *found = NULL;
> + struct winch *winch;
>
> spin_lock(&winch_handler_lock);
> +
> list_for_each(ele, &winch_handlers){
> winch = list_entry(ele, struct winch, list);
> if(winch->tty == tty){
> - found = winch;
> - break;
> + free_winch(winch);
> + break;
> }
> }
> - if(found == NULL)
> - goto err;
> -
> - list_del(&winch->list);
> - spin_unlock(&winch_handler_lock);
> -
> - if(winch->pid != -1)
> - os_kill_process(winch->pid, 1);
> -
> - free_irq(WINCH_IRQ, winch);
> - kfree(winch);
> -
> - return;
> -err:
> spin_unlock(&winch_handler_lock);
> }
>
> -/* XXX: No lock as it's an exitcall... is this valid? Depending on cleanup
> - * order... are we sure that nothing else is done on the list? */
> static void winch_cleanup(void)
> {
> - struct list_head *ele;
> + struct list_head *ele, *next;
> struct winch *winch;
>
> - list_for_each(ele, &winch_handlers){
> + spin_lock(&winch_handler_lock);
> +
> + list_for_each_safe(ele, next, &winch_handlers){
> winch = list_entry(ele, struct winch, list);
> - if(winch->fd != -1){
> - /* Why is this different from the above free_irq(),
> - * which deactivates SIGIO? This searches the FD
> - * somewhere else and removes it from the list... */
> - deactivate_fd(winch->fd, WINCH_IRQ);
> - os_close_file(winch->fd);
> - }
> - if(winch->pid != -1)
> - os_kill_process(winch->pid, 1);
> + free_winch(winch);
> }
> +
> + spin_unlock(&winch_handler_lock);
> }
> __uml_exitcall(winch_cleanup);
>
> _
>
> Patches currently in -mm which might be from [email protected] are
>
> uml-use-kstrdup.patch
> uml-non-void-functions-should-return-something.patch
> uml-formatting-changes.patch
> uml-use-array_size.patch
> uml-remove-unneeded-structure-field.patch
> uml-move-mconsole-support-out-of-generic-code.patch
> uml-add-static-initializations-and-declarations.patch
> uml-line_setup-interface-change.patch
> uml-move-console-configuration.patch
> uml-simplify-console-opening-closing-and-irq-registration.patch
> uml-fix-flip_buf-full-handling.patch
> uml-add-throttling-to-console-driver.patch
> uml-separate-libc-dependent-umid-code.patch
> uml-umid-cleanup.patch
> uml-sigwinch-handling-cleanup.patch
> uml-better-diagnostics-for-broken-configs.patch
> uml-add-mconsole_reply-variant-with-length-param.patch
> uml-capture-printk-output-for-mconsole-stack.patch
> uml-capture-printk-output-for-mconsole-sysrq.patch
> uml-fix-whitespace-in-mconsole-driver.patch
> uml-free-network-irq-correctly.patch
> add-block_device_operationsgetgeo-block-device-method.patch
> fix-possible-page_cache_shift-overflows.patch
> tty-layer-buffering-revamp-uml-fix.patch
--
Inform me of my mistakes, so I can keep imitating Homer Simpson's "Doh!".
Paolo Giarrusso, aka Blaisorblade (Skype ID "PaoloGiarrusso", ICQ 215621894)
http://www.user-mode-linux.org/~blaisorblade
Chiacchiera con i tuoi amici in tempo reale!
http://it.yahoo.com/mail_it/foot/*http://it.messenger.yahoo.com
On Thu, Jan 05, 2006 at 08:54:37PM +0100, Blaisorblade wrote:
> Meanwhile, the whole content of the new free_winch(), including some syscalls
> on the host, and various other stuff, is brought back under the
> winch_handler_lock.
And? There's no particular problem with host system calls being under
a lock. And the various other stuff is a kfree and a free_irq, which
I don't think have a problem being called under a spinlock.
> I had carefully brought that stuff out keeping only the list access under the
> lock, probably while fixing some "scheduling while atomic" warnings - once
> the element is out of the list it's unreachable thus (IMHO) safely
> accessible.
Probably? What in there is sensitive to being called under a lock?
> So, list_del should be brought out from free_winch, which would then become
> callable without the spinlock held.
That would increase the amount of code, with no gain that I can see.
The list_del would be duplicated, and the loop in winch_cleanup would
have to drop and reacquire the lock around each call to free_winch.
Jeff
On Thursday 05 January 2006 23:27, Jeff Dike wrote:
> On Thu, Jan 05, 2006 at 08:54:37PM +0100, Blaisorblade wrote:
> > Meanwhile, the whole content of the new free_winch(), including some
> > syscalls on the host, and various other stuff, is brought back under the
> > winch_handler_lock.
>
> And? There's no particular problem with host system calls being under
> a lock. And the various other stuff is a kfree and a free_irq, which
> I don't think have a problem being called under a spinlock.
Indeed, right. Ok, no real objections on the patch.
> > I had carefully brought that stuff out keeping only the list access under
> > the lock, probably while fixing some "scheduling while atomic" warnings -
> > once the element is out of the list it's unreachable thus (IMHO) safely
> > accessible.
>
> Probably? What in there is sensitive to being called under a lock?
Ok, sorry, wrong here. I remembered doing the thing but it was for other
reasons - reducing spinlock hold times. Doing syscalls under spinlocks is
just (possibly) slow, not wrong. But ok, the commendment was "thou shalt not
optimize".
> > So, list_del should be brought out from free_winch, which would then
> > become callable without the spinlock held.
> That would increase the amount of code, with no gain that I can see.
> The list_del would be duplicated, and the loop in winch_cleanup would
> have to drop and reacquire the lock around each call to free_winch.
I thought mainly to unregister_winch(); the lock in winch_cleanup() has been
added now, I didn't see it.
> Jeff
--
Inform me of my mistakes, so I can keep imitating Homer Simpson's "Doh!".
Paolo Giarrusso, aka Blaisorblade (Skype ID "PaoloGiarrusso", ICQ 215621894)
http://www.user-mode-linux.org/~blaisorblade
___________________________________
Yahoo! Mail: gratis 1GB per i messaggi e allegati da 10MB
http://mail.yahoo.it