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

[Nikolay Sivov]

On 12/29/2012 23:04, Marcus Meissner wrote:
> From: Marcus Meissner <marcus@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@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@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