From owner-freebsd-threads@FreeBSD.ORG Tue Aug 17 01:29:53 2010 Return-Path: Delivered-To: freebsd-threads@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 8C4F81065697 for ; Tue, 17 Aug 2010 01:29:53 +0000 (UTC) (envelope-from davidxu@freebsd.org) Received: from freefall.freebsd.org (freefall.freebsd.org [IPv6:2001:4f8:fff6::28]) by mx1.freebsd.org (Postfix) with ESMTP id 5B4298FC19; Tue, 17 Aug 2010 01:29:53 +0000 (UTC) Received: from [127.0.0.1] (localhost [127.0.0.1]) by freefall.freebsd.org (8.14.4/8.14.4) with ESMTP id o7H1TpK6006345; Tue, 17 Aug 2010 01:29:52 GMT (envelope-from davidxu@freebsd.org) Message-ID: <4C6A5690.6060000@freebsd.org> Date: Tue, 17 Aug 2010 09:29:52 +0000 From: David Xu User-Agent: Thunderbird 2.0.0.24 (X11/20100630) MIME-Version: 1.0 To: Kostik Belousov References: <4C642E9B.8000300@freebsd.org> <20100812093353.GS2396@deviant.kiev.zoral.com.ua> <4C650D0F.9060905@freebsd.org> <4C650F27.1000305@freebsd.org> <20100813141402.GW2396@deviant.kiev.zoral.com.ua> <4C65E0FE.2030803@freebsd.org> <20100814144715.GB2396@deviant.kiev.zoral.com.ua> <4C6926D0.2020909@freebsd.org> <20100816082022.GO2396@deviant.kiev.zoral.com.ua> <4C696A96.7020709@freebsd.org> <20100816104303.GP2396@deviant.kiev.zoral.com.ua> In-Reply-To: <20100816104303.GP2396@deviant.kiev.zoral.com.ua> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Cc: freebsd-threads@freebsd.org Subject: Re: PTHREAD_CANCEL_DEFERRED X-BeenThere: freebsd-threads@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Threading on FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 17 Aug 2010 01:29:53 -0000 Kostik Belousov wrote: > On Mon, Aug 16, 2010 at 04:43:02PM +0000, David Xu wrote: >> Kostik Belousov wrote: >>> On Mon, Aug 16, 2010 at 11:53:52AM +0000, David Xu wrote: >>>> Kostik Belousov wrote: >>>> >>>>> Missed this, thank you for pointing it out. Updated patch is at >>>>> http://people.freebsd.org/~kib//misc/cancel_defer.2.patch >>>> I found SIGCANCEL is masked by >>>> thr_cancel_deferred(THR_CANCEL_DEFERRED_ENABLE), issignal() does not >>>> return the masked signal, so how a cancellation point syscall can be >>>> interrupted by SIGCANCEL ? I think if a thread being canceled calls >>>> msleep(PCATCH), it should find the signal and return EINTR. >>>> >>> Yes, for EINTR and ERESTART case, the thread should be canceled. >>> Please look at the check_cancel() helper that is called at the syscall >>> entry and before return. If the check_cancel() decided that the syscall >>> is cancellation point and the thread in the deferred cancel mode, and >>> EINTR or ERESTART is supplied as error code, then SIGCANCEL is removed >> >from the thread signal mask. It is restored in the mask by ast(). >> >> I saw your patch has following lines, on syscall entry, if the >> check_cancel finds that the syscall is cancellation point and >> SIGCANCEL exists, it returns non-zero value, then the real syscall >> body at line 319 of subr_trap.c is not executed and prematurely returns. >> This is not what I want,as you said the syscall's implementation should >> always be executed, and if the thread will be blocked, then it should be >> interrupted by existing SIGCANCEL via issignal() which is called by >> sleepqueue routines. I think the current patch also has potential > No, I do not think what you describe is completely right behaviour. > As I understand SUSv4, the deferred cancel behaviour should be > as following: > - when we reach cancellation point with cancel already pending, > the syscall should not be executed at all, cancel happens and > cleanup handlers executed before thread exiting. > - when we reach cancellation point without cancel pending, and syscall > goes to sleep, and cancellation request arrives during the sleep, > again the syscall should be aborted, and cancellation happens. > so what's the problem with current libthr code ? it already did above in this way. using close() as an example, what does your modification gains ? is the kern_close() always executed for close() syscall? or if the cancellation is already pending and the syscall prematurely returns, will the file descriptor be leaked ? if it is leaked, then your modification is not better than current libthr code, it is even worse than current one, because your code involves kernel. > So I check for the cancellation at two points: one is the syscall > entry, to catch already pending cancellation request. Second is on > the syscall exit path, where cancellation is only checked in the > case the sleep was aborted. >> performance problem, the check_cancel uses PROC_LOCK, which might be a >> performance hit. > Might be. Lets first make sure that the semantic is right. > >> @@ -300,6 +332,9 @@ syscallenter(struct thread *td, struct syscall_args *sa) >> if (error != 0) >> goto retval; >> } >> + error = check_cancel(td, sa->callp, EINTR); >> + if (error != 0) >> + goto retval; >> error = syscall_thread_enter(td, sa->callp); >> if (error != 0) >> goto retval; >>