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@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?rev=64589&r1=64588&r2=64589&view=diff
==============================================================================
--- 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?rev=64589&r1=64588&r2=64589&view=diff
==============================================================================
--- 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?rev=64589&r1=64588&r2=64589&view=diff
==============================================================================
--- 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?rev=64589&r1=64588&r2=64589&view=diff
==============================================================================
--- 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);