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

[Alex Ionescu]

On 25-Oct-07, at 2:09 PM, tamlin at algonet.se wrote:

> 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.

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.

>
>
>> 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.

It should be added to the NDK.

>
>
>> 7) 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.

>
>
>> 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.

The global spinlock is required when inserting new quota blocks,  
dereferencing them, expanding the quota and giving back the quota.

>
>
>> 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, */"

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 at reactos.org
> http://www.reactos.org/mailman/listinfo/ros-dev