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, */"
On 25-Oct-07, at 2:09 PM, tamlin@algonet.se wrote:
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.
I had already written all these functions, but deleted them,
Wasn't that a bonehead thing to do?
No.
because nobody needed them,
... at the time.
Again, this is a matter of engineering. We still have a fragile kernel (Mm, Cm, Cc, mostly). We have scarce resources (very few developers with your abilities).
The quota code is called by the process functions, and maybe even by drivers, but it doesn't affect their behavior.
Two choices:
1) Implement a rather complex quota implementation, including commit charges in Mm (which is several thousand lines of code in NT), to a fragile kernel & Use up a valuable developer's time, probably a couple man-weeks & Add new bugs to a hot path during process/object creation/ deletion (I hope you'll agree all new code has bugs)
OR
2) Continue working on bug fixing the fragile parts of the kernel and OS || Implement critical missing functionality for a target driver/ application & Use up a valuable developer's time on these critical issues which nobody else has the ability to fix & Fix bugs instead of introducing new ones.
I removed the code because I didn't want anyone working on it at the time, including myself, because of reasons #1. It was not doing anyone a service, and anyone that would've attempted to improve my code would've wasted their time as well as endangered the system.
You are free to call sound engineering decisions "boneheaded".
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.
Considering I've once accidentally overwrote a system utility with a competition entry for CS Games 2007 last year, and that a developer, 5 months later, FIXED BUGS IN MY ENTRY! And that only 9 months later, did another developer discover that this didn't really seem to be a system utility anymore, I doubt anyone else but me would notice or even LOOK for bugs in your implementation.
- 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.
It should be added to the NDK.
- 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.
- 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.
- 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, */"
Okay, you got me here, diff output on ros-diffs was hard to read and I didn't see you had your code /* */'ed.
I still disagree with trunk-is-a-playground and I think that pseudo- code should go in branches, but you're free to disagree.
Carry on.
-- Mike _______________________________________________ Ros-dev mailing list Ros-dev@reactos.org http://www.reactos.org/mailman/listinfo/ros-dev
-
Alex Ionescu -
tamlin＠algonet.se