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

[Attilio Rao]

On Sat, Sep 8, 2012 at 7:00 PM, Konstantin Belousov <kostikbel at gmail.com> wrote:
> On Sat, Sep 08, 2012 at 06:14:41PM +0100, Attilio Rao wrote:
>> On Sat, Sep 8, 2012 at 4:38 PM, Konstantin Belousov <kostikbel at gmail.com> wrote:
>> > On Fri, Sep 07, 2012 at 11:35:15PM +0100, Attilio Rao wrote:
>> >> On Mon, Jul 30, 2012 at 9:39 PM, Attilio Rao <attilio at freebsd.org> wrote:
>> >> > On Mon, Jul 30, 2012 at 4:31 PM, Andriy Gapon <avg at freebsd.org> wrote:
>> >> >> on 30/07/2012 18:04 Attilio Rao said the following:
>> >> >>> On 7/30/12, Andriy Gapon <avg at freebsd.org> wrote:
>> >> >>>> on 30/07/2012 17:56 Attilio Rao said the following:
>> >> >>>>> More explicitly, I think such combination TDP_NOSLEEPING +
>> >> >>>>> TDP_NOBLOCKING (name invented) should be set on entering the interrupt
>> >> >>>>> context, not only related to this part of callouts. This would be a
>> >> >>>>> very good help for catching buggy situations.
>> >> >>>>
>> >> >>>> Something very tangential.  I think it would also be nice to check if a
>> >> >>>> thread has
>> >> >>>> any(?) locks held when returning to userland.
>> >> >>>
>> >> >>> This happens already for INVARIANTS case, with td_locks counters.
>> >> >>> In the !INVARIANTS case, this doesn't happen because you don't want to
>> >> >>> add the burden to bump td_locks for the fast case and I think it is a
>> >> >>> good approach.
>> >> >>
>> >> >> Ah, I missed that, thank you.
>> >> >> BTW, it seems that td_locks is checked twice in normal syscallret() path: once in
>> >> >> syscallret() itself and then in userret().  On this note, would it make sense to
>> >> >> move the whole nine yards of asserts from syscallret() to userret()?
>> >> >> I mean it might make sense to have those checks (td_critnest, td_pflags) in other
>> >> >> paths to userland.
>> >> >
>> >> > Nice catch.
>> >> > The checks were added to syscallret() in r208453. While this is fine,
>> >> > I think that putting them in userret() may give them more exposure and
>> >> > cover also cases like traps which are not covered right now.
>> >> > If you want to make a patch that moves these conditions in userret()
>> >> > I'd be in favor of it.
>> >>
>> >> More specifically, what do you think about this patch?:
>> >> http://www.freebsd.org/~attilio/userret_diag.patch
>> >>
>> >> Of course I moved the XEN par too before the checks.
>> >> The patch survived to few consecutive and parallel buildworlds, FWIW.
>> >
>> > At least in fork_return(), the last assert which checks that Giant is not
>> > held, is no longer needed.
>>
>> Actually, this is unnecessary also with -CURRENT stock code today,
>> because userret() already checks for td_locks. And it seems
>> fork_return() is not the only function where this happens, as trap()
>> did this too on x86 and possibly all the other architectures grew it
>> with cut&paste. Possibly we need this further, separate, patch before
>> userred_diag:
>> http://www.freebsd.org/~attilio/userret_nogiant.patch
> Yes, this looks good. I suggest to move #ifdef XEN part of the userret()
> to befire KASSERT(td->td_locks == 0).

All committed as r240244,240245,240246.

Thanks,
Attilio


-- 
Peace can only be achieved by understanding - A. Einstein