Alex Ionescu wrote:
- 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:
- 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
- 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).
- 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).
- 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.