search for: pthread_mutex_lock

On Wed, Feb 25, 2015 at 10:35 AM, Ilia Mirkin <imirkin at alum.mit.edu> wrote: > pthread_mutex_lock had *better* imply a compiler barrier across which > code can't be moved... which is very different from the printf case > where it might have done it due to register pressure or who knows > what. > In the dummy function, register pressure was certainly not an issue, but point taken...

I do have a question about whether patch 2 is right, or whether I've exposed a bigger problem in the truncate (and possibly other) filter, but the rest seem fairly straightforward. Eric Blake (4): server: Check for pthread lock failures truncate: Factor out reading real_size under mutex plugins: Check for mutex failures filters: Check for mutex failures filters/cache/cache.c

...t a/filters/cache/cache.c b/filters/cache/cache.c index 6a9966e..b3fef42 100644 --- a/filters/cache/cache.c +++ b/filters/cache/cache.c @@ -209,9 +209,8 @@ cache_get_size (struct nbdkit_next_ops *next_ops, void *nxdata, nbdkit_debug ("cache: underlying file size: %" PRIi64, size); - pthread_mutex_lock (&lock); + ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&lock); r = blk_set_size (size); - pthread_mutex_unlock (&lock); if (r == -1) return -1; @@ -266,9 +265,10 @@ cache_pread (struct nbdkit_next_ops *next_ops, void *nxdata, if (n > count) n = count; - pthread_mute...

...s changed, 38 insertions(+), 24 deletions(-) diff --git a/server/connections.c b/server/connections.c index a30a541..b7d9a6a 100644 --- a/server/connections.c +++ b/server/connections.c @@ -91,11 +91,13 @@ connection_get_status (struct connection *conn) { int r; - if (conn->nworkers) - pthread_mutex_lock (&conn->status_lock); + if (conn->nworkers && + pthread_mutex_lock (&conn->status_lock)) + abort (); r = conn->status; - if (conn->nworkers) - pthread_mutex_unlock (&conn->status_lock); + if (conn->nworkers && + pthread_mutex_un...

...drmClose(nvdev->base.fd); free(nvdev->client); + pthread_mutex_destroy(&nvdev->lock); free(nvdev); *pdev = NULL; } @@ -191,6 +198,8 @@ nouveau_client_new(struct nouveau_device *dev, struct nouveau_client **pclient) int id = 0, i, ret = -ENOMEM; uint32_t *clients; + pthread_mutex_lock(&nvdev->lock); + for (i = 0; i < nvdev->nr_client; i++) { id = ffs(nvdev->client[i]) - 1; if (id >= 0) @@ -199,7 +208,7 @@ nouveau_client_new(struct nouveau_device *dev, struct nouveau_client **pclient) clients = realloc(nvdev->client, sizeof(uint32_t) * (i + 1));...

...ogram has a segmentation fault at the same library call? The library call is from libpthread.so.0 and the call itself is "pthread_mutex_locl ( )". I have enclosed the core dump information below. The program comes up and then does the segmentation fault. (gdb) bt #0 0x40035944 in pthread_mutex_lock () from /lib/tls/libpthread.so.0 #1 0x419ede1e in register_verify (p=3D0x8163890, sin=3D0x41bfc0d0, req=3D0x41bfc0e0, uri=3D0x41bfc2fd "sip:10.0.0.5", ignore=3D0) at chan_sip.c:5032 #2 0x419f4495 in handle_request (p=3D0x8163890, req=3D0x41bfc0e0, sin=3D0x41bfc0d0, recount=3D0x0, nou...

...ouveau/nouveau.c > >> +++ b/nouveau/nouveau.c > >> @@ -349,8 +349,8 @@ nouveau_bo_del(struct nouveau_bo *bo) > >> struct nouveau_bo_priv *nvbo = nouveau_bo(bo); > >> struct drm_gem_close req = { bo->handle }; > >> > >> - pthread_mutex_lock(&nvdev->lock); > >> - if (nvbo->name) { > >> + if (nvbo->head.next) { > >> + pthread_mutex_lock(&nvdev->lock); > >> if (atomic_read(&nvbo->refcnt) == 0) { > >> D...

...veau/nouveau.c > >>>> @@ -349,8 +349,8 @@ nouveau_bo_del(struct nouveau_bo *bo) > >>>> struct nouveau_bo_priv *nvbo = nouveau_bo(bo); > >>>> struct drm_gem_close req = { bo->handle }; > >>>> > >>>> - pthread_mutex_lock(&nvdev->lock); > >>>> - if (nvbo->name) { > >>>> + if (nvbo->head.next) { > >>>> + pthread_mutex_lock(&nvdev->lock); > >>>> if (atomic_read(&nvbo->refcnt) == 0) { > &g...

...46,6 +46,7 @@ #include <nbdkit-plugin.h> +#include "cleanup.h" #include "sparse.h" /* If raw|base64|data parameter seen. */ @@ -339,9 +340,8 @@ data_can_multi_conn (void *handle) static int data_pread (void *handle, void *buf, uint32_t count, uint64_t offset) { - pthread_mutex_lock (&lock); + ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&lock); sparse_array_read (sa, buf, count, offset); - pthread_mutex_unlock (&lock); return 0; } @@ -349,21 +349,16 @@ data_pread (void *handle, void *buf, uint32_t count, uint64_t offset) static int data_pwrite (void *handle, const...

...veau.c > index 1c723b9..d411523 100644 > --- a/nouveau/nouveau.c > +++ b/nouveau/nouveau.c > @@ -349,8 +349,8 @@ nouveau_bo_del(struct nouveau_bo *bo) > struct nouveau_bo_priv *nvbo = nouveau_bo(bo); > struct drm_gem_close req = { bo->handle }; > > - pthread_mutex_lock(&nvdev->lock); > - if (nvbo->name) { > + if (nvbo->head.next) { > + pthread_mutex_lock(&nvdev->lock); > if (atomic_read(&nvbo->refcnt) == 0) { > DRMLISTDEL(&nvbo->head); >...

...you replace "math" with printf(), it retests the value of globalFlag. Printf(), which has absolutely no barrier semantics at all. In other words, GCC conservatively will assume any function call will require a reload because it could modify anything. The code will work as-is. Obviously, pthread_mutex_lock() does not modify the value of `nvbo->head.next`. Once GCC knows that (and maybe other compilers already do), perhaps with LTO, it doesn't have to generate a second load. If you ever switch to an inlined lock or different compiler that understands that, your code will break. I suggest you si...

...>>>>>> @@ -349,8 +349,8 @@ nouveau_bo_del(struct nouveau_bo *bo) >>>>>> struct nouveau_bo_priv *nvbo = nouveau_bo(bo); >>>>>> struct drm_gem_close req = { bo->handle }; >>>>>> >>>>>> - pthread_mutex_lock(&nvdev->lock); >>>>>> - if (nvbo->name) { >>>>>> + if (nvbo->head.next) { >>>>>> + pthread_mutex_lock(&nvdev->lock); >>>>>> if (atomic_read(&nvbo->refcnt) ==...

...al_size (void) +{ + ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&lock); + return real_size; +} + static int parse_round_param (const char *key, const char *value, unsigned *ret) { @@ -147,7 +154,7 @@ truncate_get_size (struct nbdkit_next_ops *next_ops, void *nxdata, if (r == -1) return -1; - pthread_mutex_lock (&lock); + ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&lock); real_size = size = r; @@ -163,8 +170,6 @@ truncate_get_size (struct nbdkit_next_ops *next_ops, void *nxdata, size = ROUND_DOWN (size, round_down); ret = size; - pthread_mutex_unlock (&lock); - return ret; } @@ -176,...

There's more that can be done (in particular, use of CLEANUP_FREE), but this is enough to at least see if I'm on the right track. I couldn't figure out an obvious difference between common/include and common/utils, but it looks like the former is for things that are inlineable via .h only, while the latter is when you need to link in a convenience library, so this landed in the

...gt;>>> +++ b/nouveau/nouveau.c >>>> @@ -349,8 +349,8 @@ nouveau_bo_del(struct nouveau_bo *bo) >>>> struct nouveau_bo_priv *nvbo = nouveau_bo(bo); >>>> struct drm_gem_close req = { bo->handle }; >>>> >>>> - pthread_mutex_lock(&nvdev->lock); >>>> - if (nvbo->name) { >>>> + if (nvbo->head.next) { >>>> + pthread_mutex_lock(&nvdev->lock); >>>> if (atomic_read(&nvbo->refcnt) == 0) { >>>>...

...lose(nvdev->base.fd); free(nvdev->client); + pthread_mutex_destroy(&nvdev->lock); free(nvdev); *pdev = NULL; } @@ -191,6 +198,8 @@ nouveau_client_new(struct nouveau_device *dev, struct nouveau_client **pclient) int id = 0, i, ret = -ENOMEM; uint32_t *clients; + pthread_mutex_lock(&nvdev->lock); + for (i = 0; i < nvdev->nr_client; i++) { id = ffs(nvdev->client[i]) - 1; if (id >= 0) @@ -199,7 +208,7 @@ nouveau_client_new(struct nouveau_device *dev, struct nouveau_client **pclient) clients = realloc(nvdev->client, sizeof(uint32_t) * (i +...

...r/generator index ea6f739..8068762 100755 --- a/generator/generator +++ b/generator/generator @@ -2820,10 +2820,9 @@ let generate_lib_api_c () = pr " nbd_internal_reset_error (\"nbd_%s\");\n" name; pr "\n" ); + if is_locked then + pr " pthread_mutex_lock (&h->lock);\n"; if permitted_states <> [] then ( - pr " /* We can check the state outside the handle lock because the\n"; - pr " * the state is atomic.\n"; - pr " */\n"; let tests = List.map ( functi...

...id *handle) { int64_t size; + int r; size = next_ops->get_size (nxdata); if (size == -1) @@ -139,7 +146,10 @@ cache_get_size (struct nbdkit_next_ops *next_ops, void *nxdata, nbdkit_debug ("cache: underlying file size: %" PRIi64, size); - if (blk_set_size (size)) + pthread_mutex_lock (&lock); + r = blk_set_size (size); + pthread_mutex_unlock (&lock); + if (r == -1) return -1; return size; @@ -179,6 +189,7 @@ cache_pread (struct nbdkit_next_ops *next_ops, void *nxdata, while (count > 0) { uint64_t blknum, blkoffs, n; + int r; blknum =...

...411523 100644 >> --- a/nouveau/nouveau.c >> +++ b/nouveau/nouveau.c >> @@ -349,8 +349,8 @@ nouveau_bo_del(struct nouveau_bo *bo) >> struct nouveau_bo_priv *nvbo = nouveau_bo(bo); >> struct drm_gem_close req = { bo->handle }; >> >> - pthread_mutex_lock(&nvdev->lock); >> - if (nvbo->name) { >> + if (nvbo->head.next) { >> + pthread_mutex_lock(&nvdev->lock); >> if (atomic_read(&nvbo->refcnt) == 0) { >> DRMLISTDEL(&nvbo->h...

On Wed, Feb 25, 2015 at 11:59 AM, Patrick Baggett <baggett.patrick at gmail.com> wrote: >> If code like >> >> x = *a; >> pthread_mutex_lock or unlock or __memory_barrier() >> y = *a; >> >> doesn't cause a to get loaded twice, then the compiler's in serious >> trouble. Basically functions like pthread_mutex_lock imply that all >> memory is changed to the compiler, and thus need to be reloaded. >&...