[ros-dev] Re: [ros-diffs] 29826 ...

[tamlin at algonet.se]

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