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

[Konstantin Belousov]

On Tue, Jul 31, 2012 at 12:40:59PM +0300, Alexander Motin wrote:
> 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
> >><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 ?
> 
> No, does not now. It was so originally, but was fixed recently, as it 
> caused LOR deadlocks.
Hm, ok. Probably I misread the diff.

Anyway, I believe that both direct interrupt dispatch and IPIs take
critical sections around handlers. This should have the same effect
for assertion in the mi_switch().

> 
> >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
-------------- 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/057ef671/attachment.pgp