svn commit: r238907 - projects/calloutng/sys/kern

[Konstantin Belousov]

On Mon, Jul 30, 2012 at 09:48:08PM +0100, Attilio Rao wrote:
> On Mon, Jul 30, 2012 at 3:59 PM, Konstantin Belousov
> <kostikbel at gmail.com> wrote:
> > On Mon, Jul 30, 2012 at 03:51:22PM +0100, Attilio Rao wrote:
> >> On 7/30/12, Konstantin Belousov <kostikbel at gmail.com> wrote:
> >> > On Mon, Jul 30, 2012 at 03:24:26PM +0100, Attilio Rao wrote:
> >> >> On 7/30/12, Davide Italiano <davide at freebsd.org> wrote:
> >> >> > On Mon, Jul 30, 2012 at 4:02 PM, Attilio Rao <attilio at 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.
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 ? 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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 196 bytes
Desc: not available
Url : http://lists.freebsd.org/pipermail/svn-src-projects/attachments/20120731/bc9b59c6/attachment.pgp