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

[Alexander Motin]

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.

> 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