25 Oct
2007
25 Oct
'07
8:04 p.m.
On 25-Oct-07, at 2:09 PM, tamlin(a)algonet.se wrote:
No.
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".
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.
It should be added to the NDK.
No it doesn't, you can use Interlocked Compare Exchange.
The global spinlock is required when inserting new quota blocks,
dereferencing them, expanding the quota and giving back the quota.
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.
Alex Ionescu wrote:
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.
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
_______________________________________________
Ros-dev mailing list
Ros-dev(a)reactos.org
http://www.reactos.org/mailman/listinfo/ros-dev