11 Oct
2014
11 Oct
'14
11:59 p.m.
There is also missing functionality in ReactOS, like missing commit
charge accounting. I'm sure you know how this works, and that the
implementation of the required functions is not a problem at all, but
calling the functions from ALL locations where it needs to be done and
not doing it wrong, is the problem. Sadly ARM3 is not really good at
factoring code, so that handling stuff like this would be easy. Instead
code is duplicated in all sorts of locations, requiring to handle other
thing in multiple locations as well, instead of centralizing it. And I
think I do know the reason why it is done like that: It's the principle:
don't change existing code, rather copy and paste 100 lines into another
location, if you need it, so it is at least guaranteed, that existing
code won't change behavior. This seems to be the MS way of handling
things, to make sure to never break existing applications. But for us
there is no reason to do so. Factoring the code in a more optimal way
would be beneficial, since it will a) help to reduce the places where we
introduce bugs, b) help implementing features like commit charge
accounting, that benefit from cenralized code.
There are always good reasons for my changes. Even refactoring the code
is a good reason, but that is not the final goal of my changes here. You
know that I do not buy the "Windows does it like this" argument. At
least not as countering a real, practical argument. So you need to find
something better :)
And why do you assume, I would not have even bothered "to think why
there's a reason why the code was duplicated"? Of course I have. And I
came to the conclusion that there was no good reason for us to do it the
same way. Or can you please explain why copying basically the same code
into multiple locations is alternativeless, or why I am too stupid to
refactor that code to remove the code duplication? If you can't, then I
don't see any argument, other than complaining that someone changed your
code. I do very well undertand that it's annoying when people change
your code for no apparent reason, but that's not the case here. And you
should know that.
Regarding "fixing those seems to make more sense than refactoring", I
can only say that we all could have flooded you with this argument when
you were working on ARM3. You broke all kinds of things and some of them
are still broken. And that was not hypothetical things, it was real
things! You definately killed the AMD64 port, you killed most paging and
introduced kernel crashes on too much memory usage. So unless you now
want to go ahead and fix everything you broke, I don't buy your argument
"fixing things is better than refactoring". Go ahead and fix things,
then others might follow your example. :-P
Timo
Am 11.10.2014 18:41, schrieb Alex Ionescu:
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
<mailto: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?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);
_______________________________________________
Ros-dev mailing list
Ros-dev(a)reactos.org
http://www.reactos.org/mailman/listinfo/ros-dev