Re: [waffle] [PATCH v2 2/3] wcore: rework non-compatible mutex initialization
On 27 March 2015 at 16:09, Chad Versace wrote:
> Quoting Emil Velikov (2015-03-26 19:24:21)
>> On 26 March 2015 at 14:50, Chad Versace wrote:
>> > Quoting Emil Velikov (2015-03-25 07:30:00)
>> >> Indeed you're bang on the spot. id_counter should be locked throughout
>> >> a series of display_{connect, disconnect}, thus we should push
>> >> mtx_{init,destroy} up to waffle_{init,teardown}. What worries me is
>> >> that none of the tests (ran in valgrind) point out any issues.
>> >
>> > I could be wrong, but I believe pthread_mutex_create/destroy don't
>> > allocate/free memory. They just initialize/reset the fields in the
>> > pthread_mutex_t struct. (Otherwise, how would PTHREAD_MUTEX_INITIALIZER
>> > work?). That may explain why you didn't see the expected Valgrind
>> > errors, because no allocation took place.
>> >
>> I was thinking about thread 2 diving into display_connect() and
>> attempting to lock a destroyed mutex. Afaics in such case the function
>> will return EINVAL, which I naively assumed that is a undefined
>> behaviour. Although with the failed locking around id_counter I assume
>> another problem could have happened.
>
> Not a naive assumption. POSIX does define that undefined behavior occurs
> if you try to lock an uninitialized or destroyed mutex. From
> the pthread_mutex_lock(3p) manpage:
>
>If mutex does not refer to an initialized mutex object, the behavior of
>pthread_mutex_lock(), pthread_mutex_trylock(), and
>pthread_mutex_unlock() is undefined.
Thanks Chad !
I knew I wasn't imagining stuff, yet a quick internet search [1]
pointed out that EINVAL is returned.
Coming back to the patch in question - I'm ok with doing the
call_once/mtx_init dance at display_connect and leaking the mutex.
Will send out updated patch in a bit.
Cheers,
Emil
[1] http://pubs.opengroup.org/onlinepubs/007908799/xsh/pthread_mutex_lock.html
___
waffle mailing list
waffle@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/waffle
Re: [waffle] [PATCH v2 2/3] wcore: rework non-compatible mutex initialization
Quoting Emil Velikov (2015-03-26 19:24:21)
> On 26 March 2015 at 14:50, Chad Versace wrote:
> > Quoting Emil Velikov (2015-03-25 07:30:00)
> >> Indeed you're bang on the spot. id_counter should be locked throughout
> >> a series of display_{connect, disconnect}, thus we should push
> >> mtx_{init,destroy} up to waffle_{init,teardown}. What worries me is
> >> that none of the tests (ran in valgrind) point out any issues.
> >
> > I could be wrong, but I believe pthread_mutex_create/destroy don't
> > allocate/free memory. They just initialize/reset the fields in the
> > pthread_mutex_t struct. (Otherwise, how would PTHREAD_MUTEX_INITIALIZER
> > work?). That may explain why you didn't see the expected Valgrind
> > errors, because no allocation took place.
> >
> I was thinking about thread 2 diving into display_connect() and
> attempting to lock a destroyed mutex. Afaics in such case the function
> will return EINVAL, which I naively assumed that is a undefined
> behaviour. Although with the failed locking around id_counter I assume
> another problem could have happened.
Not a naive assumption. POSIX does define that undefined behavior occurs
if you try to lock an uninitialized or destroyed mutex. From
the pthread_mutex_lock(3p) manpage:
If mutex does not refer to an initialized mutex object, the behavior of
pthread_mutex_lock(), pthread_mutex_trylock(), and
pthread_mutex_unlock() is undefined.
___
waffle mailing list
waffle@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/waffle
Re: [waffle] [PATCH v2 2/3] wcore: rework non-compatible mutex initialization
On 26 March 2015 at 14:50, Chad Versace wrote:
> Quoting Emil Velikov (2015-03-25 07:30:00)
>> On 24 March 2015 at 17:37, Jose Fonseca wrote:
>> > Is wcore_display_teardown called only once, or when destroying each
>> > display?
>> >
>> > If the latter, then it's not safe to call `mtx_destroy(&mutex)` on
>> > `wcore_display_teardown`. Otherwise when wcore_display_init is called
>> > next, wcore_display_init_once() will not be called a 2nd time, hence mutex
>> > will stay invalid.
>> >
>> >
>> > This should probably done at waffle_teardown.
>
> Right. The mutex should be destroyed, if anywhere, in waffle_teardown().
> But, we should probably not destroy it at all, because of my next
> suggestion...
>
>> > BTW, if the mutex was initialized at waffle_init, then once_flag wouldn't
>> > even be necessary.
>
> Technically correct, but I eventually want to deprecate waffle_init(),
> moving its platform parameter to a new waffle_display_connect()
> function. Using a once_flag in wcore_display_init() fits better with
> that longterm goal. Let's keep Emil's once_flag in this patch.
>
> If use a once_flag, then I believe there is no safe place to
> destroy the mutex, because we can't re-initialize the mutex by calling
> wcore_display_init_once() a second time.
>
> Which leads to the question: Emil, what benefit do you expect from
> destroying the mutex? If Waffle were continuously creating mutexes, then
> Waffle would need to destroy them too to prevent leaks. But Waffle only
> creates a small, fixed number of global mutexes during the process's
> lifetime. And "leaking" them doesn't lead to ongoing memory loss.
I don't have particular reason for wanting to destroy the mutex, other
than being a "good citizen". On the topic of leaking memory, I don't
think that it will be a major concern even if the c11
implementation(s) (unlike the pthreads one) allocates memory.
> Moreover, with pthreads... see my comments below.
>
>> Indeed you're bang on the spot. id_counter should be locked throughout
>> a series of display_{connect, disconnect}, thus we should push
>> mtx_{init,destroy} up to waffle_{init,teardown}. What worries me is
>> that none of the tests (ran in valgrind) point out any issues.
>
> I could be wrong, but I believe pthread_mutex_create/destroy don't
> allocate/free memory. They just initialize/reset the fields in the
> pthread_mutex_t struct. (Otherwise, how would PTHREAD_MUTEX_INITIALIZER
> work?). That may explain why you didn't see the expected Valgrind
> errors, because no allocation took place.
>
I was thinking about thread 2 diving into display_connect() and
attempting to lock a destroyed mutex. Afaics in such case the function
will return EINVAL, which I naively assumed that is a undefined
behaviour. Although with the failed locking around id_counter I assume
another problem could have happened.
Cheers
Emil
___
waffle mailing list
waffle@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/waffle
Re: [waffle] [PATCH v2 2/3] wcore: rework non-compatible mutex initialization
Quoting Jose Fonseca (2015-03-26 09:01:41)
> The approach we use to statically initialize critical sections on
> windows is unofficial. Read
> http://locklessinc.com/articles/pthreads_on_windows/ for the gritty details.
From the page:
#define PTHREAD_MUTEX_INITIALIZER {(void*)-1,-1,0,0,0,0}
(void*)-1 ??? I've never seen that before. That *is* a gritty detail.
___
waffle mailing list
waffle@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/waffle
Re: [waffle] [PATCH v2 2/3] wcore: rework non-compatible mutex initialization
On 26/03/15 14:50, Chad Versace wrote:
Quoting Emil Velikov (2015-03-25 07:30:00)
On 24 March 2015 at 17:37, Jose Fonseca wrote:
Is wcore_display_teardown called only once, or when destroying each display?
If the latter, then it's not safe to call `mtx_destroy(&mutex)` on
`wcore_display_teardown`. Otherwise when wcore_display_init is called
next, wcore_display_init_once() will not be called a 2nd time, hence mutex
will stay invalid.
This should probably done at waffle_teardown.
Right. The mutex should be destroyed, if anywhere, in waffle_teardown().
But, we should probably not destroy it at all, because of my next
suggestion...
BTW, if the mutex was initialized at waffle_init, then once_flag wouldn't
even be necessary.
Technically correct, but I eventually want to deprecate waffle_init(),
moving its platform parameter to a new waffle_display_connect()
function. Using a once_flag in wcore_display_init() fits better with
that longterm goal. Let's keep Emil's once_flag in this patch.
If use a once_flag, then I believe there is no safe place to
destroy the mutex, because we can't re-initialize the mutex by calling
wcore_display_init_once() a second time.
Which leads to the question: Emil, what benefit do you expect from
destroying the mutex? If Waffle were continuously creating mutexes, then
Waffle would need to destroy them too to prevent leaks. But Waffle only
creates a small, fixed number of global mutexes during the process's
lifetime. And "leaking" them doesn't lead to ongoing memory loss.
Moreover, with pthreads... see my comments below.
Indeed you're bang on the spot. id_counter should be locked throughout
a series of display_{connect, disconnect}, thus we should push
mtx_{init,destroy} up to waffle_{init,teardown}. What worries me is
that none of the tests (ran in valgrind) point out any issues.
I could be wrong, but I believe pthread_mutex_create/destroy don't
allocate/free memory. They just initialize/reset the fields in the
pthread_mutex_t struct. (Otherwise, how would PTHREAD_MUTEX_INITIALIZER
work?). That may explain why you didn't see the expected Valgrind
errors, because no allocation took place.
I think it's fine to let it leak on destroy, but just FYI on Windows
mutex (ie, CriticalSections) may allocate/free memory on certain
configurations. IIRC, they can allocate memory for stack backtraces,
for debugging purposes.
The approach we use to statically initialize critical sections on
windows is unofficial. Read
http://locklessinc.com/articles/pthreads_on_windows/ for the gritty details.
Jose
___
waffle mailing list
waffle@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/waffle
Re: [waffle] [PATCH v2 2/3] wcore: rework non-compatible mutex initialization
Quoting Emil Velikov (2015-03-25 07:30:00)
> On 24 March 2015 at 17:37, Jose Fonseca wrote:
> > Is wcore_display_teardown called only once, or when destroying each display?
> >
> > If the latter, then it's not safe to call `mtx_destroy(&mutex)` on
> > `wcore_display_teardown`. Otherwise when wcore_display_init is called
> > next, wcore_display_init_once() will not be called a 2nd time, hence mutex
> > will stay invalid.
> >
> >
> > This should probably done at waffle_teardown.
Right. The mutex should be destroyed, if anywhere, in waffle_teardown().
But, we should probably not destroy it at all, because of my next
suggestion...
> > BTW, if the mutex was initialized at waffle_init, then once_flag wouldn't
> > even be necessary.
Technically correct, but I eventually want to deprecate waffle_init(),
moving its platform parameter to a new waffle_display_connect()
function. Using a once_flag in wcore_display_init() fits better with
that longterm goal. Let's keep Emil's once_flag in this patch.
If use a once_flag, then I believe there is no safe place to
destroy the mutex, because we can't re-initialize the mutex by calling
wcore_display_init_once() a second time.
Which leads to the question: Emil, what benefit do you expect from
destroying the mutex? If Waffle were continuously creating mutexes, then
Waffle would need to destroy them too to prevent leaks. But Waffle only
creates a small, fixed number of global mutexes during the process's
lifetime. And "leaking" them doesn't lead to ongoing memory loss.
Moreover, with pthreads... see my comments below.
> Indeed you're bang on the spot. id_counter should be locked throughout
> a series of display_{connect, disconnect}, thus we should push
> mtx_{init,destroy} up to waffle_{init,teardown}. What worries me is
> that none of the tests (ran in valgrind) point out any issues.
I could be wrong, but I believe pthread_mutex_create/destroy don't
allocate/free memory. They just initialize/reset the fields in the
pthread_mutex_t struct. (Otherwise, how would PTHREAD_MUTEX_INITIALIZER
work?). That may explain why you didn't see the expected Valgrind
errors, because no allocation took place.
> Might be worth looking into, once I've got waffle_teardown() hooked in there.
>
> -Emil
> ___
> waffle mailing list
> waffle@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/waffle
___
waffle mailing list
waffle@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/waffle
Re: [waffle] [PATCH v2 2/3] wcore: rework non-compatible mutex initialization
On 24 March 2015 at 17:37, Jose Fonseca wrote:
> Is wcore_display_teardown called only once, or when destroying each display?
>
> If the latter, then it's not safe to call `mtx_destroy(&mutex)` on
> `wcore_display_teardown`. Otherwise when wcore_display_init is called
> next, wcore_display_init_once() will not be called a 2nd time, hence mutex
> will stay invalid.
>
>
> This should probably done at waffle_teardown.
>
> BTW, if the mutex was initialized at waffle_init, then once_flag wouldn't
> even be necessary.
>
Indeed you're bang on the spot. id_counter should be locked throughout
a series of display_{connect, disconnect}, thus we should push
mtx_{init,destroy} up to waffle_{init,teardown}. What worries me is
that none of the tests (ran in valgrind) point out any issues.
Might be worth looking into, once I've got waffle_teardown() hooked in there.
-Emil
___
waffle mailing list
waffle@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/waffle
Re: [waffle] [PATCH v2 2/3] wcore: rework non-compatible mutex initialization
Is wcore_display_teardown called only once, or when destroying each display?
If the latter, then it's not safe to call `mtx_destroy(&mutex)` on
`wcore_display_teardown`. Otherwise when wcore_display_init is called
next, wcore_display_init_once() will not be called a 2nd time, hence
mutex will stay invalid.
This should probably done at waffle_teardown.
BTW, if the mutex was initialized at waffle_init, then once_flag
wouldn't even be necessary.
Jose
On 24/03/15 16:56, Emil Velikov wrote:
C11 does not specify a static initializer, based on the idea that the
a mutex will be platform and/or implementation dependent. As such the
alternative solution is to initialize the mutex with call_once/mtx_init.
This will allow us to remove the transition hack, and in due time move
to the system's C11 threads implementation.
v2: Actually use call_once() to prevent the possibility of multiple
threads hitting the mtx_init() at the same time. Suggested by Jose.
Cc: Jose Fonseca
Signed-off-by: Emil Velikov
---
src/waffle/core/wcore_display.c | 19 ++-
src/waffle/core/wcore_display.h | 9 ++---
2 files changed, 20 insertions(+), 8 deletions(-)
diff --git a/src/waffle/core/wcore_display.c b/src/waffle/core/wcore_display.c
index 2feeeba..2cf0dcd 100644
--- a/src/waffle/core/wcore_display.c
+++ b/src/waffle/core/wcore_display.c
@@ -29,16 +29,25 @@
#include "wcore_display.h"
+static mtx_t mutex;
+
+static void
+wcore_display_init_once(void)
+{
+mtx_init(&mutex, mtx_plain);
+}
+
bool
wcore_display_init(struct wcore_display *self,
struct wcore_platform *platform)
{
static size_t id_counter = 0;
-static mtx_t mutex = _MTX_INITIALIZER_NP;
+static once_flag flag = ONCE_FLAG_INIT;
assert(self);
assert(platform);
+call_once(&flag, wcore_display_init_once);
mtx_lock(&mutex);
self->api.display_id = ++id_counter;
mtx_unlock(&mutex);
@@ -52,3 +61,11 @@ wcore_display_init(struct wcore_display *self,
return true;
}
+
+bool
+wcore_display_teardown(struct wcore_display *self)
+{
+assert(self);
+mtx_destroy(&mutex);
+return true;
+}
diff --git a/src/waffle/core/wcore_display.h b/src/waffle/core/wcore_display.h
index de6ca5e..6d95d98 100644
--- a/src/waffle/core/wcore_display.h
+++ b/src/waffle/core/wcore_display.h
@@ -56,10 +56,5 @@ wcore_display_init(struct wcore_display *self,
struct wcore_platform *platform);
-static inline bool
-wcore_display_teardown(struct wcore_display *self)
-{
-(void) self;
-assert(self);
-return true;
-}
+bool
+wcore_display_teardown(struct wcore_display *self);
___
waffle mailing list
waffle@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/waffle
[waffle] [PATCH v2 2/3] wcore: rework non-compatible mutex initialization
C11 does not specify a static initializer, based on the idea that the
a mutex will be platform and/or implementation dependent. As such the
alternative solution is to initialize the mutex with call_once/mtx_init.
This will allow us to remove the transition hack, and in due time move
to the system's C11 threads implementation.
v2: Actually use call_once() to prevent the possibility of multiple
threads hitting the mtx_init() at the same time. Suggested by Jose.
Cc: Jose Fonseca
Signed-off-by: Emil Velikov
---
src/waffle/core/wcore_display.c | 19 ++-
src/waffle/core/wcore_display.h | 9 ++---
2 files changed, 20 insertions(+), 8 deletions(-)
diff --git a/src/waffle/core/wcore_display.c b/src/waffle/core/wcore_display.c
index 2feeeba..2cf0dcd 100644
--- a/src/waffle/core/wcore_display.c
+++ b/src/waffle/core/wcore_display.c
@@ -29,16 +29,25 @@
#include "wcore_display.h"
+static mtx_t mutex;
+
+static void
+wcore_display_init_once(void)
+{
+mtx_init(&mutex, mtx_plain);
+}
+
bool
wcore_display_init(struct wcore_display *self,
struct wcore_platform *platform)
{
static size_t id_counter = 0;
-static mtx_t mutex = _MTX_INITIALIZER_NP;
+static once_flag flag = ONCE_FLAG_INIT;
assert(self);
assert(platform);
+call_once(&flag, wcore_display_init_once);
mtx_lock(&mutex);
self->api.display_id = ++id_counter;
mtx_unlock(&mutex);
@@ -52,3 +61,11 @@ wcore_display_init(struct wcore_display *self,
return true;
}
+
+bool
+wcore_display_teardown(struct wcore_display *self)
+{
+assert(self);
+mtx_destroy(&mutex);
+return true;
+}
diff --git a/src/waffle/core/wcore_display.h b/src/waffle/core/wcore_display.h
index de6ca5e..6d95d98 100644
--- a/src/waffle/core/wcore_display.h
+++ b/src/waffle/core/wcore_display.h
@@ -56,10 +56,5 @@ wcore_display_init(struct wcore_display *self,
struct wcore_platform *platform);
-static inline bool
-wcore_display_teardown(struct wcore_display *self)
-{
-(void) self;
-assert(self);
-return true;
-}
+bool
+wcore_display_teardown(struct wcore_display *self);
--
2.3.1
___
waffle mailing list
waffle@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/waffle
