Re: [ros-diffs] [reactos] 02/02: [NTOS:MM] Get rid of unnecessary MmZeroingPageThreadActive.
"M. Ziggyesque" <[email protected]> Sun, 10 May 2020 16:56:14 +0000
| Newsgroups | gmane.os.reactos.kernel |
|---|---|
| Message-ID | <MN2PR13MB38239A374A6EBA81A0B9F334D2A00@MN2PR13MB3823.namprd13.prod.outlook.com> |
--===============1810393129== Content-Language: en-US Content-Type: multipart/alternative; boundary="_000_MN2PR13MB38239A374A6EBA81A0B9F334D2A00MN2PR13MB3823namp_" --_000_MN2PR13MB38239A374A6EBA81A0B9F334D2A00MN2PR13MB3823namp_ Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable I agree with Alex's reasoning, plus in a multiprocessor environment such pa= ge zeroing may be done asynchronously to other cores or processors attempti= ng to acquire use of that page while its state is inconsistent. Removing it= counts as a security hole, in other words, except for single core processo= rs that have interrupts disabled, that I see. Sent from Outlook Mobile<https://aka.ms/blhgte> ________________________________ From: Ros-dev <[email protected]> on behalf of Alex Ionescu <ionu= cu-XzQKRVe1yT0V+D8aMU/[email protected]> Sent: Sunday, May 10, 2020 12:37:09 PM To: ReactOS Development List <[email protected]>; Thomas Faber <thomas.fa= [email protected]> Cc: Linda Wang <[email protected]> Subject: Re: [ros-dev] [ros-diffs] [reactos] 02/02: [NTOS:MM] Get rid of un= necessary MmZeroingPageThreadActive. I know I probably don't have much weight anymore but, I re-iterate that one of the goals of the project should be to make things as similar as possible for debugging/troubleshooting and learning as Server 2003... which does have this variable. I don't think it's the end of the world to keep it around. Stuff like this also tends to break kdexts.dll. Best regards, Alex Ionescu On Mon, Apr 6, 2020 at 5:15 AM Thomas Faber <[email protected]> wrot= e: > > https://git.reactos.org/?p=3Dreactos.git;a=3Dcommitdiff;h=3D25a5aee86f43e= 9f526d4d360e4e4f35feb9d22b4 > > commit 25a5aee86f43e9f526d4d360e4e4f35feb9d22b4 > Author: Thomas Faber <[email protected]> > AuthorDate: Sat Feb 22 12:31:54 2020 +0100 > Commit: Thomas Faber <[email protected]> > CommitDate: Mon Apr 6 11:13:55 2020 +0200 > > [NTOS:MM] Get rid of unnecessary MmZeroingPageThreadActive. > --- > ntoskrnl/mm/ARM3/miarm.h | 1 - > ntoskrnl/mm/ARM3/mminit.c | 5 ++--- > ntoskrnl/mm/ARM3/pfnlist.c | 3 +-- > ntoskrnl/mm/ARM3/zeropage.c | 3 +-- > 4 files changed, 4 insertions(+), 8 deletions(-) > > diff --git a/ntoskrnl/mm/ARM3/miarm.h b/ntoskrnl/mm/ARM3/miarm.h > index c242b795af6..9d85c42afff 100644 > --- a/ntoskrnl/mm/ARM3/miarm.h > +++ b/ntoskrnl/mm/ARM3/miarm.h > @@ -639,7 +639,6 @@ extern PMMPDE MiHighestUserPde; > extern PFN_NUMBER MmSystemPageDirectory[PPE_PER_PAGE]; > extern PMMPTE MmSharedUserDataPte; > extern LIST_ENTRY MmProcessList; > -extern BOOLEAN MmZeroingPageThreadActive; > extern KEVENT MmZeroingPageEvent; > extern ULONG MmSystemPageColor; > extern ULONG MmProcessColorSeed; > diff --git a/ntoskrnl/mm/ARM3/mminit.c b/ntoskrnl/mm/ARM3/mminit.c > index 7cd7af10838..d72e7186193 100644 > --- a/ntoskrnl/mm/ARM3/mminit.c > +++ b/ntoskrnl/mm/ARM3/mminit.c > @@ -2181,9 +2181,8 @@ MmArmInitSystem(IN ULONG Phase, > /* Initialize the Loader Lock */ > KeInitializeMutant(&MmSystemLoadLock, FALSE); > > - /* Set the zero page event */ > - KeInitializeEvent(&MmZeroingPageEvent, SynchronizationEvent, FAL= SE); > - MmZeroingPageThreadActive =3D FALSE; > + /* Set up the zero page event */ > + KeInitializeEvent(&MmZeroingPageEvent, NotificationEvent, FALSE)= ; > > /* Initialize the dead stack S-LIST */ > InitializeSListHead(&MmDeadStackSListHead); > diff --git a/ntoskrnl/mm/ARM3/pfnlist.c b/ntoskrnl/mm/ARM3/pfnlist.c > index eac3a043418..f175541a500 100644 > --- a/ntoskrnl/mm/ARM3/pfnlist.c > +++ b/ntoskrnl/mm/ARM3/pfnlist.c > @@ -701,10 +701,9 @@ MiInsertPageInFreeList(IN PFN_NUMBER PageFrameIndex) > ColorTable->Count++; > > /* Notify zero page thread if enough pages are on the free list now = */ > - if ((ListHead->Total >=3D 8) && !(MmZeroingPageThreadActive)) > + if (ListHead->Total >=3D 8) > { > /* Set the event */ > - MmZeroingPageThreadActive =3D TRUE; > KeSetEvent(&MmZeroingPageEvent, IO_NO_INCREMENT, FALSE); > } > > diff --git a/ntoskrnl/mm/ARM3/zeropage.c b/ntoskrnl/mm/ARM3/zeropage.c > index 6d859f6806f..5fe22294199 100644 > --- a/ntoskrnl/mm/ARM3/zeropage.c > +++ b/ntoskrnl/mm/ARM3/zeropage.c > @@ -17,7 +17,6 @@ > > /* GLOBALS *************************************************************= *******/ > > -BOOLEAN MmZeroingPageThreadActive; > KEVENT MmZeroingPageEvent; > > /* PRIVATE FUNCTIONS ***************************************************= *******/ > @@ -73,7 +72,7 @@ MmZeroPageThread(VOID) > { > if (!MmFreePageListHead.Total) > { > - MmZeroingPageThreadActive =3D FALSE; > + KeClearEvent(&MmZeroingPageEvent); > MiReleasePfnLock(OldIrql); > break; > } > _______________________________________________ Ros-dev mailing list [email protected] http://reactos.org/mailman/listinfo/ros-dev --_000_MN2PR13MB38239A374A6EBA81A0B9F334D2A00MN2PR13MB3823namp_ Content-Type: text/html; charset="us-ascii" Content-Transfer-Encoding: quoted-printable <html> <head> <meta http-equiv=3D"Content-Type" content=3D"text/html; charset=3Dus-ascii"= > </head> <body> <div dir=3D"auto" style=3D"direction: ltr; margin: 0; padding: 0; font-fami= ly: sans-serif; font-size: 11pt; color: black; "> I agree with Alex's reasoning, plus in a multiprocessor environment such pa= ge zeroing may be done asynchronously to other cores or processors attempti= ng to acquire use of that page while its state is inconsistent. Removing it= counts as a security hole, in other words, except for single core processors that have interrupts disabled, th= at I see.<br> <br> </div> <div dir=3D"auto" style=3D"direction: ltr; margin: 0; padding: 0; font-fami= ly: sans-serif; font-size: 11pt; color: black; "> <span id=3D"OutlookSignature"> <div dir=3D"auto" style=3D"direction: ltr; margin: 0; padding: 0; font-fami= ly: sans-serif; font-size: 11pt; color: black; "> Sent from <a href=3D"https://aka.ms/blhgte">Outlook Mobile</a></div> </span><br> </div> <hr style=3D"display:inline-block;width:98%" tabindex=3D"-1"> <div id=3D"divRplyFwdMsg" dir=3D"ltr"><font face=3D"Calibri, sans-serif" st= yle=3D"font-size:11pt" color=3D"#000000"><b>From:</b> Ros-dev <ros-dev-b= [email protected]> on behalf of Alex Ionescu <ionucu-XzQKRVe1yT0V+D8aMU/[email protected]>= ;<br> <b>Sent:</b> Sunday, May 10, 2020 12:37:09 PM<br> <b>To:</b> ReactOS Development List <[email protected]>; Thomas Fab= er <[email protected]><br> <b>Cc:</b> Linda Wang <[email protected]><br> <b>Subject:</b> Re: [ros-dev] [ros-diffs] [reactos] 02/02: [NTOS:MM] Get ri= d of unnecessary MmZeroingPageThreadActive.</font> <div> </div> </div> <div class=3D"BodyFragment"><font size=3D"2"><span style=3D"font-size:11pt;= "> <div class=3D"PlainText">I know I probably don't have much weight anymore b= ut, I re-iterate<br> that one of the goals of the project should be to make things as<br> similar as possible for debugging/troubleshooting and learning as<br> Server 2003... which does have this variable. I don't think it's the<br> end of the world to keep it around. Stuff like this also tends to<br> break kdexts.dll.<br> <br> Best regards,<br> Alex Ionescu<br> <br> On Mon, Apr 6, 2020 at 5:15 AM Thomas Faber <[email protected]>= ; wrote:<br> ><br> > <a href=3D"https://git.reactos.org/?p=3Dreactos.git;a=3Dcommitdiff;h= =3D25a5aee86f43e9f526d4d360e4e4f35feb9d22b4"> https://git.reactos.org/?p=3Dreactos.git;a=3Dcommitdiff;h=3D25a5aee86f43e9f= 526d4d360e4e4f35feb9d22b4</a><br> ><br> > commit 25a5aee86f43e9f526d4d360e4e4f35feb9d22b4<br> > Author: Thomas Faber <thomas.faber@reactos.= org><br> > AuthorDate: Sat Feb 22 12:31:54 2020 +0100<br> > Commit: Thomas Faber <thomas.faber@reactos.= org><br> > CommitDate: Mon Apr 6 11:13:55 2020 +0200<br> ><br> > [NTOS:MM] Get rid of unnecessary MmZeroingPage= ThreadActive.<br> > ---<br> > ntoskrnl/mm/ARM3/miarm.h | 1 -<br> > ntoskrnl/mm/ARM3/mminit.c | 5 ++---<br> > ntoskrnl/mm/ARM3/pfnlist.c | 3 +--<br> > ntoskrnl/mm/ARM3/zeropage.c | 3 +--<br> > 4 files changed, 4 insertions(+), 8 deletions(-)<br> ><br> > diff --git a/ntoskrnl/mm/ARM3/miarm.h b/ntoskrnl/mm/ARM3/miarm.h<br> > index c242b795af6..9d85c42afff 100644<br> > --- a/ntoskrnl/mm/ARM3/miarm.h<br> > +++ b/ntoskrnl/mm/ARM3/miarm.h<br> > @@ -639,7 +639,6 @@ extern PMMPDE MiHighestUserPde;<br> > extern PFN_NUMBER MmSystemPageDirectory[PPE_PER_PAGE];<br> > extern PMMPTE MmSharedUserDataPte;<br> > extern LIST_ENTRY MmProcessList;<br> > -extern BOOLEAN MmZeroingPageThreadActive;<br> > extern KEVENT MmZeroingPageEvent;<br> > extern ULONG MmSystemPageColor;<br> > extern ULONG MmProcessColorSeed;<br> > diff --git a/ntoskrnl/mm/ARM3/mminit.c b/ntoskrnl/mm/ARM3/mminit.c<br> > index 7cd7af10838..d72e7186193 100644<br> > --- a/ntoskrnl/mm/ARM3/mminit.c<br> > +++ b/ntoskrnl/mm/ARM3/mminit.c<br> > @@ -2181,9 +2181,8 @@ MmArmInitSystem(IN ULONG Phase,<br> > /* Initialize th= e Loader Lock */<br> > KeInitializeMuta= nt(&MmSystemLoadLock, FALSE);<br> ><br> > - /* Set the zero page event= */<br> > - KeInitializeEvent(&MmZ= eroingPageEvent, SynchronizationEvent, FALSE);<br> > - MmZeroingPageThreadActive = =3D FALSE;<br> > + /* Set up the zero pag= e event */<br> > + KeInitializeEvent(&= ;MmZeroingPageEvent, NotificationEvent, FALSE);<br> ><br> > /* Initialize th= e dead stack S-LIST */<br> > InitializeSListH= ead(&MmDeadStackSListHead);<br> > diff --git a/ntoskrnl/mm/ARM3/pfnlist.c b/ntoskrnl/mm/ARM3/pfnlist.c<b= r> > index eac3a043418..f175541a500 100644<br> > --- a/ntoskrnl/mm/ARM3/pfnlist.c<br> > +++ b/ntoskrnl/mm/ARM3/pfnlist.c<br> > @@ -701,10 +701,9 @@ MiInsertPageInFreeList(IN PFN_NUMBER PageFram= eIndex)<br> > ColorTable->Count++;<br> ><br> > /* Notify zero page thread if enough pag= es are on the free list now */<br> > - if ((ListHead->Total >=3D 8) && !(Mm= ZeroingPageThreadActive))<br> > + if (ListHead->Total >=3D 8)<br> > {<br> > /* Set the event= */<br> > - MmZeroingPageThreadActive = =3D TRUE;<br> > KeSetEvent(&= MmZeroingPageEvent, IO_NO_INCREMENT, FALSE);<br> > }<br> ><br> > diff --git a/ntoskrnl/mm/ARM3/zeropage.c b/ntoskrnl/mm/ARM3/zeropage.c= <br> > index 6d859f6806f..5fe22294199 100644<br> > --- a/ntoskrnl/mm/ARM3/zeropage.c<br> > +++ b/ntoskrnl/mm/ARM3/zeropage.c<br> > @@ -17,7 +17,6 @@<br> ><br> > /* GLOBALS *****************************************************= ***************/<br> ><br> > -BOOLEAN MmZeroingPageThreadActive;<br> > KEVENT MmZeroingPageEvent;<br> ><br> > /* PRIVATE FUNCTIONS *******************************************= ***************/<br> > @@ -73,7 +72,7 @@ MmZeroPageThread(VOID)<br> > {<br> >  = ; if (!MmFreePageListHead.Total)<br> >  = ; {<br> > - &nb= sp; MmZeroingPageThreadActive =3D FALSE;<br> > +  = ; KeClearEvent(&MmZeroingPageEvent);<br> >  = ; MiReleasePfnLock(OldIrql);<br> >  = ; break;<br> >  = ; }<br> ><br> <br> _______________________________________________<br> Ros-dev mailing list<br> [email protected]<br> <a href=3D"http://reactos.org/mailman/listinfo/ros-dev">http://reactos.org/= mailman/listinfo/ros-dev</a><br> </div> </span></font></div> </body> </html> --_000_MN2PR13MB38239A374A6EBA81A0B9F334D2A00MN2PR13MB3823namp_-- --===============1810393129== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Ros-dev mailing list [email protected] http://reactos.org/mailman/listinfo/ros-dev --===============1810393129==--