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, <tkreu...@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); > > > >
_______________________________________________ Ros-dev mailing list Ros-dev@reactos.org http://www.reactos.org/mailman/listinfo/ros-dev
