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
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]);
