Date: Fri, 20 Aug 2010 17:49:06 +0000 From: David Xu <davidxu@freebsd.org> To: Kostik Belousov <kostikbel@gmail.com> Cc: freebsd-threads@freebsd.org Subject: Re: PTHREAD_CANCEL_DEFERRED Message-ID: <4C6EC012.7000506@freebsd.org> In-Reply-To: <20100820085621.GK2396@deviant.kiev.zoral.com.ua> References: <4C696A96.7020709@freebsd.org> <20100816104303.GP2396@deviant.kiev.zoral.com.ua> <4C6AA092.40708@freebsd.org> <4C6BE0F7.10207@freebsd.org> <20100818100403.GS2396@deviant.kiev.zoral.com.ua> <4C6C6184.9030602@freebsd.org> <20100819083809.GC2396@deviant.kiev.zoral.com.ua> <4C6D6384.7080506@freebsd.org> <20100819144218.GH2396@deviant.kiev.zoral.com.ua> <4C6DD70F.5070803@freebsd.org> <20100820085621.GK2396@deviant.kiev.zoral.com.ua>
next in thread | previous in thread | raw e-mail | index | archive | help
Kostik Belousov wrote:
> On Fri, Aug 20, 2010 at 01:14:55AM +0000, David Xu wrote:
>> Kostik Belousov wrote:
>>
>>> What would happen in the following situation:
>>> thr_wake() is called;
>>> some syscall is started executing and slept, assume that SA_RESTART is
>>> for SIGHUP (just an example);
>>> SIGHUP is sent to the process and the thread is selected
>>> for delivery, also assume that handler is installed.
>>>
>>> As I understand, in this situation, EINTR is returned from syscall.
>> Yes, because cancellation should have higher priority over signals,
>> if signal always causes ERESTART to be returned, and system call
>> is always restarted, the system call may not return forever because
>> its event it is waiting never happens, for example, it is reading a byte
>> from a socket, but it is never available, we end up not being able to
>> cancel the thread, this is incorrect.
>
> Again, this is not exactly what I mean. Assume that the cancellation is
> not requested at all. I see no reason to return EINTR.
This flag is only turned on when the target thread has cancellation
enabled and it is at cancellation point and pthread_cancel() was
issued for it.
>
> As I see, you do not check errno when seeing syscall error return
> in the committed patch.
>
Yes, I don't check it now. This becauses I can not figure out a reliable
behavior, not because I want to offend you.
There are two requirements I remembered:
1) At a cancellation point, it must have a chance to cancel thread.
2) Its behavior must be reliable.
at present, I check cancellation at start of a cancellation point
function, it may kill the thread itself, this fits requirement 1.
then if the thread is still alive, it invokes system call, when
system call returns, it checks cancellation request again, and
may kill the thread itself. This fits requirement 2.
because of requirement 1, at the start of cancellation point,
the thread may be killed, error code is not returned even if
caller provided invalid argument to system call because
system call is not issued !
if later, system call returns error, if I use your idea, the
cancellation point function should return error code to caller,
then this is not a reliable behavior!
The cancellation point ends up that it either returns error
or not, even if the provided argument to kernel is always
invalid at all!
If you eliminates requirement 1, and use your idea to not cancel
thread on error, if user always provides an invalid argument to
kernel syscall, then the cancellation point never can cancel
the thread, this is incorrect.
So at present, I can only find the stable behavior as current code does,
if you can find a better behavior, feel free to improve it.
> Anyway, below is mostly cosmetic change to reduce code duplication,
> saved from the reverted patch of mine. Any objections ?
>
will commit it, thanks!
> diff --git a/lib/libthr/thread/thr_sig.c b/lib/libthr/thread/thr_sig.c
> index eed6fdb..115aa65 100644
> --- a/lib/libthr/thread/thr_sig.c
> +++ b/lib/libthr/thread/thr_sig.c
> @@ -268,23 +268,26 @@ _pthread_sigmask(int how, const sigset_t *set, sigset_t *oset)
>
> __weak_reference(__sigsuspend, sigsuspend);
>
> -int
> -_sigsuspend(const sigset_t * set)
> +static const sigset_t *
> +thr_remove_thr_signals(const sigset_t *set, sigset_t *newset)
> {
> - sigset_t newset;
> const sigset_t *pset;
> - int ret;
>
> if (SIGISMEMBER(*set, SIGCANCEL)) {
> - newset = *set;
> - SIGDELSET(newset, SIGCANCEL);
> - pset = &newset;
> + *newset = *set;
> + SIGDELSET(*newset, SIGCANCEL);
> + pset = newset;
> } else
> pset = set;
> + return (pset);
> +}
>
> - ret = __sys_sigsuspend(pset);
> +int
> +_sigsuspend(const sigset_t * set)
> +{
> + sigset_t newset;
>
> - return (ret);
> + return (__sys_sigsuspend(thr_remove_thr_signals(set, &newset)));
> }
>
> int
> @@ -292,18 +295,10 @@ __sigsuspend(const sigset_t * set)
> {
> struct pthread *curthread = _get_curthread();
> sigset_t newset;
> - const sigset_t *pset;
> int ret;
>
> - if (SIGISMEMBER(*set, SIGCANCEL)) {
> - newset = *set;
> - SIGDELSET(newset, SIGCANCEL);
> - pset = &newset;
> - } else
> - pset = set;
> -
> _thr_cancel_enter(curthread);
> - ret = __sys_sigsuspend(pset);
> + ret = __sys_sigsuspend(thr_remove_thr_signals(set, &newset));
> _thr_cancel_leave(curthread);
>
> return (ret);
> @@ -318,17 +313,9 @@ _sigtimedwait(const sigset_t *set, siginfo_t *info,
> const struct timespec * timeout)
> {
> sigset_t newset;
> - const sigset_t *pset;
> - int ret;
>
> - if (SIGISMEMBER(*set, SIGCANCEL)) {
> - newset = *set;
> - SIGDELSET(newset, SIGCANCEL);
> - pset = &newset;
> - } else
> - pset = set;
> - ret = __sys_sigtimedwait(pset, info, timeout);
> - return (ret);
> + return (__sys_sigtimedwait(thr_remove_thr_signals(set, &newset), info,
> + timeout));
> }
>
> /*
> @@ -342,17 +329,11 @@ __sigtimedwait(const sigset_t *set, siginfo_t *info,
> {
> struct pthread *curthread = _get_curthread();
> sigset_t newset;
> - const sigset_t *pset;
> int ret;
>
> - if (SIGISMEMBER(*set, SIGCANCEL)) {
> - newset = *set;
> - SIGDELSET(newset, SIGCANCEL);
> - pset = &newset;
> - } else
> - pset = set;
> _thr_cancel_enter_defer(curthread, 1);
> - ret = __sys_sigtimedwait(pset, info, timeout);
> + ret = __sys_sigtimedwait(thr_remove_thr_signals(set, &newset), info,
> + timeout);
> _thr_cancel_leave_defer(curthread, (ret == -1));
> return (ret);
> }
> @@ -361,18 +342,8 @@ int
> _sigwaitinfo(const sigset_t *set, siginfo_t *info)
> {
> sigset_t newset;
> - const sigset_t *pset;
> - int ret;
> -
> - if (SIGISMEMBER(*set, SIGCANCEL)) {
> - newset = *set;
> - SIGDELSET(newset, SIGCANCEL);
> - pset = &newset;
> - } else
> - pset = set;
>
> - ret = __sys_sigwaitinfo(pset, info);
> - return (ret);
> + return (__sys_sigwaitinfo(thr_remove_thr_signals(set, &newset), info));
> }
>
> /*
> @@ -385,18 +356,10 @@ __sigwaitinfo(const sigset_t *set, siginfo_t *info)
> {
> struct pthread *curthread = _get_curthread();
> sigset_t newset;
> - const sigset_t *pset;
> int ret;
>
> - if (SIGISMEMBER(*set, SIGCANCEL)) {
> - newset = *set;
> - SIGDELSET(newset, SIGCANCEL);
> - pset = &newset;
> - } else
> - pset = set;
> -
> _thr_cancel_enter_defer(curthread, 1);
> - ret = __sys_sigwaitinfo(pset, info);
> + ret = __sys_sigwaitinfo(thr_remove_thr_signals(set, &newset), info);
> _thr_cancel_leave_defer(curthread, ret == -1);
> return (ret);
> }
> @@ -405,18 +368,8 @@ int
> _sigwait(const sigset_t *set, int *sig)
> {
> sigset_t newset;
> - const sigset_t *pset;
> - int ret;
> -
> - if (SIGISMEMBER(*set, SIGCANCEL)) {
> - newset = *set;
> - SIGDELSET(newset, SIGCANCEL);
> - pset = &newset;
> - } else
> - pset = set;
>
> - ret = __sys_sigwait(pset, sig);
> - return (ret);
> + return (__sys_sigwait(thr_remove_thr_signals(set, &newset), sig));
> }
>
> /*
> @@ -429,18 +382,10 @@ __sigwait(const sigset_t *set, int *sig)
> {
> struct pthread *curthread = _get_curthread();
> sigset_t newset;
> - const sigset_t *pset;
> int ret;
>
> - if (SIGISMEMBER(*set, SIGCANCEL)) {
> - newset = *set;
> - SIGDELSET(newset, SIGCANCEL);
> - pset = &newset;
> - } else
> - pset = set;
> -
> _thr_cancel_enter_defer(curthread, 1);
> - ret = __sys_sigwait(pset, sig);
> + ret = __sys_sigwait(thr_remove_thr_signals(set, &newset), sig);
> _thr_cancel_leave_defer(curthread, (ret != 0));
> return (ret);
> }
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?4C6EC012.7000506>