Some proposed mm edits to sort out address space locking

[art yerkes]

Since our MmNotPresentFault handler and kin need to lock the
associated MADDRESS_SPACE, we often need to lock an address space
when it's already been locked.  Until now, this has caused a
recursive acquisition of a kernel mutex.

The current kernel bugchecks for me when running the wget binary
here:

http://www.superheterodyne.net/reactos/mm_edit/wget.exe

So I've added a boolean to MADDRESS_SPACE to specify that it's been
locked and propogated its use (and also disentangled this flag from
the one specifying that pages are locked in various cases).

The patch is here:

http://www.superheterodyne.net/reactos/mm_edit/mm.diff

There's a sore spot in section.c at line 684 where I got a bugcheck,
but it was easy enough to check for a NULL region.  This probably
isn't right however.

jimtabor was seeing bugchecks at startup that are at least
apparently solved by this patch, but I'm sure that those more in
tune with the kernel can weigh in on whether we should do something
like this or whether the real problem is deeper or different.
-- 
Discordant is the murmur at such treading down of lovely things while
god's most lordly gift to man is decency of mind.  Call that man only
blest who has in sweet tranquility brought his life to close.
If only I could act as such, my hope is good.
-- Aeschylus' Agamemnon (translated by H. W. Smyth)

[Filip Navara]

----- PŮVODNÍ ZPRÁVA -----
Od: "art yerkes" [e-mail]
Komu: [e-mail]
Předmět: [ros-dev] Some proposed mm edits to sort out address space
Datum: 21.6.2006 - 2:46:48
> Since our MmNotPresentFault handler and kin need to
> lock the
> associated MADDRESS_SPACE, we often need to lock an
> address space
> when it's already been locked.  Until now, this has
> caused a
> recursive acquisition of a kernel mutex.

I have never seen this happen in practice. That's also why we had the
FromMdl parameter here to avoid it in the cases where it was supposed
to happen.
> The current kernel bugchecks for me when running the
> wget binary
> here:
>
> http://www.superheterodyne.net/reactos/mm_edit/wget.exe
>
> So I've added a boolean to MADDRESS_SPACE to specify
> that it's been
> locked and propogated its use (and also disentangled
> this flag from
> the one specifying that pages are locked in various
> cases).
>
> The patch is here:
>
> http://www.superheterodyne.net/reactos/mm_edit/mm.diff

The patch looks wrong. Not only it effectively kills the locks (because
you don't check for thread affinity, so it's like not locking at all
... not to mention that it wasn't thread-safe either) ...

... but also i disagree with recursive locking (even if it was implemented
correcty) since you should never cause a page fault or anything when
you're holding the lock.

[snip]

Best regards,
Filip