RE: [ion] 20554: - Fix shamefully dangerously broken WorkThread/Queue/Item implementation:

[Ge van Geldorp]

> From: [e-mail]
> Do not pollute the kernel with 10 real-time threads and 5
>
> high-priority threads in order to manage work items. Work 
> threads are very-low priority (< 7) and should never pre-empt 
> userthreads like they do now. 1 priority 7, 5 priority 5 and 
> 3 priority 4 threads are now properly created.

I haven't looked at your code yet, but the comment worries me. Does it mean
that if a usermode app is stuck in a "while (1) ;" loop stuff queued by
IoAllocateWorkItem( ) never gets executed? If I misunderstood, nevermind,
disregard this message. If this is true we're in big trouble. We're using
work items in a lot of places to get from DPC level to PASSIVE level. For
example, the networking stack queues a work item when data was received from
the network card. I'd hate to see a stuck usermode app halt all network
communications...

GvG

[Alex Ionescu]

Ge van Geldorp wrote:
> I haven't looked at your code yet, but the comment worries me. Does it mean
> that if a usermode app is stuck in a "while (1) ;" loop stuff queued by
> IoAllocateWorkItem( ) never gets executed? If I misunderstood, nevermind,
> disregard this message. If this is true we're in big trouble. We're using
> work items in a lot of places to get from DPC level to PASSIVE level. For
> example, the networking stack queues a work item when data was received from
> the network card. I'd hate to see a stuck usermode app halt all network
> communications...
>
> GvG

Hi,

I've simply made work threads have the same priorities as in NT, so now 
they act the same (that is, lower priority then normal threads).
I'm not quite sure how the scheduler in ReactOS handles your scenario 
however; while it is true that when an app is stuck and its thread's 
quantum runs out, the NT scheduler will prefer to choose a thread with a 
higher priority; however, after a while, the threads with lower priority 
will have been in a wait state for a long time, the kernel will realize 
this, boost their priorities and have them run again.

Best regards,
Alex Ionescu

[Ge van Geldorp]

> From: Alex Ionescu
>
> I've simply made work threads have the same priorities as in 
> NT, so now they act the same (that is, lower priority then 
> normal threads).
> I'm not quite sure how the scheduler in ReactOS handles your 
> scenario however; while it is true that when an app is stuck 
> and its thread's quantum runs out, the NT scheduler will 
> prefer to choose a thread with a higher priority; however, 
> after a while, the threads with lower priority will have been 
> in a wait state for a long time, the kernel will realize 
> this, boost their priorities and have them run again.

AFAIK (but this is certainly not my area of expertise) we don't boost
priorities. But even if we did, it's not good enough since it takes too long
for the work items to be run. Having a single network packet come in every
tenth of a second or so doesn't sound too appealing.

Your change doesn't seem consistent with MSDN, my CD copy of Oct-2005 (can't
find it in the online version) documents a WORK_QUEUE_TYPE CriticalWorkQueue
as "Insert the WorkItem into the queue from which a system thread with a
real-time priority attribute will process the work item." Of course, MSDN
has been known to be wrong before.

Anyway, you still have a couple of weeks before the next release to fix the
regression you introduced (affecting at least networking, Tab+K behaviour, I
think mouse message delivery), although fixing the regression (bug 1213)
caused by your previous commits might take some of your time too.

GvG

[Alex Ionescu]

Ge van Geldorp wrote:
> > From: Alex Ionescu
> >
> > I've simply made work threads have the same priorities as in 
> > NT, so now they act the same (that is, lower priority then 
> > normal threads).
> > I'm not quite sure how the scheduler in ReactOS handles your 
> > scenario however; while it is true that when an app is stuck 
> > and its thread's quantum runs out, the NT scheduler will 
> > prefer to choose a thread with a higher priority; however, 
> > after a while, the threads with lower priority will have been 
> > in a wait state for a long time, the kernel will realize 
> > this, boost their priorities and have them run again.
>
> AFAIK (but this is certainly not my area of expertise) we don't boost
> priorities. But even if we did, it's not good enough since it takes too long
> for the work items to be run. Having a single network packet come in every
> tenth of a second or so doesn't sound too appealing.
>
> Your change doesn't seem consistent with MSDN, my CD copy of Oct-2005 (can't
> find it in the online version) documents a WORK_QUEUE_TYPE CriticalWorkQueue
> as "Insert the WorkItem into the queue from which a system thread with a
> real-time priority attribute will process the work item." Of course, MSDN
> has been known to be wrong before.

I will re-verify this...
> Anyway, you still have a couple of weeks before the next release to fix the
> regression you introduced (affecting at least networking, Tab+K behaviour, I
> think mouse message delivery)

Huh? Where are you getting this information from? I'm not denying it but 
I have received no warning that I've caused such a regression, unless 
this is it? :)
> , although fixing the regression (bug 1213)
> caused by your previous commits might take some of your time too.

Sorry, again, what does 1213 have to do with me? I received no 
information relating to this bug... I will look at the bug report now..

Next time I apparently cause regressions, please add me to the CC list 
of the bug so I can know what regressions I may or may not have caused.
> GvG

Best regards,
Alex Ionescu

[Ge van Geldorp]

> From: Alex Ionescu
>
> Ge van Geldorp wrote:
> > (affecting at least networking, Tab+K behaviour, I think mouse
> > message delivery)
>
> Huh? Where are you getting this information from? I'm not 
> denying it but I have received no warning that I've caused 
> such a regression, unless this is it? :)

I was just listing the places from the top of my head where I knew work
items were being queued that should be handled in a "timely manner". I saw
your commit 20561 which solves the issue again, thanks.
> Sorry, again, what does 1213 have to do with me? I received 
> no information relating to this bug... I will look at the bug 
> report now..

I was assuming that every developer is subscribed to ros-bugs, apparently an
incorrect assumption. On the other hand, perhaps ros-bugs is redundant and
Bugzilla reports should go to ros-dev instead?

GvG

[Steven Edwards]

Hi,

On 1/4/06, Ge van Geldorp [e-mail] wrote:
> I was assuming that every developer is subscribed to ros-bugs, apparently an
> incorrect assumption. On the other hand, perhaps ros-bugs is redundant and
> Bugzilla reports should go to ros-dev instead?

I think ros-bugs is kind of redundant. Its not like we get that many
reports yet. I think we should freeze the list and just forward it to
ros-dev until we start getting more than say 10 bug reports a day.

--
Steven Edwards - ReactOS and Wine developer

"There is one thing stronger than all the armies in the world, and
that is an idea whose time has come." - Victor Hugo

[Ged Murphy]

Steven Edwards wrote:
> Hi,
>
> On 1/4/06, Ge van Geldorp [e-mail] wrote:
> > I was assuming that every developer is subscribed to ros-bugs, apparently an
> > incorrect assumption. On the other hand, perhaps ros-bugs is redundant and
> > Bugzilla reports should go to ros-dev instead?
>
> I think ros-bugs is kind of redundant. Its not like we get that many
> reports yet. I think we should freeze the list and just forward it to
> ros-dev until we start getting more than say 10 bug reports a day.

Ros-Bugs is one of our most active lists. It's very useful and I would 
really like to see this stay.
IMO, Ros-SVN is kinda redundant as the info it displays is available in 
Ros-Diffs

[TwoTailedFox]

Ros-svn allows me to filter all search results so only commits appear.
Very handy to see the logs at-a-glance.

On 1/4/06, Ged Murphy [e-mail] wrote:
> Steven Edwards wrote:
> > Hi,
> >
> > On 1/4/06, Ge van Geldorp [e-mail] wrote:
> > > I was assuming that every developer is subscribed to ros-bugs, apparently an
> > > incorrect assumption. On the other hand, perhaps ros-bugs is redundant and
> > > Bugzilla reports should go to ros-dev instead?
> >
> > I think ros-bugs is kind of redundant. Its not like we get that many
> > reports yet. I think we should freeze the list and just forward it to
> > ros-dev until we start getting more than say 10 bug reports a day.
>
> Ros-Bugs is one of our most active lists. It's very useful and I would
> really like to see this stay.
> IMO, Ros-SVN is kinda redundant as the info it displays is available in
> Ros-Diffs
>
> Ros-dev mailing list
> [e-mail]
> http://www.reactos.org/mailman/listinfo/ros-dev

--
"I had a handle on life, but then it broke"

[Hartmut Birr]

Ged Murphy wrote:
> Ros-Bugs is one of our most active lists.

I didn't know that we have this list. It is not part of
http://www.reactos.org/xhtml/en/community_mailinglists.html

- Hartmut

[Ge van Geldorp]

> From: Hartmut Birr
>
> I didn't know that we have this list. It is not part of 
> http://www.reactos.org/xhtml/en/community_mailinglists.html

Sorry, it has been added now (together with ros-cis).

GvG

[Alex Ionescu]

Ge van Geldorp wrote:
> I was assuming that every developer is subscribed to ros-bugs, apparently an
> incorrect assumption. On the other hand, perhaps ros-bugs is redundant and
> Bugzilla reports should go to ros-dev instead?
>
> GvG

Hi,

Hmm, ros-bugs only notifies me when I'm on the CC list or am the Owner 
of the bug... maybe I need to subscribe to the full feed.

Incidentally Ge, I really don't have a VMWare test/debug platform setup 
and it would take me a day or two to get it setup and the vmware video 
driver bug really bothers me, so if you have any spare time this week, 
could you perhaps find me to isolate the issue? Perhaps it's another 
alignment problem -- my patch changed no actual code, only headers and 
prototypes.

Best regards,
Alex Ionescu

[Ge van Geldorp]

> From: Alex Ionescu
>
> Incidentally Ge, I really don't have a VMWare test/debug 
> platform setup and it would take me a day or two to get it 
> setup and the vmware video driver bug really bothers me, so 
> if you have any spare time this week, could you perhaps find 
> me to isolate the issue? Perhaps it's another alignment 
> problem -- my patch changed no actual code, only headers and 
> prototypes.

I'll look into it.

GvG

[WaxDragon]

On 1/4/06, Ge van Geldorp [e-mail] wrote:
> Anyway, you still have a couple of weeks before the next release to fix the
> regression you introduced (affecting at least networking, Tab+K behaviour, I
> think mouse message delivery), although fixing the regression (bug 1213)
> caused by your previous commits might take some of your time too.

Are there bugs filed on the other regressions?

WD
--
<Alex_Ionescu> it's like saying let's rename Ke to Kernel because
people think it's Ketchup

[Ge van Geldorp]

> From: WaxDragon
>
> Are there bugs filed on the other regressions?

No, I was waiting for Alex to confirm my understanding of his patch. Since
he changed it already there's no point anymore.

GvG

[Alex Buell]

On Wed, 4 Jan 2006 22:22:39 +0100 "Ge van Geldorp" [e-mail]
waved a wand and this message magically appeared:
> > From: WaxDragon
> >
> > Are there bugs filed on the other regressions?
>
> No, I was waiting for Alex to confirm my understanding of his patch. Since
> he changed it already there's no point anymore.

Personally ReactOS will crash if I move the mouse after booting. This
needs fixing asap.

Also, I bit the bullet and upgraded my binutils, now cmpxhg8b will
compile in fastinterlck_asm.S. But it's still wrong and doesn't feel
right somehow.
-- 
http://www.munted.org.uk

Anyone that thinks an imaginary deity is going to protect them against 
earthquakes and hurricanes needs psychiatric help.

[Alex Ionescu]

Alex Buell wrote:
> Personally ReactOS will crash if I move the mouse after booting. This
> needs fixing asap.
>
> Also, I bit the bullet and upgraded my binutils, now cmpxhg8b will
> compile in fastinterlck_asm.S. But it's still wrong and doesn't feel
> right somehow.

The previous interlck_asm.S contained:

.global @ExfInterlockedCompareExchange64@12
@ExfInterlockedCompareExchange64@12:
    <...>
    movl 4(%edx),%edx
    LOCK
    cmpxchg8b (%esi)
    <...>

Which is the same as what is in it now... how did your binutils manage to compile it? If it did, then it's simply a bug
in binutils and at one point we have to draw the line at how many buggy releases of compilers and linkers we want to
support.

Best regards,
Alex Ionescu

[Alex Buell]

On Wed, 04 Jan 2006 23:38:04 -0500 Alex Ionescu [e-mail]
waved a wand and this message magically appeared:
> Alex Buell wrote:
> > Personally ReactOS will crash if I move the mouse after booting. This
> > needs fixing asap.
> >
> > Also, I bit the bullet and upgraded my binutils, now cmpxhg8b will
> > compile in fastinterlck_asm.S. But it's still wrong and doesn't feel
> > right somehow.
>
> The previous interlck_asm.S contained:
>
> .global @ExfInterlockedCompareExchange64@12
> @ExfInterlockedCompareExchange64@12:
>    <...>
>    movl 4(%edx),%edx
>    LOCK
>    cmpxchg8b (%esi)
>    <...>
>
> Which is the same as what is in it now... how did your binutils manage to compile it? If it did, then it's simply a bug
> in binutils and at one point we have to draw the line at how many buggy releases of compilers and linkers we want to
> support.

Not that one, the other one; fastinterlck_asm.S.
-- 
http://www.munted.org.uk

Anyone that thinks an imaginary deity is going to protect them against 
earthquakes and hurricanes needs psychiatric help.