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

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

[Blaisorblade]

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 

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

[Jeff Dike]

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

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

[Blaisorblade]

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