Messages in this thread | | | Date | Sat, 4 Aug 2007 15:07:21 +0400 | From | Oleg Nesterov <> | Subject | Re: [PATCH] Use ERESTART_RESTARTBLOCK if poll() is interrupted by a signal (was: Re: [PATCH] Use ERESTARTNOHAND if poll() is interrupted by a signal) |
| |
On 08/03, Chris Wright wrote: > > +long do_restart_poll(struct restart_block *restart_block) > +{ > + struct pollfd __user *ufds = (struct pollfd __user*)restart_block->arg0; > + int nfds = restart_block->arg1; > + s64 timeout = ((s64)restart_block->arg3<<32) | (s64)restart_block->arg2; > + int ret; > + > + restart_block->fn = do_no_restart_syscall;
(just in case, this is not strictly necessary)
> + ret = do_sys_poll(ufds, nfds, &timeout); > + if (ret == -EINTR) { > + restart_block->fn = do_restart_poll; > + restart_block->arg2 = timeout & 0xFFFFFFFF; > + restart_block->arg3 = timeout >> 32; > + ret = -ERESTART_RESTARTBLOCK; > + } > + return ret; > +} > + > asmlinkage long sys_poll(struct pollfd __user *ufds, unsigned int nfds, > long timeout_msecs) > { > s64 timeout_jiffies; > + int ret; > > if (timeout_msecs > 0) { > #if HZ > 1000 > @@ -754,7 +773,20 @@ asmlinkage long sys_poll(struct pollfd __user *ufds, unsigned int nfds, > timeout_jiffies = timeout_msecs; > } > > - return do_sys_poll(ufds, nfds, &timeout_jiffies); > + ret = do_sys_poll(ufds, nfds, &timeout_jiffies); > + if (ret == -EINTR) { > + if (timeout_msecs > 0) {
This should be "if (timeout_msecs)", a negative timeout means "wait indefinitely", so we should restart as well.
> + struct restart_block *restart_block; > + restart_block = ¤t_thread_info()->restart_block; > + restart_block->fn = do_restart_poll; > + restart_block->arg0 = (unsigned long)ufds; > + restart_block->arg1 = nfds; > + restart_block->arg2 = timeout_jiffies & 0xFFFFFFFF; > + restart_block->arg3 = timeout_jiffies >> 32;
and, in that case, we should use "(u64)timeout_jiffies >> 32".
Small nit: sys_poll() and do_restart_poll() are not "symmetrical", the latter doesn't check *timeout at all. Either way is correct, restart with zero timeout means "try again once but doesn't wait".
There is a subtle (but harmless) difference though. Suppose that *timeout == 0 (either because timeout_msecs == 0 or because timeout expiried) and we have a "false" signal_pending(). In that case sys_poll() returns EINTR, but do_restart_poll() returns 0.
I was a bit surprized that sys_poll() return EINTR even if timeout expired, but preserved this behaviour in do_poll-return-eintr-when-signalled.patch.
Perhaps it is better to just remove "if (timeout_msecs > 0)" check from sys_poll(). Then we can modify do_poll() to return EINTR only when !*timeout, to eliminate unneeded (but correct) restart.
What do you think?
Oleg.
- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |