Re: [ros-dev] [ros-diffs] [pschweitzer] 60802: [NTOSKRNL] Implement MiSetControlAreaSymbolsLoaded and MiLoadUserSymbols. These are responsible for marking the control area that we've loaded the symbols and for effectively l...

[Timo Kreuzer]

Am 31.10.2013 00:17, schrieb pschweitzer(a)svn.reactos.org:
>   +VOID
>  +NTAPI
>  +MiSetControlAreaSymbolsLoaded(IN PCONTROL_AREA ControlArea)
>  +{
>  +    KIRQL OldIrql;
>  +
>  +    ASSERT(KeGetCurrentIrql() <= APC_LEVEL);
>  +
>  +    OldIrql = KeAcquireQueuedSpinLock(LockQueuePfnLock);
>  +    ControlArea->u.Flags.DebugSymbolsLoaded |= 1;
>  +
>  +    ASSERT(OldIrql <= APC_LEVEL);
>  +    KeReleaseQueuedSpinLock(LockQueuePfnLock, OldIrql);
>  +    ASSERT(KeGetCurrentIrql() <= APC_LEVEL);
>  +} 
No offence, but seriously? The PFN lock to set a flag?

[Pierre Schweitzer]

Hey,

yes, it can sound weird (especially because in some other places where
we use the ControlArea, we don't lock it), BUT, three reasons here:
-> We change flags (let's ensure there's only one change at a time)
-> Windows does it
-> Microsoft documents it:
http://download.microsoft.com/download/7/E/7/7E7662CF-CBEA-470B-A97E-CE7CE0…

"PFN Lock : The Scope -> All control area, segments, subsections and
prototype PTE usage".

But even themselves criticize such usage (hence its removal in Win7).

Regards,

On 02/11/2013 00:38, Timo Kreuzer wrote:
>   Am 31.10.2013 00:17, schrieb
> pschweitzer(a)svn.reactos.org:
> >   +VOID
> >  +NTAPI
> >  +MiSetControlAreaSymbolsLoaded(IN PCONTROL_AREA ControlArea)
> >  +{
> >  +    KIRQL OldIrql;
> >  +
> >  +    ASSERT(KeGetCurrentIrql() <= APC_LEVEL);
> >  +
> >  +    OldIrql = KeAcquireQueuedSpinLock(LockQueuePfnLock);
> >  +    ControlArea->u.Flags.DebugSymbolsLoaded |= 1;
> >  +
> >  +    ASSERT(OldIrql <= APC_LEVEL);
> >  +    KeReleaseQueuedSpinLock(LockQueuePfnLock, OldIrql);
> >  +    ASSERT(KeGetCurrentIrql() <= APC_LEVEL);
> >  +} 
>  No offence, but seriously? The PFN lock to set a flag?
>
>
>
>  _______________________________________________
>  Ros-dev mailing list
>  Ros-dev(a)reactos.org
>  http://www.reactos.org/mailman/listinfo/ros-dev 

-- 
Pierre Schweitzer<pierre at reactos.org>
System Administrator
ReactOS Foundation

[Timo Kreuzer]

Hi,

While I find using the PFN lock to synchronize access to control areas
ridiculous in general, I wouldn't dare to  expect anyone to design a
better locking architecture.
My point here is basically that for setting a single flag (and derived
from it's name this function has obviously no other purpose than setting
this flag), without any checks or anythig else, using a lock is not how
you do it. And you probably know how much I love the "Windows does it"
argument...

Timo


Am 02.11.2013 10:48, schrieb Pierre Schweitzer:
>   Hey,
>
>  yes, it can sound weird (especially because in some other places where
>  we use the ControlArea, we don't lock it), BUT, three reasons here:
>  -> We change flags (let's ensure there's only one change at a time)
>  -> Windows does it
>  -> Microsoft documents it:
>
> http://download.microsoft.com/download/7/E/7/7E7662CF-CBEA-470B-A97E-CE7CE0…
>
>  "PFN Lock : The Scope -> All control area, segments, subsections and
>  prototype PTE usage".
>
>  But even themselves criticize such usage (hence its removal in Win7).
>
>  Regards,
>
>  On 02/11/2013 00:38, Timo Kreuzer wrote:
> >   Am 31.10.2013 00:17, schrieb
> > pschweitzer(a)svn.reactos.org:
> > >   +VOID
> > >  +NTAPI
> > >  +MiSetControlAreaSymbolsLoaded(IN PCONTROL_AREA ControlArea)
> > >  +{
> > >  +    KIRQL OldIrql;
> > >  +
> > >  +    ASSERT(KeGetCurrentIrql() <= APC_LEVEL);
> > >  +
> > >  +    OldIrql = KeAcquireQueuedSpinLock(LockQueuePfnLock);
> > >  +    ControlArea->u.Flags.DebugSymbolsLoaded |= 1;
> > >  +
> > >  +    ASSERT(OldIrql <= APC_LEVEL);
> > >  +    KeReleaseQueuedSpinLock(LockQueuePfnLock, OldIrql);
> > >  +    ASSERT(KeGetCurrentIrql() <= APC_LEVEL);
> > >  +} 
> >  No offence, but seriously? The PFN lock to set a flag?
> >
> >
> >
> >  _______________________________________________
> >  Ros-dev mailing list
> >  Ros-dev(a)reactos.org
> >  http://www.reactos.org/mailman/listinfo/ros-dev 
>
>
>  _______________________________________________
>  Ros-dev mailing list
>  Ros-dev(a)reactos.org
>  http://www.reactos.org/mailman/listinfo/ros-dev