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.
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.
- 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.
- Instead of duplicating code, perhaps consider calling a main
worker function such as PspChargeQuota?
A sound refactoring suggestion.
- 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.
- The quota routines require interlocks
Yep. Even requires a lock for checking and updating QuotaPeak.
- 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.
- 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, */"