Date: Mon, 30 Jul 2012 22:03:15 +0100 From: Attilio Rao <attilio@freebsd.org> To: Davide Italiano <davide@freebsd.org> Cc: Konstantin Belousov <kostikbel@gmail.com>, svn-src-projects@freebsd.org, src-committers@freebsd.org Subject: Re: svn commit: r238907 - projects/calloutng/sys/kern Message-ID: <CAJ-FndCmFmbpN7yNBXxr5BChUtwvxeKsm1HXBJFV1WXVEFDCqQ@mail.gmail.com> In-Reply-To: <CACYV=-H4Z9XXYYvWRfyFCg3O0s7yHGgOcyH4OBC7HAEx0083iA@mail.gmail.com> References: <201207301350.q6UDobCI099069@svn.freebsd.org> <CAJ-FndBJNNBNDUEDsDBUvwoVExZpnXmoJmpY58gE3QQbw3hRGA@mail.gmail.com> <CACYV=-HmOwZ=E8Pw3-mUw0994SbvZaA3eMfcwM0fDTu_zykBJg@mail.gmail.com> <CAJ-FndBmXkyJJ=fCkEpVm84E56A2_EoM6kbch03e4RMEM6WCGQ@mail.gmail.com> <20120730143943.GY2676@deviant.kiev.zoral.com.ua> <CAJ-FndByYcZ%2BUhnkFT_n2=W=UheqUCi0%2BUAX%2BF07EqbVU=6iDQ@mail.gmail.com> <20120730145912.GZ2676@deviant.kiev.zoral.com.ua> <CAJ-FndAdyL5-29vjkS1deAhc4ewYTmA6tEhXUNh%2BqQzUCcTpGw@mail.gmail.com> <CACYV=-H4Z9XXYYvWRfyFCg3O0s7yHGgOcyH4OBC7HAEx0083iA@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Mon, Jul 30, 2012 at 9:58 PM, Davide Italiano <davide@freebsd.org> wrote: > On Mon, Jul 30, 2012 at 10:48 PM, Attilio Rao <attilio@freebsd.org> wrote: >> On Mon, Jul 30, 2012 at 3:59 PM, Konstantin Belousov >> <kostikbel@gmail.com> wrote: >>> On Mon, Jul 30, 2012 at 03:51:22PM +0100, Attilio Rao wrote: >>>> On 7/30/12, Konstantin Belousov <kostikbel@gmail.com> wrote: >>>> > On Mon, Jul 30, 2012 at 03:24:26PM +0100, Attilio Rao wrote: >>>> >> On 7/30/12, Davide Italiano <davide@freebsd.org> wrote: >>>> >> > On Mon, Jul 30, 2012 at 4:02 PM, Attilio Rao <attilio@freebsd.org> >>>> >> > 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. >> >> 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? > > I may work on that as final part of my GSoC work considering I've an > interest in this. > Though, I could need some guidance and help in review. Can you provide these? Yes, don't worry about it, in general these are type of checks that are not strictly related to your work, O_DIRECT dispatching just offered a good spot for discussing them. If they are still needed to go when you finish GSoC we can start hammering them down. Attilio -- Peace can only be achieved by understanding - A. Einstein
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAJ-FndCmFmbpN7yNBXxr5BChUtwvxeKsm1HXBJFV1WXVEFDCqQ>