Re: [uml-devel] Occasional hang starting up.

[Blaisorblade <blaisorblade@yahoo.it>]

[-- Attachment #1: Type: text/plain, Size: 3630 bytes --]

On Thursday 23 March 2006 19:45, Jeff Dike wrote:
> On Tue, Mar 21, 2006 at 02:21:40AM +0100, Blaisorblade wrote:
> > However, while I overlooked this, you overlooked what I did:
> > *) you must also move the setting of write_sigio_fds and sigio_private
> > above, or we'll get the same problem again in different places of
> > write_sigio_thread.

> OK, this is all done, I think.

> What do you think about this?

Yep, it seems it looks good. But 1 bug still and two suggestions.
See also attached patch (it misses one of the suggestions because it's late 
and I'm going to sleep).

> Index: linux-2.6.15/arch/um/os-Linux/sigio.c
> ===================================================================
> --- linux-2.6.15.orig/arch/um/os-Linux/sigio.c	2006-03-23
> 12:55:27.000000000 -0500 +++
> linux-2.6.15/arch/um/os-Linux/sigio.c	2006-03-23 13:41:11.000000000 -0500
> @@ -269,42 +269,41 @@ void write_sigio_workaround(void)
>  	/* Did we race? Don't try to optimize this, please, it's not so likely
>  	 * to happen, and no more than once at the boot. */
>  	if(write_sigio_pid != -1)
> -		goto out_unlock;
> +		goto out_free;
>
> -	write_sigio_pid = run_helper_thread(write_sigio_thread, NULL,
> -					    CLONE_FILES | CLONE_VM, &stack, 0);
> -
> -	if (write_sigio_pid < 0)
> -		goto out_clear;
> +	current_poll = ((struct pollfds) { .poll 	= p,
> +					   .used 	= 1,
> +					   .size 	= 1 });
>
>  	if (write_sigio_irq(l_write_sigio_fds[0]))
> -		goto out_kill;
> +		goto out_free;

<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<
<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<
Here you're not clearing current_poll!
<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<
<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<

> -	/* Success, finally. */
>  	memcpy(write_sigio_fds, l_write_sigio_fds, sizeof(l_write_sigio_fds));
>  	memcpy(sigio_private, l_sigio_private, sizeof(l_sigio_private));
>
> -	current_poll = ((struct pollfds) { .poll 	= p,
> -					   .used 	= 1,
> -					   .size 	= 1 });
> +	write_sigio_pid = run_helper_thread(write_sigio_thread, NULL,
> +					    CLONE_FILES | CLONE_VM, &stack, 0);
>
> -	sigio_unlock();
> -	return;
> +	if (write_sigio_pid < 0)
> +		goto out_clear;
>
> - out_kill:
> -	l_write_sigio_pid = write_sigio_pid;
> -	write_sigio_pid = -1;
>  	sigio_unlock();
> -	/* Going to call waitpid, avoid holding the lock. */
> -	os_kill_process(l_write_sigio_pid, 1);
> -	goto out_free;
> +	return;
>
>   out_clear:
>  	write_sigio_pid = -1;
> - out_unlock:
> -	sigio_unlock();
> +	current_poll = ((struct pollfds) { .poll	= NULL,
> +					   .size	= 0,
> +					   .used	= 0 });
> +	write_sigio_fds[0] = -1;
> +	write_sigio_fds[1] = -1;
> +
> +	sigio_private[0] = -1;
> +	sigio_private[1] = -1;
> +

If possible (there should be such array constructors) this shouldn't be 
redundant / duplicated with the initial declarations.

Like write_sigio_fds = (int[]) {-1,-1} and having a common macro like 

#define SIGIO_FDS_INIT {-1,-1}

static int write_sigio_fds[2] = SIGIO_FDS_INIT;
...
write_sigio_fds = (int[]) SIGIO_FDS_INIT

(we _will_ get one copy wrong, if we can :-)).

If sending to 2.6.16 this can be kept separate or they could maybe complain 
"ah, no extra changes, just the fix!".
Btw, this one is not attached.
>   out_free:
>  	kfree(p);
> +	sigio_unlock();

p is a local variable, so sigio_unlock can be done before kfree(p).

>   out_close2:
>  	close(l_sigio_private[0]);
>  	close(l_sigio_private[1]);

-- 
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

[-- Attachment #2: uml-fix-sigio-startup-race-hang-fix.patch --]
[-- Type: text/x-diff, Size: 1076 bytes --]

Index: linux-2.6.git/arch/um/kernel/sigio_user.c
===================================================================
--- linux-2.6.git.orig/arch/um/kernel/sigio_user.c
+++ linux-2.6.git/arch/um/kernel/sigio_user.c
@@ -398,7 +398,7 @@ void write_sigio_workaround(void)
 					   .size 	= 1 });
 
 	if (write_sigio_irq(l_write_sigio_fds[0]))
-		goto out_free;
+		goto out_clear_poll;
 
 	memcpy(write_sigio_fds, l_write_sigio_fds, sizeof(l_write_sigio_fds));
 	memcpy(sigio_private, l_sigio_private, sizeof(l_sigio_private));
@@ -414,18 +414,20 @@ void write_sigio_workaround(void)
 
  out_clear:
 	write_sigio_pid = -1;
-	current_poll = ((struct pollfds) { .poll	= NULL,
-					   .size	= 0,
-					   .used	= 0 });
+
 	write_sigio_fds[0] = -1;
 	write_sigio_fds[1] = -1;
 
 	sigio_private[0] = -1;
 	sigio_private[1] = -1;
 
+ out_clear_poll:
+	current_poll = ((struct pollfds) { .poll	= NULL,
+					   .size	= 0,
+					   .used	= 0 });
  out_free:
-	kfree(p);
 	sigio_unlock();
+	kfree(p);
  out_close2:
 	os_close_file(l_sigio_private[0]);
 	os_close_file(l_sigio_private[1]);