[PATCH] Cygwin: af_unix_spinlock_t: add initializer

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH] Cygwin: af_unix_spinlock_t: add initializer

Ken Brown-6
Also fix a typo.
---
 winsup/cygwin/fhandler.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/winsup/cygwin/fhandler.h b/winsup/cygwin/fhandler.h
index d02b9a913..7e460701c 100644
--- a/winsup/cygwin/fhandler.h
+++ b/winsup/cygwin/fhandler.h
@@ -832,9 +832,10 @@ class fhandler_socket_local: public fhandler_socket_wsock
 /* Sharable spinlock with low CPU profile.  These locks are NOT recursive! */
 class af_unix_spinlock_t
 {
-  LONG  locked;          /* 0 oder 1 */
+  LONG  locked;          /* 0 or 1 */
 
 public:
+  af_unix_spinlock_t () : locked (0) {}
   void lock ()
   {
     LONG ret = InterlockedExchange (&locked, 1);
--
2.17.0

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Cygwin: af_unix_spinlock_t: add initializer

Corinna Vinschen-2
On Jan 10 17:56, Ken Brown wrote:

> Also fix a typo.
> ---
>  winsup/cygwin/fhandler.h | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/winsup/cygwin/fhandler.h b/winsup/cygwin/fhandler.h
> index d02b9a913..7e460701c 100644
> --- a/winsup/cygwin/fhandler.h
> +++ b/winsup/cygwin/fhandler.h
> @@ -832,9 +832,10 @@ class fhandler_socket_local: public fhandler_socket_wsock
>  /* Sharable spinlock with low CPU profile.  These locks are NOT recursive! */
>  class af_unix_spinlock_t
>  {
> -  LONG  locked;          /* 0 oder 1 */
> +  LONG  locked;          /* 0 or 1 */
Huh.

>  public:
> +  af_unix_spinlock_t () : locked (0) {}

Why do we need that?  The spinlock is created as part of a shared mem
region which gets initialized to all zero, no?  Or do you plan to use it
outside of this scenario?


Corinna

--
Corinna Vinschen
Cygwin Maintainer

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Cygwin: af_unix_spinlock_t: add initializer

Ken Brown-6
On 1/10/2019 1:02 PM, Corinna Vinschen wrote:

> On Jan 10 17:56, Ken Brown wrote:
>> Also fix a typo.
>> ---
>>   winsup/cygwin/fhandler.h | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/winsup/cygwin/fhandler.h b/winsup/cygwin/fhandler.h
>> index d02b9a913..7e460701c 100644
>> --- a/winsup/cygwin/fhandler.h
>> +++ b/winsup/cygwin/fhandler.h
>> @@ -832,9 +832,10 @@ class fhandler_socket_local: public fhandler_socket_wsock
>>   /* Sharable spinlock with low CPU profile.  These locks are NOT recursive! */
>>   class af_unix_spinlock_t
>>   {
>> -  LONG  locked;          /* 0 oder 1 */
>> +  LONG  locked;          /* 0 or 1 */
>
> Huh.
>
>>   public:
>> +  af_unix_spinlock_t () : locked (0) {}
>
> Why do we need that?  The spinlock is created as part of a shared mem
> region which gets initialized to all zero, no?  Or do you plan to use it
> outside of this scenario?

At the moment I'm using it in the new FIFO code, and I'm not sure yet whether it
will eventually be in shared memory.  (Until I get things working, I'm
postponing thinking about whether I need shared memory.)

Would it be better to use some other kind of spinlock until I know for sure that
I need shared memory?  My only reason for choosing af_unix_spinlock_t is that I
was copying code from fhandler_socket_unix, and this saved me the trouble of
learning about other kinds of spinlocks.

Ken
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Cygwin: af_unix_spinlock_t: add initializer

Corinna Vinschen-2
On Jan 10 18:36, Ken Brown wrote:

> On 1/10/2019 1:02 PM, Corinna Vinschen wrote:
> > On Jan 10 17:56, Ken Brown wrote:
> >> Also fix a typo.
> >> ---
> >>   winsup/cygwin/fhandler.h | 3 ++-
> >>   1 file changed, 2 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/winsup/cygwin/fhandler.h b/winsup/cygwin/fhandler.h
> >> index d02b9a913..7e460701c 100644
> >> --- a/winsup/cygwin/fhandler.h
> >> +++ b/winsup/cygwin/fhandler.h
> >> @@ -832,9 +832,10 @@ class fhandler_socket_local: public fhandler_socket_wsock
> >>   /* Sharable spinlock with low CPU profile.  These locks are NOT recursive! */
> >>   class af_unix_spinlock_t
> >>   {
> >> -  LONG  locked;          /* 0 oder 1 */
> >> +  LONG  locked;          /* 0 or 1 */
> >
> > Huh.
> >
> >>   public:
> >> +  af_unix_spinlock_t () : locked (0) {}
> >
> > Why do we need that?  The spinlock is created as part of a shared mem
> > region which gets initialized to all zero, no?  Or do you plan to use it
> > outside of this scenario?
>
> At the moment I'm using it in the new FIFO code, and I'm not sure yet whether it
> will eventually be in shared memory.  (Until I get things working, I'm
> postponing thinking about whether I need shared memory.)
>
> Would it be better to use some other kind of spinlock until I know for sure that
> I need shared memory?  My only reason for choosing af_unix_spinlock_t is that I
> was copying code from fhandler_socket_unix, and this saved me the trouble of
> learning about other kinds of spinlocks.
The above patch shouldn't hurt in the least since it's not used anyway
when allocating the shared mem region used by the AF_UNIX socket code.
If it helps you, I can push it, no problem.

Just make sure this spinlock is the right thing for you.  The idea here
was to have a fast, sharable(*) lock without context switching, only
guarding small code blocks which don't hang due to resource starving.

If you have to guard more complex code chunks, it might be better to
use a kernel locking object like mutex or semaphore.


So, push or no push?


Corinna


(*) Originally I thought slim R/W locks are nice for unix sockets...
    until I realised they are not sharable.  Reading MSDN helped :}

--
Corinna Vinschen
Cygwin Maintainer

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Cygwin: af_unix_spinlock_t: add initializer

Ken Brown-6
On 1/10/2019 3:16 PM, Corinna Vinschen wrote:

> On Jan 10 18:36, Ken Brown wrote:
>> On 1/10/2019 1:02 PM, Corinna Vinschen wrote:
>>> On Jan 10 17:56, Ken Brown wrote:
>>>> Also fix a typo.
>>>> ---
>>>>    winsup/cygwin/fhandler.h | 3 ++-
>>>>    1 file changed, 2 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/winsup/cygwin/fhandler.h b/winsup/cygwin/fhandler.h
>>>> index d02b9a913..7e460701c 100644
>>>> --- a/winsup/cygwin/fhandler.h
>>>> +++ b/winsup/cygwin/fhandler.h
>>>> @@ -832,9 +832,10 @@ class fhandler_socket_local: public fhandler_socket_wsock
>>>>    /* Sharable spinlock with low CPU profile.  These locks are NOT recursive! */
>>>>    class af_unix_spinlock_t
>>>>    {
>>>> -  LONG  locked;          /* 0 oder 1 */
>>>> +  LONG  locked;          /* 0 or 1 */
>>>
>>> Huh.
>>>
>>>>    public:
>>>> +  af_unix_spinlock_t () : locked (0) {}
>>>
>>> Why do we need that?  The spinlock is created as part of a shared mem
>>> region which gets initialized to all zero, no?  Or do you plan to use it
>>> outside of this scenario?
>>
>> At the moment I'm using it in the new FIFO code, and I'm not sure yet whether it
>> will eventually be in shared memory.  (Until I get things working, I'm
>> postponing thinking about whether I need shared memory.)
>>
>> Would it be better to use some other kind of spinlock until I know for sure that
>> I need shared memory?  My only reason for choosing af_unix_spinlock_t is that I
>> was copying code from fhandler_socket_unix, and this saved me the trouble of
>> learning about other kinds of spinlocks.
>
> The above patch shouldn't hurt in the least since it's not used anyway
> when allocating the shared mem region used by the AF_UNIX socket code.
> If it helps you, I can push it, no problem.
>
> Just make sure this spinlock is the right thing for you.  The idea here
> was to have a fast, sharable(*) lock without context switching, only
> guarding small code blocks which don't hang due to resource starving.
>
> If you have to guard more complex code chunks, it might be better to
> use a kernel locking object like mutex or semaphore.
>
>
> So, push or no push?

Please push.  Thanks.

Ken
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Cygwin: af_unix_spinlock_t: add initializer

Corinna Vinschen-2
On Jan 10 23:20, Ken Brown wrote:

> On 1/10/2019 3:16 PM, Corinna Vinschen wrote:
> > On Jan 10 18:36, Ken Brown wrote:
> >> On 1/10/2019 1:02 PM, Corinna Vinschen wrote:
> >>> On Jan 10 17:56, Ken Brown wrote:
> >>>> Also fix a typo.
> >>>> ---
> >>>>    winsup/cygwin/fhandler.h | 3 ++-
> >>>>    1 file changed, 2 insertions(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/winsup/cygwin/fhandler.h b/winsup/cygwin/fhandler.h
> >>>> index d02b9a913..7e460701c 100644
> >>>> --- a/winsup/cygwin/fhandler.h
> >>>> +++ b/winsup/cygwin/fhandler.h
> >>>> @@ -832,9 +832,10 @@ class fhandler_socket_local: public fhandler_socket_wsock
> >>>>    /* Sharable spinlock with low CPU profile.  These locks are NOT recursive! */
> >>>>    class af_unix_spinlock_t
> >>>>    {
> >>>> -  LONG  locked;          /* 0 oder 1 */
> >>>> +  LONG  locked;          /* 0 or 1 */
> >>>
> >>> Huh.
> >>>
> >>>>    public:
> >>>> +  af_unix_spinlock_t () : locked (0) {}
> >>>
> >>> Why do we need that?  The spinlock is created as part of a shared mem
> >>> region which gets initialized to all zero, no?  Or do you plan to use it
> >>> outside of this scenario?
> >>
> >> At the moment I'm using it in the new FIFO code, and I'm not sure yet whether it
> >> will eventually be in shared memory.  (Until I get things working, I'm
> >> postponing thinking about whether I need shared memory.)
> >>
> >> Would it be better to use some other kind of spinlock until I know for sure that
> >> I need shared memory?  My only reason for choosing af_unix_spinlock_t is that I
> >> was copying code from fhandler_socket_unix, and this saved me the trouble of
> >> learning about other kinds of spinlocks.
> >
> > The above patch shouldn't hurt in the least since it's not used anyway
> > when allocating the shared mem region used by the AF_UNIX socket code.
> > If it helps you, I can push it, no problem.
> >
> > Just make sure this spinlock is the right thing for you.  The idea here
> > was to have a fast, sharable(*) lock without context switching, only
> > guarding small code blocks which don't hang due to resource starving.
> >
> > If you have to guard more complex code chunks, it might be better to
> > use a kernel locking object like mutex or semaphore.
> >
> >
> > So, push or no push?
>
> Please push.  Thanks.
>
> Ken
Done.


Thanks,
Corinna

--
Corinna Vinschen
Cygwin Maintainer

signature.asc (849 bytes) Download Attachment