25 Oct
2007
25 Oct
'07
6:09 p.m.
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