From owner-svn-src-projects@FreeBSD.ORG Tue Jul 31 09:41:06 2012 Return-Path: Delivered-To: svn-src-projects@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 8D0891065672; Tue, 31 Jul 2012 09:41:06 +0000 (UTC) (envelope-from mavbsd@gmail.com) Received: from mail-bk0-f54.google.com (mail-bk0-f54.google.com [209.85.214.54]) by mx1.freebsd.org (Postfix) with ESMTP id 753F18FC0C; Tue, 31 Jul 2012 09:41:05 +0000 (UTC) Received: by bkcje9 with SMTP id je9so3179910bkc.13 for ; Tue, 31 Jul 2012 02:41:04 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=sender:message-id:date:from:user-agent:mime-version:to:cc:subject :references:in-reply-to:content-type:content-transfer-encoding; bh=62cRqE474JlqAomeIvx1waqjrp7e0tuKiTKXRnWDl60=; b=UmkaJD70QIsx8mIoKB0agSv1AD4hyxa38DFnxbVI62wjUjFtIJGIjd/TmWU8mqS9kz s24pLd/yNJ+PzbXdSmsodE1gyNPo5QjjOj1A1QJRfjw+pVAdUcQUFgmCd0cDiqVBLc2e lSR04AJiEAbQFeeOvrD+TlnLpgsw8x1gg5G3vfEKnihWOeFni6afnEnHjoBW5wKPRe3a yfQgQOmjW+8xqdoEebnX21mKYCf7rhnLgzLxvFKBPrrn+8xBmM7O6B15sEvJLW1+crh1 AQ479WFkw3UoCzwgCS719d8ve5JEKf5gdKvr9GrQWW97GSpWyw31nmnBIPNzqJCe3hGE /jNA== Received: by 10.205.118.14 with SMTP id fo14mr5136582bkc.130.1343727664214; Tue, 31 Jul 2012 02:41:04 -0700 (PDT) Received: from mavbook.mavhome.dp.ua (93-127-73-162.static.vega-ua.net. [93.127.73.162]) by mx.google.com with ESMTPS id fu8sm4950176bkc.5.2012.07.31.02.41.02 (version=TLSv1/SSLv3 cipher=OTHER); Tue, 31 Jul 2012 02:41:03 -0700 (PDT) Sender: Alexander Motin Message-ID: <5017A82B.3040704@FreeBSD.org> Date: Tue, 31 Jul 2012 12:40:59 +0300 From: Alexander Motin User-Agent: Mozilla/5.0 (X11; FreeBSD amd64; rv:13.0) Gecko/20120628 Thunderbird/13.0.1 MIME-Version: 1.0 To: Konstantin Belousov References: <201207301350.q6UDobCI099069@svn.freebsd.org> <20120730143943.GY2676@deviant.kiev.zoral.com.ua> <20120730145912.GZ2676@deviant.kiev.zoral.com.ua> <20120731093735.GB2676@deviant.kiev.zoral.com.ua> In-Reply-To: <20120731093735.GB2676@deviant.kiev.zoral.com.ua> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Cc: Attilio Rao , Davide Italiano , src-committers@freebsd.org, svn-src-projects@freebsd.org Subject: Re: svn commit: r238907 - projects/calloutng/sys/kern X-BeenThere: svn-src-projects@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: "SVN commit messages for the src " projects" tree" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 31 Jul 2012 09:41:06 -0000 On 31.07.2012 12:37, Konstantin Belousov wrote: > On Mon, Jul 30, 2012 at 09:48:08PM +0100, Attilio Rao wrote: >> On Mon, Jul 30, 2012 at 3:59 PM, Konstantin Belousov >> wrote: >>> On Mon, Jul 30, 2012 at 03:51:22PM +0100, Attilio Rao wrote: >>>> On 7/30/12, Konstantin Belousov wrote: >>>>> On Mon, Jul 30, 2012 at 03:24:26PM +0100, Attilio Rao wrote: >>>>>> On 7/30/12, Davide Italiano wrote: >>>>>>> On Mon, Jul 30, 2012 at 4:02 PM, Attilio Rao >>>>>>> wrote: >>>>>>> Thanks for the comment, Attilio. >>>>>>> Yes, it's exactly what you thought. If direct flag is equal to one >>>>>>> you're sure you're processing a callout which runs directly from >>>>>>> hardware interrupt context. In this case, the running thread cannot >>>>>>> sleep and it's likely you have TDP_NOSLEEPING flags set, failing the >>>>>>> KASSERT() in THREAD_NO_SLEEPING() and leading to panic if kernel is >>>>>>> compiled with INVARIANTS. >>>>>>> In case you're running from SWI context (direct equals to zero) code >>>>>>> remains the same as before. >>>>>>> I think what I'm doing works due the assumption thread running never >>>>>>> sleeps. Do you suggest some other way to handle this? >>>>>> >>>>>> Possibly the quicker way to do this is to have a way to deal with the >>>>>> TDP_NOSLEEPING flag in recursed way, thus implement the same logic as >>>>>> VFS_LOCK_GIANT() does, for example. >>>>>> You will need to change the few callers of THREAD_NO_SLEEPING(), but >>>>>> the patch should be no longer than 10/15 lines. >>>>> >>>>> There are already curthread_pflags_set/restore KPI designed exactly to >>>>> handle >>>>> nested private thread flags. >>>> >>>> Yes, however I would use curthread_pflags* KPI within >>>> THREAD_NO_SLEEPING() as this name is much more explicit. >>>> >>> Sure, hiding it in THREAD_NO_SLEEPING (THREAD_NO_SLEEP_ENTER/LEAVE ?) >>> is the way to use curthread_pflags_set there. >>> >>> As a second though, on the other hand, is it safe to modify td_flags >>> from the interrupt context at all ? Probably yes if interrupt handler >>> always leave td_pflags in the same state on leave as it was on entry, >>> but couldn't too smart compiler cause inconsistent view of td_pflags >>> inside the handler ? >> >> Can you think of any? Because I cannot think of a case where a nested >> interrupt can messup with already compiled code, unless it leaks a >> cleanup. > In principle, compiler might compile the > x |= a; > into whatever it finds suitable, e.g. it could write 0 temporary into > x if the corresponding instruction sequence is considered faster. > > No sane compiler for x86 does this. >> >> I was more worried about the compiler reordering operations before >> locking could really see it, but I think in this case the functions >> call to sleepqueue (at least) works as a sequence point so we are >> safe. >> >>> >>>>> Also, I wonder, should you assert somehow that direct dispatch cannot block >>>>> as well ? >>>> >>>> Yes, it would be optimal, but I don't think we have a flag for that >>>> right now, do we? >>> >>> I am not aware of such flag, this might be a good reason to introduce it, >>> if issue about td_pflags is just a product of my imagination. >> >> I think you should be good to go. Do you plan to work on such a patch? > > Ok, I looked closely at the direct dispatch and TD_NOBLOCKING. I now > think that such flag is not needed. > > Am I right that direct dispatch executes callback while owning cc_lock > spinlock ? No, does not now. It was so originally, but was fixed recently, as it caused LOR deadlocks. > If true, then TD_NOBLOCKING is definitely not needed for > direct dispatch. For thread to be blocked, it shall be scheduled off the > CPU, going through mi_switch(). And mi_switch() asserts that critical > section level is exactly 1, which assertion fails due to direct dispatch > context owning spinlock. -- Alexander Motin