Alex Ionescu wrote:
>>> 1) Why implement new, complex features in the kernel instead of
>>> fixing current bugs?
>>
>> You pay, you get a saying in what I work on. I pay, I decide.
>
>No, this is an open source project, everyone gets a say on what you
>work on -- you're free to ignore such sayings at your leisure.
Seems I was too imprecise (or again a language
barrier/misunderstanding). Sorry. My intention was to communicate that
if you pay, you get the option of (trying to) tell me what to work on.
When I pay, I decide. Obviously anyone is at any time free to comment.
>The quota code is called by the process functions, and maybe even by
>drivers, but it doesn't affect their behavior.
I'm going out on a limb here (as I haven't checked it), but can't a
JOB (artificially) limit available resources? That's part of their
intention, isn't it? (please read on)
>Two choices:
>
>1) Implement a rather complex quota implementation, including commit
>charges in Mm (which is several thousand lines of code in NT),
I knew I got into a rather large area, but this sounds insane for
something as seemingly "simple".
> to a fragile kernel
If it's that fragile it needs to be fixed. If you got a better idea to
do it than implementing previously missing functionality to expose
those fragile parts of it, especially as what I started to work on
apparently exposed those fragile parts, I'm all ears.
> &
> Use up a valuable developer's time, probably a couple man-weeks
Should that be how I chose to spend my time - what's the difference if
I hadn't been around at all? None, except the end result might make
Win32 GlobalMemoryStatus and VM_COUNTERS present useful and usable
data.
> &
> Add new bugs to a hot path during process/object creation/
>deletion (I hope you'll agree all new code has bugs)
Only by convention, not by definition. :-)
>OR
>
>2) Continue working on bug fixing the fragile parts of the kernel and
>OS || Implement critical missing functionality for a target driver/
>application &
This actually started as fixing critical missing functionality (Wax:
feel free to chime in). Currently ROS does not COMMIT on such
allocation requests. It happily allows an exe in a 128MB machine with
32MB pagefile to COMMIT almost 2GB memory.
The error is of course in the way ROS handles COMMIT requests to
VirtualAlloc, in that it defers committing the memory until something
touches it... (that is no joke)
The first step to research this was (for me) to see the behaviour of
GlobalMemoryStatus, and indeed it was broken - it was unimplemented...
So what do I do? I "dig where I stand". I start an attempt implementing
the missing functionality.
So the reason I came to touch Ps, and am digging way deeper in Mm than
I'd really like to, is simply the GlobalMemoryStatus(Ex) Win32 API
function(s), to even be able to verify, from user-mode, behaviour.
[...]
>> On a sidenote, the only public reference to PS_QUOTA_TYPE I could
find
>> were from a PDB file transcript from ntdll, posted at a french
site.
>
>It should be added to the NDK.
Agreed. But as I didn't know if that's still your baby (SVN access or
not) and only synched at intervals, or if it has been "adopted", and if
adding such data types, even if suspected to be correct, from such few
and vague information source was even acceptable... I played it safe
and used just the indices. Thanks for sharing the typename.
If concensus is that adding that data type introduces no problems,
even if from such a vague source, I'm in favor of adding it (as it'd
solve more than one immediate problem).
>>> 7) The quota routines require interlocks
>>
>> Yep. Even requires a lock for checking and updating QuotaPeak.
>
>No it doesn't, you can use Interlocked Compare Exchange.
Are you thinking of the, semantically non-obvious for maintenance
programmers,
OldVal = Interlocked*Add(&Quota, Addend);
InterlockedCompareExchange(&Peak, OldVal + Addend, OldVal + Addend);
?
I think using a spinlock for that would make it much more obvious.
Should profiling display it had a measurable impact on execution, sure.
But we should also keep in mind we are then trading a single spinlock
for two *locked* (bus-snooping/cache-invalidating) instructions. I
don't know which is worse (performance wise).
>>> 8) They also require a global spinlock in the "give back" case.
>>
>> I don't follow. Why? I'd imagine the "give back" case to be simpler
>> than the charge case, as the former only needs to decrement the
usage
>> (InterlockedExchangeAdd with a negative value), while the latter
needs
>> to increment, followed by comparing with and possibly update Peak.
>
>The global spinlock is required when inserting new quota blocks,
>dereferencing them, expanding the quota and giving back the quota.
Ah, right... the quota blocks. As already displayed I didn't touch
them (I honestly don't even know what they are for yet). I only looked
at the (immediate) process quota entries. The linked lists I never
touched.
>I still disagree with trunk-is-a-playground
I have never disagreed with that, so I'm not sure what you're
referring to.
>and I think that pseudo-code should go in branches, but you're free
to disagree.
I suggest you take that discussion with the project coordinator, that
I cleared that diff with and he explicitly stated a preference for it
even after I mentioned "branch".
I disagree with you re. comments. I think pseudo-code in comments (in
trunk even) is to prefer, as it can explain behaviours and intentions
way better than plain english or even documentation. If those comments
help maintenance-programmers ten or more years from now, even if only
one single person, the comments were well spent time and place.
--
Mike
Alex Ionescu wrote:
> 1) Why implement new, complex features in the kernel instead of
> fixing current bugs?
You pay, you get a saying in what I work on. I pay, I decide.
> I had already written all these functions, but deleted them,
Wasn't that a bonehead thing to do?
> because nobody needed them,
... at the time.
> and the Mm part was too complex to do.
Guess what; I'm currently trying to actually implement it for Mm.
Ps was a first step in that direction.
Yes, Mm is complex, and yes I don't understand it enough yet - not
by a longshot. But checking in incremental improvements, so that
the code doesn't get lost, and disabled by default to not affect
the behaviour of trunk - that's a good thing. It means people can
both review and test the incremental changes, and perhaps even
suggest improvements - like you did in pt. 5 below.
> 2) Why are you bothering with PspPoolQuotaIndexFromPoolType?
Ignorance of:
> In case you haven't noticed, there's something called pool masks
PAGED_POOL_MASK. Got it.
> 5) Instead of duplicating code, perhaps consider calling a main
> worker function such as PspChargeQuota?
A sound refactoring suggestion.
> 6) Consider using an enum instead of magic numbers inside the
> quota entries, I believe it's called PS_QUOTA_TYPE.
I just followed the lead of existing code. Had there been any mention
of an enumeration for nonpaged, paged and pagefile, I would obviously
have used those names.
On a sidenote, the only public reference to PS_QUOTA_TYPE I could find
were from a PDB file transcript from ntdll, posted at a french site.
> 7) The quota routines require interlocks
Yep. Even requires a lock for checking and updating QuotaPeak.
> 8) They also require a global spinlock in the "give back" case.
I don't follow. Why? I'd imagine the "give back" case to be simpler
than the charge case, as the former only needs to decrement the usage
(InterlockedExchangeAdd with a negative value), while the latter needs
to increment, followed by comparing with and possibly update Peak.
> 9) Committing code that won't even build, even with the define on,
> isn't very appropriate.
I agree.
> If someone wants to actually test this, they won't be able to build
it
That's an interesting statement. Care to elaborate on how you reached
such a factually wrong conclusion?
> because of things like "refuse"
Some people prefer pseudo-code over nothing at all. Some prefer void.
Some people are able to spot C comment blocks. Some are not.
Me, I checked that in with both the approval and a statement of
preference for this kind of committs by the project coordinator.
It not only compiles, it partially works too.
> SVN should contain code that builds at any time. It's
> not hard to write /* TODO */..
You really need (new?) glasses. You even quoted it yourself!
"/* TODO: Check with Process->QuotaBlock if this can be satisfied, */"
--
Mike
Alex Ionescu wrote:
> Windows uses the FILE_READ_DATA flag here, not FILE_READ_ATTRIBUTES.
Yes? And?
Let's study the code and the open modes used here.
This code tries to open Device\HarddiskN\Partition0 (N = 0 ...) for
the
sole purpose of checking for availability, and immediately close the
handle if the open succeeded.
Why would one use FILE_READ_DATA if one never intended to read
anything?
The code also uses SYNCHRONIZE. Why would one use SYNCHRONIZE if one
never
intended to wait on the handle?
As I see it, that's just requesting extra work for the purpose of
nothing.
The purpose of that open call is only to see if the disk exists, and
if
so create the symbolic link PhysicalDriveN -> HarddiskN\Partition0.
I don't buy "Windows does this" without backing it up with any
arguments whatsoever as to why Windows does this. Perhaps Windows does
more with the returned handle than immediately closing it, something
that actually requires READ_DATA?
> all this is doing is hiding the bug (which is somewhere in vfat)
I'm looking forward to the explanation of how opening
HarddiskN\Partition0 (not "Partition1", not "Partition1\", but
"Partition0" - aka the whole disk) would involve any filesystem
activity whatsoever?
> Because it's now gone, it'll probably happen in some mysterious
> application 2 years from now and nobody will know why.
Hahaha, you really are a joker. An app? Calling
xHalIoAssignDriveLetters, that is called with a frickin' LoaderBlock?
That function is used exactly once, during system startup.
Health reasons suggest not holding your breath while waiting for such
an application. :-)
--
Mike
Windows uses the FILE_READ_DATA flag here, not FILE_READ_ATTRIBUTES.
Silencing a warning does not mean fixing it -- all this is doing is
hiding the bug (which is somewhere in vfat) in a place that's quite
exposed. Because it's now gone, it'll probably happen in some
mysterious application 2 years from now and nobody will know why.
--
Best regards,
Alex Ionescu
On 20-Oct-07, at 3:36 AM, mnordell(a)svn.reactos.org wrote:
> Author: mnordell
> Date: Sat Oct 20 11:36:17 2007
> New Revision: 29702
>
> URL: http://svn.reactos.org/svn/reactos?rev=29702&view=rev
> Log:
> Don't try to open a harddisk for reading when checking for it to
> create the PhysicalDriveN links. Instead, request
> FILE_READ_ATTRIBUTES. This silences a hack-warning in
> IopParseDevice, that now possibly can be removed.
>
> Modified:
> trunk/reactos/ntoskrnl/fstub/disksup.c
> trunk/reactos/ntoskrnl/io/iomgr/file.c
>
> Modified: trunk/reactos/ntoskrnl/fstub/disksup.c
> URL: http://svn.reactos.org/svn/reactos/trunk/reactos/ntoskrnl/
> fstub/disksup.c?rev=29702&r1=29701&r2=29702&view=diff
> ======================================================================
> ========
> --- trunk/reactos/ntoskrnl/fstub/disksup.c (original)
> +++ trunk/reactos/ntoskrnl/fstub/disksup.c Sat Oct 20 11:36:17 2007
> @@ -481,7 +481,7 @@
> NULL);
>
> Status = ZwOpenFile(&FileHandle,
> - FILE_READ_DATA | SYNCHRONIZE,
> + FILE_READ_ATTRIBUTES | SYNCHRONIZE,
> &ObjectAttributes,
> &StatusBlock,
> FILE_SHARE_READ,
>
> Modified: trunk/reactos/ntoskrnl/io/iomgr/file.c
> URL: http://svn.reactos.org/svn/reactos/trunk/reactos/ntoskrnl/io/
> iomgr/file.c?rev=29702&r1=29701&r2=29702&view=diff
> ======================================================================
> ========
> --- trunk/reactos/ntoskrnl/io/iomgr/file.c (original)
> +++ trunk/reactos/ntoskrnl/io/iomgr/file.c Sat Oct 20 11:36:17 2007
> @@ -404,6 +404,9 @@
>
> /* FIXME: Small hack still exists, have to check why...
> * This is triggered multiple times by usetup and then once
> per boot.
> + * TMN: NOTE: It might have been fixed now, by changing the
> requested
> + * openmode in xHalIoAssignDriveLetters from FILE_READ_DATA to
> + * FILE_READ_ATTRIBUTES. If verified, this hack should be
> removed.
> */
> if (!(DirectOpen) &&
> !(RemainingName->Length) &&
>
>
Couple of comments:
1) Why implement new, complex features in the kernel instead of
fixing current bugs? I know it's fun to start hacking on new code,
but priorities should be to implement stuff that's required not to
break other apps/add functionality, not internal features which will
take ages to implement (the Ps functions for quota are "easy", it's
the Mm part that's bad).
I had already written all these functions, but deleted them, because
nobody needed them, and the Mm part was too complex to do.
2) Why are you bothering with PspPoolQuotaIndexFromPoolType? In case
you haven't noticed, there's something called pool masks which make
the operation very easy without requiring a function call (such as
POOL_TYPE_MASK or BASE_POOL_MASK, forgot how it's called).
3) INT is not an acceptable kernel type.
4) We don't use "static" in the kernel.
5) Instead of duplicating code, perhaps consider calling a main
worker function such as PspChargeQuota?
6) Consider using an enum instead of magic numbers inside the quota
entries, I believe it's called PS_QUOTA_TYPE.
7) The quota routines require interlocks, otherwise they will not be
thread safe.
8) They also require a global spinlock in the "give back" case.
9) Committing code that won't even build, even with the define on,
isn't very appropriate. Trunk isn't a playground. If someone wants to
actually test this, they won't be able to build it because of things
like "refuse". SVN should contain code that builds at any time. It's
not hard to write /* TODO */..
Just my 2 cents
--
Best regards,
Alex Ionescu
On 23-Oct-07, at 6:05 AM, mnordell(a)svn.reactos.org wrote:
> Author: mnordell
> Date: Tue Oct 23 14:05:40 2007
> New Revision: 29826
>
> URL: http://svn.reactos.org/svn/reactos?rev=29826&view=rev
> Log:
> First small attempt at implementing process memory quota. Currently
> disabled without explicit code modification (enabled by macro) to
> not modify behaviour of trunk.
>
> Modified:
> trunk/reactos/ntoskrnl/ps/quota.c
>
> Modified: trunk/reactos/ntoskrnl/ps/quota.c
> URL: http://svn.reactos.org/svn/reactos/trunk/reactos/ntoskrnl/ps/
> quota.c?rev=29826&r1=29825&r2=29826&view=diff
> ======================================================================
> ========
> --- trunk/reactos/ntoskrnl/ps/quota.c (original)
> +++ trunk/reactos/ntoskrnl/ps/quota.c Tue Oct 23 14:05:40 2007
> @@ -15,6 +15,12 @@
>
> EPROCESS_QUOTA_BLOCK PspDefaultQuotaBlock;
>
> +
> +/* Define this macro to enable quota code testing. Once quota code
> is */
> +/* stable and verified, remove this macro and checks for it. */
> +/*#define PS_QUOTA_ENABLE_QUOTA_CODE*/
> +
> +
> /* FUNCTIONS
> ***************************************************************/
>
> VOID
> @@ -66,9 +72,29 @@
> {
> /* Don't do anything for the system process */
> if (Process == PsInitialSystemProcess) return STATUS_SUCCESS;
> -
> +
> +#ifdef PS_QUOTA_ENABLE_QUOTA_CODE
> + if (Process)
> + {
> + /* TODO: Check with Process->QuotaBlock if this can be
> satisfied, */
> + /* assuming this indeed is the place to check it. */
> + /* Probably something like:
> + if (Process->QuotaUsage[2] + Amount >
> + Process->QuotaBlock->QuotaEntry[2].Limit)
> + {
> + refuse
> + }
> + */
> + Process->QuotaUsage[2] += Amount;
> + if (Process->QuotaPeak[2] < Process->QuotaUsage[2])
> + {
> + Process->QuotaPeak[2] = Process->QuotaUsage[2];
> + }
> + }
> +#else
> /* Otherwise, not implemented */
> UNIMPLEMENTED;
> +#endif
> return STATUS_SUCCESS;
> }
>
> @@ -115,6 +141,38 @@
> return PsChargeProcessPoolQuota(Process, PagedPool, Amount);
> }
>
> +#ifdef PS_QUOTA_ENABLE_QUOTA_CODE
> +/*
> + * Internal helper function.
> + * Returns the index of the Quota* member in EPROCESS for
> + * a specified pool type, or -1 on failure.
> + */
> +static
> +INT
> +PspPoolQuotaIndexFromPoolType(POOL_TYPE PoolType)
> +{
> + switch (PoolType)
> + {
> + case NonPagedPool:
> + case NonPagedPoolMustSucceed:
> + case NonPagedPoolCacheAligned:
> + case NonPagedPoolCacheAlignedMustS:
> + case NonPagedPoolSession:
> + case NonPagedPoolMustSucceedSession:
> + case NonPagedPoolCacheAlignedSession:
> + case NonPagedPoolCacheAlignedMustSSession:
> + return 1;
> + case PagedPool:
> + case PagedPoolCacheAligned:
> + case PagedPoolSession:
> + case PagedPoolCacheAlignedSession:
> + return 0;
> + default:
> + return -1;
> + }
> +}
> +#endif
> +
> /*
> * @implemented
> */
> @@ -124,10 +182,32 @@
> IN POOL_TYPE PoolType,
> IN ULONG Amount)
> {
> + INT PoolIndex;
> /* Don't do anything for the system process */
> if (Process == PsInitialSystemProcess) return STATUS_SUCCESS;
>
> - UNIMPLEMENTED;
> +#ifdef PS_QUOTA_ENABLE_QUOTA_CODE
> + PoolIndex = PspPoolQuotaIndexFromPoolType(PoolType);
> + if (Process && PoolIndex != -1)
> + {
> + /* TODO: Check with Process->QuotaBlock if this can be
> satisfied, */
> + /* assuming this indeed is the place to check it. */
> + /* Probably something like:
> + if (Process->QuotaUsage[PoolIndex] + Amount >
> + Process->QuotaBlock->QuotaEntry[PoolIndex].Limit)
> + {
> + refuse
> + }
> + */
> + Process->QuotaUsage[PoolIndex] += Amount;
> + if (Process->QuotaPeak[PoolIndex] < Process->QuotaUsage
> [PoolIndex])
> + {
> + Process->QuotaPeak[PoolIndex] = Process->QuotaUsage
> [PoolIndex];
> + }
> + }
> +#else
> + UNIMPLEMENTED;
> +#endif
> return STATUS_SUCCESS;
> }
>
> @@ -140,10 +220,26 @@
> IN POOL_TYPE PoolType,
> IN ULONG_PTR Amount)
> {
> + INT PoolIndex;
> /* Don't do anything for the system process */
> if (Process == PsInitialSystemProcess) return;
>
> - UNIMPLEMENTED;
> +#ifdef PS_QUOTA_ENABLE_QUOTA_CODE
> + PoolIndex = PspPoolQuotaIndexFromPoolType(PoolType);
> + if (Process && PoolIndex != -1)
> + {
> + if (Process->QuotaUsage[PoolIndex] < Amount)
> + {
> + DPRINT1("WARNING: Process->QuotaUsage sanity check
> failed.\n");
> + }
> + else
> + {
> + Process->QuotaUsage[PoolIndex] -= Amount;
> + }
> + }
> +#else
> + UNIMPLEMENTED;
> +#endif
> }
>
> /*
> @@ -157,7 +253,11 @@
> /* Don't do anything for the system process */
> if (Process == PsInitialSystemProcess) return;
>
> - UNIMPLEMENTED;
> +#ifdef PS_QUOTA_ENABLE_QUOTA_CODE
> + PsReturnPoolQuota(Process, NonPagedPool, Amount);
> +#else
> + UNIMPLEMENTED;
> +#endif
> }
>
> /*
> @@ -171,7 +271,11 @@
> /* Don't do anything for the system process */
> if (Process == PsInitialSystemProcess) return;
>
> - UNIMPLEMENTED;
> +#ifdef PS_QUOTA_ENABLE_QUOTA_CODE
> + PsReturnPoolQuota(Process, PagedPool, Amount);
> +#else
> + UNIMPLEMENTED;
> +#endif
> }
>
> NTSTATUS
> @@ -182,8 +286,22 @@
> /* Don't do anything for the system process */
> if (Process == PsInitialSystemProcess) return STATUS_SUCCESS;
>
> +#ifdef PS_QUOTA_ENABLE_QUOTA_CODE
> + if (Process)
> + {
> + if (Process->QuotaUsage[2] < Amount)
> + {
> + DPRINT1("WARNING: Process PageFileQuotaUsage sanity
> check failed.\n");
> + }
> + else
> + {
> + Process->QuotaUsage[2] -= Amount;
> + }
> + }
> +#else
> /* Otherwise, not implemented */
> UNIMPLEMENTED;
> +#endif
> return STATUS_SUCCESS;
> }
>
>
>
Hi!
Good job! But this is all kinds of type of wrong! There are internal calls for win32k dc.c
and dcutils.c in there. What you should have done was move the DC object structure to ntgdihdl.h.
So, please revert or but back dc.h and then move the DC object to ntgdihld.h where it belongs.
Thanks,
James
Hi!
What was the revision for the floppy driver that made it inoperative?
You told me on IRC. I did not take my notes to work.
> Hello,
>
> I think that 0.3.4 should have most hardware regressions (floppy and
> vga) fixed. I know I'm not the one to say this but, we are already
> closer to this goal than we were with 0.3.3. Fixing the floppy
> regression should only involve finding the spot in which floppy is not
> reporting correctly to ntoskrnl. I did some tests on this issue and it
> fails on line 596-598 of ntoskrnl\io\iomgr\volume.c and floppy was never
> mounted as the status reported was STATUS_IO_DEVICE_ERROR from line
> 1017-1021 in drivers\storage\floppy\floppy.c. I don't know much about
> the vga regression but I'd think that it may only require a few days
> work to find and fix the issue. We already have SATA support included
> with trunk (uniata.sys) and SCSI is also working now.
>
> -aicommander
>
Thanks,
James
These resources have been lifted straight out of the Windows dll.
Absolutley no attempt has been made to change anything.
IMO, this should be removed until someone manually implements it.
Ged.
fireball(a)svn.reactos.org wrote:
> Author: fireball
> Date: Sat Oct 20 21:16:30 2007
> New Revision: 29714
>
> URL: http://svn.reactos.org/svn/reactos?rev=29714&view=rev
> Log:
> Dmitry Chapyshev <lentind(a)yandex.ru>
> - Fully implement tapiui.dll, compatible with Windows.
> - Icons come from SUSE's yast2-industrial; 203,202.ico and 301,302.bmp are done by me based on Tango
>
>
>
Hello,
I think that 0.3.4 should have most hardware regressions (floppy and
vga) fixed. I know I'm not the one to say this but, we are already
closer to this goal than we were with 0.3.3. Fixing the floppy
regression should only involve finding the spot in which floppy is not
reporting correctly to ntoskrnl. I did some tests on this issue and it
fails on line 596-598 of ntoskrnl\io\iomgr\volume.c and floppy was never
mounted as the status reported was STATUS_IO_DEVICE_ERROR from line
1017-1021 in drivers\storage\floppy\floppy.c. I don't know much about
the vga regression but I'd think that it may only require a few days
work to find and fix the issue. We already have SATA support included
with trunk (uniata.sys) and SCSI is also working now.
-aicommander
Hi there,
i am thinking of porting ros to the L4 micro-kernel as topic for my
diploma thesis. The operating systems group in Dresden already ported
Linux to L4 (http://os.inf.tu-dresden.de/L4/LinuxOnL4/) and proofed
that L4 offers all that is needed to run an operating system in
userspace on top of it.
The goal of the work would be to be able to run windows applications
next to miro-kernel applications. And to compare the performance with
that of pure ros, L4Linux/wine and windows. A comparison between the
port and L4Linux could also be part of the work. (mapping processes to
L4 tasks, memory management, interrupt-handling and so on)
I am not very experienced in C or assembler programming but am
motivated to learn. But before i start working on that topic i have
some questions on the portability of ros.
In svn and on the ros website i found that there is a ppc port of ros.
And on irc someone told me that there is ongoing work on a ros usermode
port. I also read about a xen port. But i did not yet find working
copies of the any of that ports. I built ros for i386 on my
machine and was able to boot it in vmware. I also tried building the
latest svn version with ROS_ARCH=powerpc but that did not succeed.
1. Did anyone ever finish a port to another arch?
2. How portable is ros in general? Meaning how much code would have to
be changed. I hope there are abstractions and only 10-15% of the kernel
would have to be changed.
3. Do you believe that one person new to ros can finish that kind of
work in 6 months? I know that might be hard to answer but how long did
any of you need to learn ros in detail?
4. If there are working ports, how long did it take to implement them?
If there are not, what are the reasons they where never finished? I am
thinking of lots of assembler and i386 hacks.
5. Finally, what do think of the idea itself?
I hope you take the time to answer my questions. I am still on the way
to determine whether i really want to do that and whether i can do this
in 6 months.
regards,
Henning Schild