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

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. 
Seems I was too imprecise (or again a language 
barrier/misunderstanding). Sorry. My intention was to communicate that 
if you pay, you get the option of (trying to) tell me what to work on. 
When I pay, I decide. Obviously anyone is at any time free to comment.

>  The quota code is called by the process functions, and
> maybe even by
> drivers, but it doesn't affect their behavior. 
I'm going out on a limb here (as I haven't checked it), but can't a 
JOB (artificially) limit available resources? That's part of their 
intention, isn't it? (please read on)

>  Two choices:
>
> 1) Implement a rather complex quota implementation, including commit
> charges in Mm (which is several thousand lines of code in NT), 
I knew I got into a rather large area, but this sounds insane for 
something as seemingly "simple".

>   to a fragile kernel 
If it's that fragile it needs to be fixed. If you got a better idea to 
do it than implementing previously missing functionality to expose 
those fragile parts of it, especially as what I started to work on 
apparently exposed those fragile parts, I'm all ears.

>   &
>      Use up a valuable developer's time, probably a couple man-weeks 
Should that be how I chose to spend my time - what's the difference if 
I hadn't been around at all? None, except the end result might make 
Win32 GlobalMemoryStatus and VM_COUNTERS present useful and usable 
data.

>   &
>      Add new bugs to a hot path during process/object creation/
> deletion (I hope you'll agree all new code has bugs) 
Only by convention, not by definition. :-)

>  OR
>
> 2) Continue working on bug fixing the fragile parts of the kernel and
> OS || Implement critical missing functionality for a target driver/
> application & 
This actually started as fixing critical missing functionality (Wax: 
feel free to chime in). Currently ROS does not COMMIT on such 
allocation requests. It happily allows an exe in a 128MB machine with 
32MB pagefile to COMMIT almost 2GB memory.

The error is of course in the way ROS handles COMMIT requests to 
VirtualAlloc, in that it defers committing the memory until something 
touches it... (that is no joke)

The first step to research this was (for me) to see the behaviour of 
GlobalMemoryStatus, and indeed it was broken - it was unimplemented... 
So what do I do? I "dig where I stand". I start an attempt implementing 
the missing functionality.

So the reason I came to touch Ps, and am digging way deeper in Mm than 
I'd really like to, is simply the GlobalMemoryStatus(Ex) Win32 API 
function(s), to even be able to verify, from user-mode, behaviour.

[...]
>  > 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. 
Agreed. But as I didn't know if that's still your baby (SVN access or 
not) and only synched at intervals, or if it has been "adopted", and if 
adding such data types, even if suspected to be correct, from such few 
and vague information source was even acceptable... I played it safe 
and used just the indices. Thanks for sharing the typename.

If concensus is that adding that data type introduces no problems, 
even if from such a vague source, I'm in favor of adding it (as it'd 
solve more than one immediate problem).


> > >   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. 
Are you thinking of the, semantically non-obvious for maintenance 
programmers,

OldVal = Interlocked*Add(&Quota, Addend);
InterlockedCompareExchange(&Peak, OldVal + Addend, OldVal + Addend);

?

I think using a spinlock for that would make it much more obvious. 
Should profiling display it had a measurable impact on execution, sure. 
But we should also keep in mind we are then trading a single spinlock 
for two *locked* (bus-snooping/cache-invalidating) instructions. I 
don't know which is worse (performance wise).


>  >> 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. 
Ah, right... the quota blocks. As already displayed I didn't touch 
them (I honestly don't even know what they are for yet). I only looked 
at the (immediate) process quota entries. The linked lists I never 
touched.


>  I still disagree with trunk-is-a-playground 
I have never disagreed with that, so I'm not sure what you're 
referring to.

>  and I think that pseudo-code should go in branches, but
> you're free  
to disagree.

I suggest you take that discussion with the project coordinator, that 
I cleared that diff with and he explicitly stated a preference for it 
even after I mentioned "branch".

I disagree with you re. comments. I think pseudo-code in comments (in 
trunk even) is to prefer, as it can explain behaviours and intentions 
way better than plain english or even documentation. If those comments 
help maintenance-programmers ten or more years from now, even if only 
one single person, the comments were well spent time and place.

-- 
Mike