11 Oct
2014
11 Oct
'14
4:41 p.m.
I think it would be really great that instead of combining functions
together for "removing code duplication" and not even bothering to think
why there's a reason why the code was duplicated, people would work on
actual improvements like Jerome or Pierre, instead of re-factoring code
that works just to make it 'simpler'. Especially since this introduced a
few bugs which had to be later fixed. There's a million bugs in ReactOS,
including in the memory manager, so fixing those seems to make more sense
than refactoring.
Just my 2 cents.
Best regards,
Alex Ionescu
On Tue, Oct 7, 2014 at 5:31 PM, <tkreuzer(a)svn.reactos.org> wrote:
Author: tkreuzer
Date: Wed Oct 8 00:31:17 2014
New Revision: 64589
URL: http://svn.reactos.org/svn/reactos?rev=64589&view=rev
Log:
[NTOSKRNL]
Implement MiInsertVadEx, replacing duplicated code from
NtAllocateVirtualMemory and MiMapViewOfDataSection
Modified:
trunk/reactos/ntoskrnl/mm/ARM3/miarm.h
trunk/reactos/ntoskrnl/mm/ARM3/section.c
trunk/reactos/ntoskrnl/mm/ARM3/vadnode.c
trunk/reactos/ntoskrnl/mm/ARM3/virtual.c
Modified: trunk/reactos/ntoskrnl/mm/ARM3/miarm.h
URL:
http://svn.reactos.org/svn/reactos/trunk/reactos/ntoskrnl/mm/ARM3/miarm.h?r…
==============================================================================
--- trunk/reactos/ntoskrnl/mm/ARM3/miarm.h [iso-8859-1] (original)
+++ trunk/reactos/ntoskrnl/mm/ARM3/miarm.h [iso-8859-1] Wed Oct 8
00:31:17 2014
@@ -2144,6 +2144,16 @@
IN PEPROCESS Process
);
+NTSTATUS
+NTAPI
+MiInsertVadEx(
+ _Inout_ PMMVAD Vad,
+ _In_ ULONG_PTR *BaseAddress,
+ _In_ SIZE_T ViewSize,
+ _In_ ULONG_PTR HighestAddress,
+ _In_ ULONG_PTR Alignment,
+ _In_ ULONG AllocationType);
+
VOID
NTAPI
MiInsertBasedSection(
Modified: trunk/reactos/ntoskrnl/mm/ARM3/section.c
URL:
http://svn.reactos.org/svn/reactos/trunk/reactos/ntoskrnl/mm/ARM3/section.c…
==============================================================================
--- trunk/reactos/ntoskrnl/mm/ARM3/section.c [iso-8859-1] (original)
+++ trunk/reactos/ntoskrnl/mm/ARM3/section.c [iso-8859-1] Wed Oct 8
00:31:17 2014
@@ -1252,9 +1252,7 @@
IN ULONG AllocationType)
{
PMMVAD_LONG Vad;
- PETHREAD Thread = PsGetCurrentThread();
- PMMSUPPORT AddressSpace;
- ULONG_PTR StartAddress, EndingAddress;
+ ULONG_PTR StartAddress;
ULONG_PTR ViewSizeInPages;
PSUBSECTION Subsection;
PSEGMENT Segment;
@@ -1263,8 +1261,6 @@
ULONG QuotaCharge = 0, QuotaExcess = 0;
PMMPTE PointerPte, LastPte;
MMPTE TempPte;
- PMMADDRESS_NODE Parent;
- TABLE_SEARCH_RESULT Result;
DPRINT("Mapping ARM3 data section\n");
/* Get the segment for this section */
@@ -1361,6 +1357,7 @@
/* Write all the data required in the VAD for handling a fault */
Vad->ControlArea = ControlArea;
+ Vad->u.VadFlags.CommitCharge = 0;
Vad->u.VadFlags.Protection = ProtectionMask;
Vad->u2.VadFlags2.FileOffset = (ULONG)(SectionOffset->QuadPart >> 16);
Vad->u2.VadFlags2.Inherit = (InheritDisposition == ViewShare);
@@ -1437,100 +1434,23 @@
StartAddress = 0;
}
- /* Lock the address space and make sure the process is alive */
- AddressSpace = MmGetCurrentAddressSpace();
- MmLockAddressSpace(AddressSpace);
- if (Process->VmDeleted)
- {
- MmUnlockAddressSpace(AddressSpace);
- ExFreePoolWithTag(Vad, 'ldaV');
- DPRINT1("The process is dying\n");
- return STATUS_PROCESS_IS_TERMINATING;
- }
-
- /* Did the caller specify an address? */
- if (StartAddress == 0)
- {
- /* ARM3 does not support these flags yet */
- ASSERT(Process->VmTopDown == 0);
- ASSERT(ZeroBits == 0);
-
- /* Which way should we search? */
- if (AllocationType & MEM_TOP_DOWN)
- {
- /* No, find an address top-down */
- Result = MiFindEmptyAddressRangeDownTree(*ViewSize,
-
(ULONG_PTR)MM_HIGHEST_VAD_ADDRESS,
- _64K,
- &Process->VadRoot,
- &StartAddress,
- &Parent);
- }
- else
- {
- /* No, find an address bottom-up */
- Result = MiFindEmptyAddressRangeInTree(*ViewSize,
- _64K,
- &Process->VadRoot,
- &Parent,
- &StartAddress);
- }
-
- /* Check if we found a suitable location */
- if (Result == TableFoundNode)
- {
- DPRINT1("Not enough free space to insert this section!\n");
- MmUnlockAddressSpace(AddressSpace);
- MiDereferenceControlArea(ControlArea);
- ExFreePoolWithTag(Vad, 'ldaV');
- return STATUS_CONFLICTING_ADDRESSES;
- }
-
- /* Get the ending address, which is the last piece we need for
the VAD */
- EndingAddress = StartAddress + (ViewSizeInPages * PAGE_SIZE) - 1;
- }
- else
- {
- /* Get the ending address, which is the last piece we need for
the VAD */
- EndingAddress = StartAddress + (ViewSizeInPages * PAGE_SIZE) - 1;
-
- /* Make sure it doesn't conflict with an existing allocation */
- Result = MiCheckForConflictingNode(StartAddress >> PAGE_SHIFT,
- EndingAddress >> PAGE_SHIFT,
- &Process->VadRoot,
- &Parent);
- if (Result == TableFoundNode)
- {
- DPRINT1("Conflict with SEC_BASED or manually based
section!\n");
- MmUnlockAddressSpace(AddressSpace);
- MiDereferenceControlArea(ControlArea);
- ExFreePoolWithTag(Vad, 'ldaV');
- return STATUS_CONFLICTING_ADDRESSES;
- }
- }
-
- /* Now set the VAD address */
- Vad->StartingVpn = StartAddress >> PAGE_SHIFT;
- Vad->EndingVpn = EndingAddress >> PAGE_SHIFT;
-
- /* Pretend as if we own the working set */
- MiLockProcessWorkingSetUnsafe(Process, Thread);
-
/* Insert the VAD */
- Process->VadRoot.NodeHint = Vad;
- MiInsertNode(&Process->VadRoot, (PVOID)Vad, Parent, Result);
-
- /* Release the working set */
- MiUnlockProcessWorkingSetUnsafe(Process, Thread);
-
- /* Unlock the address space */
- MmUnlockAddressSpace(AddressSpace);
+ Status = MiInsertVadEx((PMMVAD)Vad,
+ &StartAddress,
+ ViewSizeInPages * PAGE_SIZE,
+ MAXULONG_PTR >> ZeroBits,
+ MM_VIRTMEM_GRANULARITY,
+ AllocationType);
+ if (!NT_SUCCESS(Status))
+ {
+ return Status;
+ }
/* Windows stores this for accounting purposes, do so as well */
if (!Segment->u2.FirstMappedVa) Segment->u2.FirstMappedVa =
(PVOID)StartAddress;
/* Finally, let the caller know where, and for what size, the view
was mapped */
- *ViewSize = (ULONG_PTR)EndingAddress - (ULONG_PTR)StartAddress + 1;
+ *ViewSize = ViewSizeInPages * PAGE_SIZE;
*BaseAddress = (PVOID)StartAddress;
DPRINT("Start and region: 0x%p, 0x%p\n", *BaseAddress, *ViewSize);
return STATUS_SUCCESS;
Modified: trunk/reactos/ntoskrnl/mm/ARM3/vadnode.c
URL:
http://svn.reactos.org/svn/reactos/trunk/reactos/ntoskrnl/mm/ARM3/vadnode.c…
==============================================================================
--- trunk/reactos/ntoskrnl/mm/ARM3/vadnode.c [iso-8859-1] (original)
+++ trunk/reactos/ntoskrnl/mm/ARM3/vadnode.c [iso-8859-1] Wed Oct 8
00:31:17 2014
@@ -193,6 +193,138 @@
MiInsertNode(&Process->VadRoot, (PVOID)Vad, Parent, Result);
}
+NTSTATUS
+NTAPI
+MiInsertVadEx(
+ _Inout_ PMMVAD Vad,
+ _In_ ULONG_PTR *BaseAddress,
+ _In_ SIZE_T ViewSize,
+ _In_ ULONG_PTR HighestAddress,
+ _In_ ULONG_PTR Alignment,
+ _In_ ULONG AllocationType)
+{
+ ULONG_PTR StartingAddress, EndingAddress;
+ PEPROCESS CurrentProcess;
+ PETHREAD CurrentThread;
+ TABLE_SEARCH_RESULT Result;
+ PMMADDRESS_NODE Parent;
+
+ /* Align the view size to pages */
+ ViewSize = ALIGN_UP_BY(ViewSize, PAGE_SIZE);
+
+ /* Get the current process */
+ CurrentProcess = PsGetCurrentProcess();
+
+ /* Acquire the address creation lock and make sure the process is
alive */
+ KeAcquireGuardedMutex(&CurrentProcess->AddressCreationLock);
+ if (CurrentProcess->VmDeleted)
+ {
+ KeReleaseGuardedMutex(&CurrentProcess->AddressCreationLock);
+ DPRINT1("The process is dying\n");
+ return STATUS_PROCESS_IS_TERMINATING;
+ }
+
+ /* Did the caller specify an address? */
+ if (*BaseAddress == 0)
+ {
+ /* Make sure HighestAddress is not too large */
+ HighestAddress = min(HighestAddress,
(ULONG_PTR)MM_HIGHEST_VAD_ADDRESS);
+
+ /* Which way should we search? */
+ if ((AllocationType & MEM_TOP_DOWN) || CurrentProcess->VmTopDown)
+ {
+ /* Find an address top-down */
+ Result = MiFindEmptyAddressRangeDownTree(ViewSize,
+ HighestAddress,
+ Alignment,
+
&CurrentProcess->VadRoot,
+ &StartingAddress,
+ &Parent);
+ }
+ else
+ {
+ /* Find an address bottom-up */
+ Result = MiFindEmptyAddressRangeInTree(ViewSize,
+ Alignment,
+
&CurrentProcess->VadRoot,
+ &Parent,
+ &StartingAddress);
+ }
+
+ /* Get the ending address, which is the last piece we need for
the VAD */
+ EndingAddress = StartingAddress + ViewSize - 1;
+
+ /* Check if we found a suitable location */
+ if ((Result == TableFoundNode) || (EndingAddress >
HighestAddress))
+ {
+ DPRINT1("Not enough free space to insert this VAD node!\n");
+ KeReleaseGuardedMutex(&CurrentProcess->AddressCreationLock);
+ return STATUS_CONFLICTING_ADDRESSES;
+ }
+
+ ASSERT(StartingAddress != 0);
+ ASSERT(StartingAddress < (ULONG_PTR)HighestAddress);
+ ASSERT(EndingAddress > StartingAddress);
+ }
+ else
+ {
+ /* Calculate the starting and ending address */
+ StartingAddress = ALIGN_DOWN_BY(*BaseAddress, Alignment);
+ EndingAddress = StartingAddress + ViewSize - 1;
+
+ /* Make sure it doesn't conflict with an existing allocation */
+ Result = MiCheckForConflictingNode(StartingAddress >> PAGE_SHIFT,
+ EndingAddress >> PAGE_SHIFT,
+ &CurrentProcess->VadRoot,
+ &Parent);
+ if (Result == TableFoundNode)
+ {
+ DPRINT1("Given address conflicts with existing node\n");
+ KeReleaseGuardedMutex(&CurrentProcess->AddressCreationLock);
+ return STATUS_CONFLICTING_ADDRESSES;
+ }
+ }
+
+ /* Now set the VAD address */
+ Vad->StartingVpn = StartingAddress >> PAGE_SHIFT;
+ Vad->EndingVpn = EndingAddress >> PAGE_SHIFT;
+
+ /* Check if the VAD is to be secured */
+ if (Vad->u2.VadFlags2.OneSecured)
+ {
+ /* This *must* be a long VAD! */
+ ASSERT(Vad->u2.VadFlags2.LongVad);
+
+ /* Yeah this is retarded, I didn't invent it! */
+ ((PMMVAD_LONG)Vad)->u3.Secured.StartVpn = StartingAddress;
+ ((PMMVAD_LONG)Vad)->u3.Secured.EndVpn = EndingAddress;
+ }
+
+ /* Lock the working set */
+ CurrentThread = PsGetCurrentThread();
+ MiLockProcessWorkingSetUnsafe(CurrentProcess, CurrentThread);
+
+ /* Insert the VAD */
+ CurrentProcess->VadRoot.NodeHint = Vad;
+ MiInsertNode(&CurrentProcess->VadRoot, (PVOID)Vad, Parent, Result);
+
+ /* Release the working set */
+ MiUnlockProcessWorkingSetUnsafe(CurrentProcess, CurrentThread);
+
+ /* Update the process' virtual size, and peak virtual size */
+ CurrentProcess->VirtualSize += ViewSize;
+ if (CurrentProcess->VirtualSize > CurrentProcess->PeakVirtualSize)
+ {
+ CurrentProcess->PeakVirtualSize = CurrentProcess->VirtualSize;
+ }
+
+ /* Unlock the address space */
+ KeReleaseGuardedMutex(&CurrentProcess->AddressCreationLock);
+
+ *BaseAddress = StartingAddress;
+ return STATUS_SUCCESS;
+}
+
VOID
NTAPI
MiInsertBasedSection(IN PSECTION Section)
Modified: trunk/reactos/ntoskrnl/mm/ARM3/virtual.c
URL:
http://svn.reactos.org/svn/reactos/trunk/reactos/ntoskrnl/mm/ARM3/virtual.c…
==============================================================================
--- trunk/reactos/ntoskrnl/mm/ARM3/virtual.c [iso-8859-1] (original)
+++ trunk/reactos/ntoskrnl/mm/ARM3/virtual.c [iso-8859-1] Wed Oct 8
00:31:17 2014
@@ -4304,12 +4304,12 @@
{
PEPROCESS Process;
PMEMORY_AREA MemoryArea;
- PFN_NUMBER PageCount;
PMMVAD Vad = NULL, FoundVad;
NTSTATUS Status;
PMMSUPPORT AddressSpace;
PVOID PBaseAddress;
- ULONG_PTR PRegionSize, StartingAddress, EndingAddress, HighestAddress;
+ ULONG_PTR PRegionSize, StartingAddress, EndingAddress;
+ ULONG_PTR HighestAddress = (ULONG_PTR)MM_HIGHEST_VAD_ADDRESS;
PEPROCESS CurrentProcess = PsGetCurrentProcess();
KPROCESSOR_MODE PreviousMode = KeGetPreviousMode();
PETHREAD CurrentThread = PsGetCurrentThread();
@@ -4319,7 +4319,6 @@
MMPTE TempPte;
PMMPTE PointerPte, PointerPde, LastPte;
TABLE_SEARCH_RESULT Result;
- PMMADDRESS_NODE Parent;
PAGED_CODE();
/* Check for valid Zero bits */
@@ -4544,7 +4543,6 @@
// This is a blind commit, all we need is the region size
//
PRegionSize = ROUND_TO_PAGES(PRegionSize);
- PageCount = BYTES_TO_PAGES(PRegionSize);
EndingAddress = 0;
StartingAddress = 0;
@@ -4563,13 +4561,6 @@
goto FailPathNoLock;
}
}
- else
- {
- //
- // Use the highest VAD address as maximum
- //
- HighestAddress = (ULONG_PTR)MM_HIGHEST_VAD_ADDRESS;
- }
}
else
{
@@ -4579,8 +4570,8 @@
// fall based on the aligned address and the passed in region
size
//
EndingAddress = ((ULONG_PTR)PBaseAddress + PRegionSize - 1) |
(PAGE_SIZE - 1);
- StartingAddress = ROUND_DOWN((ULONG_PTR)PBaseAddress, _64K);
- PageCount = BYTES_TO_PAGES(EndingAddress - StartingAddress);
+ PRegionSize = EndingAddress + 1 -
ROUND_DOWN((ULONG_PTR)PBaseAddress, _64K);
+ StartingAddress = (ULONG_PTR)PBaseAddress;
}
//
@@ -4594,7 +4585,7 @@
goto FailPathNoLock;
}
- Vad->u.LongFlags = 0;
+ RtlZeroMemory(Vad, sizeof(MMVAD_LONG));
if (AllocationType & MEM_COMMIT) Vad->u.VadFlags.MemCommit = 1;
Vad->u.VadFlags.Protection = ProtectionMask;
Vad->u.VadFlags.PrivateMemory = 1;
@@ -4602,124 +4593,24 @@
Vad->ControlArea = NULL; // For Memory-Area hack
//
- // Lock the address space and make sure the process isn't already
dead
- //
- AddressSpace = MmGetCurrentAddressSpace();
- MmLockAddressSpace(AddressSpace);
- if (Process->VmDeleted)
- {
- Status = STATUS_PROCESS_IS_TERMINATING;
- goto FailPath;
- }
-
- //
- // Did we have a base address? If no, find a valid address that
is 64KB
- // aligned in the VAD tree. Otherwise, make sure that the address
range
- // which was passed in isn't already conflicting with an existing
address
- // range.
- //
- if (!PBaseAddress)
- {
- /* Which way should we search? */
- if ((AllocationType & MEM_TOP_DOWN) || Process->VmTopDown)
- {
- /* Find an address top-down */
- Result = MiFindEmptyAddressRangeDownTree(PRegionSize,
- HighestAddress,
- _64K,
-
&Process->VadRoot,
- &StartingAddress,
- &Parent);
- }
- else
- {
- /* Find an address bottom-up */
- Result = MiFindEmptyAddressRangeInTree(PRegionSize,
- _64K,
- &Process->VadRoot,
- &Parent,
- &StartingAddress);
- }
-
- if (Result == TableFoundNode)
- {
- Status = STATUS_NO_MEMORY;
- goto FailPath;
- }
-
- //
- // Now we know where the allocation ends. Make sure it
doesn't end up
- // somewhere in kernel mode.
- //
- ASSERT(StartingAddress != 0);
- ASSERT(StartingAddress < (ULONG_PTR)MM_HIGHEST_USER_ADDRESS);
- EndingAddress = (StartingAddress + PRegionSize - 1) |
(PAGE_SIZE - 1);
- ASSERT(EndingAddress > StartingAddress);
- if (EndingAddress > HighestAddress)
- {
- Status = STATUS_NO_MEMORY;
- goto FailPath;
- }
- }
- else
- {
- /* Make sure it doesn't conflict with an existing allocation
*/
- Result = MiCheckForConflictingNode(StartingAddress >>
PAGE_SHIFT,
- EndingAddress >>
PAGE_SHIFT,
- &Process->VadRoot,
- &Parent);
- if (Result == TableFoundNode)
- {
- //
- // The address specified is in conflict!
- //
- Status = STATUS_CONFLICTING_ADDRESSES;
- goto FailPath;
- }
- }
-
- //
- // Write out the VAD fields for this allocation
- //
- Vad->StartingVpn = StartingAddress >> PAGE_SHIFT;
- Vad->EndingVpn = EndingAddress >> PAGE_SHIFT;
-
- //
- // FIXME: Should setup VAD bitmap
- //
- Status = STATUS_SUCCESS;
-
- //
- // Lock the working set and insert the VAD into the process VAD
tree
- //
- MiLockProcessWorkingSetUnsafe(Process, CurrentThread);
- Process->VadRoot.NodeHint = Vad;
- MiInsertNode(&Process->VadRoot, (PVOID)Vad, Parent, Result);
- MiUnlockProcessWorkingSetUnsafe(Process, CurrentThread);
-
- //
- // Make sure the actual region size is at least as big as the
- // requested region size and update the value
- //
- ASSERT(PRegionSize <= (EndingAddress + 1 - StartingAddress));
- PRegionSize = (EndingAddress + 1 - StartingAddress);
-
- //
- // Update the virtual size of the process, and if this is now the
highest
- // virtual size we have ever seen, update the peak virtual size
to reflect
- // this.
- //
- Process->VirtualSize += PRegionSize;
- if (Process->VirtualSize > Process->PeakVirtualSize)
- {
- Process->PeakVirtualSize = Process->VirtualSize;
- }
-
- //
- // Release address space and detach and dereference the target
process if
+ // Insert the VAD
+ //
+ Status = MiInsertVadEx(Vad,
+ &StartingAddress,
+ PRegionSize,
+ HighestAddress,
+ MM_VIRTMEM_GRANULARITY,
+ AllocationType);
+ if (!NT_SUCCESS(Status))
+ {
+ DPRINT1("Failed to insert the VAD!\n");
+ goto FailPathNoLock;
+ }
+
+ //
+ // Detach and dereference the target process if
// it was different from the current process
//
- MmUnlockAddressSpace(AddressSpace);
if (Attached) KeUnstackDetachProcess(&ApcState);
if (ProcessHandle != NtCurrentProcess())
ObDereferenceObject(Process);