Re: + uml-sigwinch-handling-cleanup.patch added to -mm tree

[Blaisorblade <blaisorblade@yahoo.it>]

On Thursday 05 January 2006 00:25, akpm@osdl.org 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 <jdike@addtoit.com>
>
> 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 jdike@addtoit.com 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