Re: [PATCH 2/2] kernel32: conditional variable support

[Nikolay Sivov]

On 12/29/2012 23:04, Marcus Meissner wrote:
> From: Marcus Meissner <marcus(a)jet.(none)>
>
> Modeled pretty much after the implementation described
> in glibc/nptl/DESIGN-condvar.txt with its futex usage
> sofar replaced by regular Win32 Semaphores.
>
> An additional futex implementation is possible.
>
> I *think* I have the semantics correct, there is 1 small
> place where it might have unnecessary CPU usage though.
>
> Ciao, Marcus
> +/**********************************************************************
> + *           InitializeConditionVariable     (KERNEL32.@)
> + */
> +VOID WINAPI InitializeConditionVariable(PCONDITION_VARIABLE variable)
> +{
> +    struct _condition_var_intern *var;
> +    variable->Ptr = HeapAlloc(GetProcessHeap(),HEAP_ZERO_MEMORY,sizeof(struct _condition_var_intern));
> +    var = variable->Ptr;
> +    if (!var) return;
> +    InitializeCriticalSection (&var->crit);
> +    var->crit.DebugInfo->Spare[0] = (DWORD_PTR)(__FILE__ ": ConditionVariable.cs");
> +}
> I don't think it allocates anything at all, what makes me believe that 
> is that stuff like CONDITION_VARIABLE_INIT exists.
> After googling a bit for it I see that gallium from mesa has some 
> platform dependent stuff in code that uses this API like that:

---
+#define pipe_static_condvar(cond) \
  + /*static*/ pipe_condvar cond = CONDITION_VARIABLE_INIT
---

So this structure has nothing more than a single pointer value, value 0 
of it means it's initialized. I think you could just add some tests to 
see if it's really reset to 0 from initial value.

Also regarding tests I don't think randomizing timeouts is a good idea 
cause it adds some unpredictable behaviour and it's not really clear if 
we could rely on such tests results.

[PATCH 2/2] kernel32: conditional variable support

[Marcus Meissner]

On Sun, Dec 30, 2012 at 12:02:34AM +0400, Nikolay Sivov wrote:
> On 12/29/2012 23:04, Marcus Meissner wrote:
> > From: Marcus Meissner <marcus(a)jet.(none)>
> >
> > Modeled pretty much after the implementation described
> > in glibc/nptl/DESIGN-condvar.txt with its futex usage
> > sofar replaced by regular Win32 Semaphores.
> >
> > An additional futex implementation is possible.
> >
> > I *think* I have the semantics correct, there is 1 small
> > place where it might have unnecessary CPU usage though.
> >
> > Ciao, Marcus
> > +/**********************************************************************
> > + *           InitializeConditionVariable     (KERNEL32.@)
> > + */
> > +VOID WINAPI InitializeConditionVariable(PCONDITION_VARIABLE variable)
> > +{
> > +    struct _condition_var_intern *var;
> > +    variable->Ptr = HeapAlloc(GetProcessHeap(),HEAP_ZERO_MEMORY,sizeof(struct _condition_var_intern));
> > +    var = variable->Ptr;
> > +    if (!var) return;
> > +    InitializeCriticalSection (&var->crit);
> > +    var->crit.DebugInfo->Spare[0] = (DWORD_PTR)(__FILE__ ": ConditionVariable.cs");
> > +}
> > I don't think it allocates anything at all, what makes me believe
> > that is that stuff like CONDITION_VARIABLE_INIT exists.
> > After googling a bit for it I see that gallium from mesa has some
> > platform dependent stuff in code that uses this API like that:
>
> ---
> +#define pipe_static_condvar(cond) \
>  + /*static*/ pipe_condvar cond = CONDITION_VARIABLE_INIT
> ---
>
> So this structure has nothing more than a single pointer value,
> value 0 of it means it's initialized. I think you could just add
> some tests to see if it's really reset to 0 from initial value.

I am not fully sure. The SDK only has "PVOID Ptr" and that alone
is not enough space to implement conditional variables.

CONDITION_VARIABLE_INIT is only there to init it to NULL.
(if you look at the SDK CONDITION_VARIABLE_INIT seems to be {0} )

So I suspect some private data structure is hidden into it.

I however do not know if windows allocates it from heap
or from where exactly.
> Also regarding tests I don't think randomizing timeouts is a good
> idea cause it adds some unpredictable behaviour and it's not really
> clear if we could rely on such tests results.

I have to think about those some more :/

I testing Adobe Lightroom 4 with the patch I encountered more conditions
I should probably also test for.

Ciao, Marcus

[PATCH 2/2] kernel32: conditional variable support

[Nikolay Sivov]

On 12/30/2012 00:58, Marcus Meissner wrote:
> On Sun, Dec 30, 2012 at 12:02:34AM +0400, Nikolay Sivov wrote:
> > On 12/29/2012 23:04, Marcus Meissner wrote:
> > > From: Marcus Meissner <marcus(a)jet.(none)>
> > >
> > > Modeled pretty much after the implementation described
> > > in glibc/nptl/DESIGN-condvar.txt with its futex usage
> > > sofar replaced by regular Win32 Semaphores.
> > >
> > > An additional futex implementation is possible.
> > >
> > > I *think* I have the semantics correct, there is 1 small
> > > place where it might have unnecessary CPU usage though.
> > >
> > > Ciao, Marcus
> > > +/**********************************************************************
> > > + *           InitializeConditionVariable     (KERNEL32.@)
> > > + */
> > > +VOID WINAPI InitializeConditionVariable(PCONDITION_VARIABLE variable)
> > > +{
> > > +    struct _condition_var_intern *var;
> > > +    variable->Ptr = HeapAlloc(GetProcessHeap(),HEAP_ZERO_MEMORY,sizeof(struct _condition_var_intern));
> > > +    var = variable->Ptr;
> > > +    if (!var) return;
> > > +    InitializeCriticalSection (&var->crit);
> > > +    var->crit.DebugInfo->Spare[0] = (DWORD_PTR)(__FILE__ ": ConditionVariable.cs");
> > > +}
> > > I don't think it allocates anything at all, what makes me believe
> > > that is that stuff like CONDITION_VARIABLE_INIT exists.
> > > After googling a bit for it I see that gallium from mesa has some
> > > platform dependent stuff in code that uses this API like that:
> >
> > ---
> > +#define pipe_static_condvar(cond) \
> >   + /*static*/ pipe_condvar cond = CONDITION_VARIABLE_INIT
> > ---
> >
> > So this structure has nothing more than a single pointer value,
> > value 0 of it means it's initialized. I think you could just add
> > some tests to see if it's really reset to 0 from initial value.
> > I am not fully sure. The SDK only has "PVOID Ptr" and that alone
> > is not enough space to implement conditional variables.
> > Why it's not enough? API calls use pointer to this struct all the time 
> > so it's known
> > which cond variable is it sleeping on for example.
>
> CONDITION_VARIABLE_INIT is only there to init it to NULL.
> (if you look at the SDK CONDITION_VARIABLE_INIT seems to be {0} )
>
> So I suspect some private data structure is hidden into it.
> I think it's there to initialize it statically.

I believe all of that 'recently' introduced sync APIs (like InitOnce* 
stuff) must be based on similar mechanism
internally that makes them slim and lightweight.
> I however do not know if windows allocates it from heap
> or from where exactly.
> > Also regarding tests I don't think randomizing timeouts is a good
> > idea cause it adds some unpredictable behaviour and it's not really
> > clear if we could rely on such tests results.
> > I have to think about those some more :/
>
> I testing Adobe Lightroom 4 with the patch I encountered more conditions
> I should probably also test for.
>
> Ciao, Marcus