* [PATCH 1/3, libgomp] Resolve libgomp plugin deadlock on exit, libgomp proper parts [not found] <566EC2F4.1030109@codesourcery.com> @ 2015-12-14 15:47 ` Chung-Lin Tang 2016-01-05 14:22 ` Chung-Lin Tang 0 siblings, 1 reply; 4+ messages in thread From: Chung-Lin Tang @ 2015-12-14 15:47 UTC (permalink / raw) To: Jakub Jelinek, Nathan Sidwell, Thomas Schwinge, Verbin, Ilya, gcc-patches [-- Attachment #1: Type: text/plain, Size: 2752 bytes --] [sorry, forgot to C gcc-patches in last send] Hi Jakub, these patches are a revision of https://gcc.gnu.org/ml/gcc-patches/2015-08/msg01701.html since that patch set have bitrotten by now. To recap the original situation, due to the way that device locks are held when entering plugin code, a GOMP_PLUGIN_fatal() call will deadlock when the GOMP_unregister_var() exit destructor tries to obtain the same device lock. This patch set revises many functions on libgomp plugin interface to return false on error, and back to libgomp to release the lock and call gomp_fatal() there. This first patch is the changes for the machine independent libgomp proper. The entire patch set was tested without regressions. Is this okay for trunk? Thanks, Chung-Lin 2015-12-14 Chung-Lin Tang <cltang@codesourcery.com> * target.c (gomp_device_copy): New function. (gomp_copy_host2dev): Likewise. (gomp_copy_dev2host): Likewise. (gomp_free_device_memory): Likewise. (gomp_map_vars_existing): Adjust to call gomp_copy_host2dev(). (gomp_map_pointer): Likewise. (gomp_map_vars): Adjust to call gomp_copy_host2dev(), handle NULL value from alloc_func plugin hook. (gomp_unmap_tgt): Adjust to call gomp_free_device_memory(). (gomp_copy_from_async): Adjust to call gomp_copy_dev2host(). (gomp_unmap_vars): Likewise. (gomp_update): Adjust to call gomp_copy_dev2host() and gomp_copy_host2dev() functions. (gomp_init_device): Handle false value from init_device_func plugin hook. (gomp_fini_device): Handle false value from fini_device_func plugin hook. (gomp_exit_data): Adjust to call gomp_copy_dev2host(). (omp_target_free): Adjust to call gomp_free_device_memory(). (omp_target_memcpy): Handle return values from host2dev_func, dev2host_func, and dev2dev_func plugin hooks. (omp_target_memcpy_rect_worker): Likewise. * libgomp.h (struct gomp_device_descr): Adjust return type of init_device_func, fini_device_func, free_func, dev2host_func, host2dev_func, and dev2dev_func plugin hooks from 'void *' to bool. * oacc-host.c (host_init_device): Change return type to bool. (host_fini_device): Likewise. (host_free): Likewise. (host_dev2host): Likewise. (host_host2dev): Likewise. * oacc-mem.c (acc_free): Handle plugin hook fatal error case. (acc_memcpy_to_device): Likewise. (acc_memcpy_from_device): Likewise. (delete_copyout): Add libfnname parameter, handle free_func hook fatal error case. (acc_delete): Adjust delete_copyout call. (acc_copyout): Likewise. [-- Attachment #2: 01-libgomp.patch --] [-- Type: text/x-patch, Size: 19730 bytes --] Index: libgomp/libgomp.h =================================================================== --- libgomp/libgomp.h (revision 231613) +++ libgomp/libgomp.h (working copy) @@ -914,16 +914,17 @@ struct gomp_device_descr unsigned int (*get_caps_func) (void); int (*get_type_func) (void); int (*get_num_devices_func) (void); - void (*init_device_func) (int); - void (*fini_device_func) (int); + bool (*init_device_func) (int); + bool (*fini_device_func) (int); unsigned (*version_func) (void); int (*load_image_func) (int, unsigned, const void *, struct addr_pair **); void (*unload_image_func) (int, unsigned, const void *); void *(*alloc_func) (int, size_t); - void (*free_func) (int, void *); - void *(*dev2host_func) (int, void *, const void *, size_t); - void *(*host2dev_func) (int, void *, const void *, size_t); - void *(*dev2dev_func) (int, void *, const void *, size_t); + bool (*free_func) (int, void *); + bool (*dev2host_func) (int, void *, const void *, size_t); + bool (*host2dev_func) (int, void *, const void *, size_t); + /*xxx*/ + bool (*dev2dev_func) (int, void *, const void *, size_t); void (*run_func) (int, void *, void *); void (*async_run_func) (int, void *, void *, void *); Index: libgomp/oacc-host.c =================================================================== --- libgomp/oacc-host.c (revision 231613) +++ libgomp/oacc-host.c (working copy) @@ -60,14 +60,16 @@ host_get_num_devices (void) return 1; } -static void +static bool host_init_device (int n __attribute__ ((unused))) { + return true; } -static void +static bool host_fini_device (int n __attribute__ ((unused))) { + return true; } static unsigned @@ -98,28 +100,29 @@ host_alloc (int n __attribute__ ((unused)), size_t return gomp_malloc (s); } -static void +static bool host_free (int n __attribute__ ((unused)), void *p) { free (p); + return true; } -static void * +static bool host_dev2host (int n __attribute__ ((unused)), void *h __attribute__ ((unused)), const void *d __attribute__ ((unused)), size_t s __attribute__ ((unused))) { - return NULL; + return true; } -static void * +static bool host_host2dev (int n __attribute__ ((unused)), void *d __attribute__ ((unused)), const void *h __attribute__ ((unused)), size_t s __attribute__ ((unused))) { - return NULL; + return true; } static void Index: libgomp/oacc-mem.c =================================================================== --- libgomp/oacc-mem.c (revision 231613) +++ libgomp/oacc-mem.c (working copy) @@ -142,7 +142,8 @@ acc_free (void *d) else gomp_mutex_unlock (&acc_dev->lock); - acc_dev->free_func (acc_dev->target_id, d); + if (!acc_dev->free_func (acc_dev->target_id, d)) + gomp_fatal ("error in freeing device memory in %s", __FUNCTION__); } void @@ -154,7 +155,8 @@ acc_memcpy_to_device (void *d, void *h, size_t s) assert (thr && thr->dev); - thr->dev->host2dev_func (thr->dev->target_id, d, h, s); + if (!thr->dev->host2dev_func (thr->dev->target_id, d, h, s)) + gomp_fatal ("error in %s", __FUNCTION__); } void @@ -166,7 +168,8 @@ acc_memcpy_from_device (void *h, void *d, size_t s assert (thr && thr->dev); - thr->dev->dev2host_func (thr->dev->target_id, h, d, s); + if (!thr->dev->dev2host_func (thr->dev->target_id, h, d, s)) + gomp_fatal ("error in %s", __FUNCTION__); } /* Return the device pointer that corresponds to host data H. Or NULL @@ -488,7 +491,7 @@ acc_present_or_copyin (void *h, size_t s) #define FLAG_COPYOUT (1 << 0) static void -delete_copyout (unsigned f, void *h, size_t s) +delete_copyout (unsigned f, void *h, size_t s, const char *libfnname) { size_t host_size; splay_tree_key n; @@ -527,18 +530,20 @@ static void acc_unmap_data (h); - acc_dev->free_func (acc_dev->target_id, d); + if (!acc_dev->free_func (acc_dev->target_id, d)) + gomp_fatal ("error in freeing device memory in %s", libfnname); } void acc_delete (void *h , size_t s) { - delete_copyout (0, h, s); + delete_copyout (0, h, s, __FUNCTION__); } -void acc_copyout (void *h, size_t s) +void +acc_copyout (void *h, size_t s) { - delete_copyout (FLAG_COPYOUT, h, s); + delete_copyout (FLAG_COPYOUT, h, s, __FUNCTION__); } static void Index: libgomp/target.c =================================================================== --- libgomp/target.c (revision 231613) +++ libgomp/target.c (working copy) @@ -157,6 +157,45 @@ gomp_map_0len_lookup (splay_tree mem_map, splay_tr return n; } +static inline void +gomp_device_copy (struct gomp_device_descr *devicep, + bool (*copy_func) (int, void *, const void *, size_t), + const char *dst, void *dstaddr, + const char *src, const void *srcaddr, + size_t size) +{ + if (!copy_func (devicep->target_id, dstaddr, srcaddr, size)) + { + gomp_mutex_unlock (&devicep->lock); + gomp_fatal ("Copying of %s object [%p..%p) to %s object [%p..%p) failed", + src, srcaddr, srcaddr + size, dst, dstaddr, dstaddr + size); + } +} + +static void +gomp_copy_host2dev (struct gomp_device_descr *devicep, + void *d, const void *h, size_t sz) +{ + gomp_device_copy (devicep, devicep->host2dev_func, "dev", d, "host", h, sz); +} + +static void +gomp_copy_dev2host (struct gomp_device_descr *devicep, + void *h, const void *d, size_t sz) +{ + gomp_device_copy (devicep, devicep->dev2host_func, "host", h, "dev", d, sz); +} + +static void +gomp_free_device_memory (struct gomp_device_descr *devicep, void *devptr) +{ + if (!devicep->free_func (devicep->target_id, devptr)) + { + gomp_mutex_unlock (&devicep->lock); + gomp_fatal ("error in freeing device memory block at %p", devptr); + } +} + /* Handle the case where gomp_map_lookup, splay_tree_lookup or gomp_map_0len_lookup found oldn for newn. Helper function of gomp_map_vars. */ @@ -184,11 +223,12 @@ gomp_map_vars_existing (struct gomp_device_descr * } if (GOMP_MAP_ALWAYS_TO_P (kind)) - devicep->host2dev_func (devicep->target_id, - (void *) (oldn->tgt->tgt_start + oldn->tgt_offset - + newn->host_start - oldn->host_start), - (void *) newn->host_start, - newn->host_end - newn->host_start); + gomp_copy_host2dev (devicep, + (void *) (oldn->tgt->tgt_start + oldn->tgt_offset + + newn->host_start - oldn->host_start), + (void *) newn->host_start, + newn->host_end - newn->host_start); + if (oldn->refcount != REFCOUNT_INFINITY) oldn->refcount++; } @@ -213,10 +253,10 @@ gomp_map_pointer (struct target_mem_desc *tgt, uin { cur_node.tgt_offset = (uintptr_t) NULL; /* FIXME: see comment about coalescing host/dev transfers below. */ - devicep->host2dev_func (devicep->target_id, - (void *) (tgt->tgt_start + target_offset), - (void *) &cur_node.tgt_offset, - sizeof (void *)); + gomp_copy_host2dev (devicep, + (void *) (tgt->tgt_start + target_offset), + (void *) &cur_node.tgt_offset, + sizeof (void *)); return; } /* Add bias to the pointer value. */ @@ -236,10 +276,8 @@ gomp_map_pointer (struct target_mem_desc *tgt, uin to initialize the pointer with. */ cur_node.tgt_offset -= bias; /* FIXME: see comment about coalescing host/dev transfers below. */ - devicep->host2dev_func (devicep->target_id, - (void *) (tgt->tgt_start + target_offset), - (void *) &cur_node.tgt_offset, - sizeof (void *)); + gomp_copy_host2dev (devicep, (void *) (tgt->tgt_start + target_offset), + (void *) &cur_node.tgt_offset, sizeof (void *)); } static void @@ -504,6 +542,12 @@ gomp_map_vars (struct gomp_device_descr *devicep, memory. */ tgt->to_free = devicep->alloc_func (devicep->target_id, tgt_size + tgt_align - 1); + if (!tgt->to_free) + { + gomp_mutex_unlock (&devicep->lock); + gomp_fatal ("device memory allocation fail"); + } + tgt->tgt_start = (uintptr_t) tgt->to_free; tgt->tgt_start = (tgt->tgt_start + tgt_align - 1) & ~(tgt_align - 1); tgt->tgt_end = tgt->tgt_start + tgt_size; @@ -543,9 +587,9 @@ gomp_map_vars (struct gomp_device_descr *devicep, tgt_size = (tgt_size + align - 1) & ~(align - 1); tgt->list[i].offset = tgt_size; len = sizes[i]; - devicep->host2dev_func (devicep->target_id, - (void *) (tgt->tgt_start + tgt_size), - (void *) hostaddrs[i], len); + gomp_copy_host2dev (devicep, + (void *) (tgt->tgt_start + tgt_size), + (void *) hostaddrs[i], len); tgt_size += len; continue; case GOMP_MAP_FIRSTPRIVATE_INT: @@ -597,13 +641,13 @@ gomp_map_vars (struct gomp_device_descr *devicep, cur_node.tgt_offset = gomp_map_val (tgt, hostaddrs, i - 1); if (cur_node.tgt_offset) cur_node.tgt_offset -= sizes[i]; - devicep->host2dev_func (devicep->target_id, - (void *) (n->tgt->tgt_start - + n->tgt_offset - + cur_node.host_start - - n->host_start), - (void *) &cur_node.tgt_offset, - sizeof (void *)); + gomp_copy_host2dev (devicep, + (void *) (n->tgt->tgt_start + + n->tgt_offset + + cur_node.host_start + - n->host_start), + (void *) &cur_node.tgt_offset, + sizeof (void *)); cur_node.tgt_offset = n->tgt->tgt_start + n->tgt_offset + cur_node.host_start - n->host_start; continue; @@ -666,11 +710,11 @@ gomp_map_vars (struct gomp_device_descr *devicep, /* FIXME: Perhaps add some smarts, like if copying several adjacent fields from host to target, use some host buffer to avoid sending each var individually. */ - devicep->host2dev_func (devicep->target_id, - (void *) (tgt->tgt_start - + k->tgt_offset), - (void *) k->host_start, - k->host_end - k->host_start); + gomp_copy_host2dev (devicep, + (void *) (tgt->tgt_start + + k->tgt_offset), + (void *) k->host_start, + k->host_end - k->host_start); break; case GOMP_MAP_POINTER: gomp_map_pointer (tgt, (uintptr_t) *(void **) k->host_start, @@ -678,11 +722,11 @@ gomp_map_vars (struct gomp_device_descr *devicep, break; case GOMP_MAP_TO_PSET: /* FIXME: see above FIXME comment. */ - devicep->host2dev_func (devicep->target_id, - (void *) (tgt->tgt_start - + k->tgt_offset), - (void *) k->host_start, - k->host_end - k->host_start); + gomp_copy_host2dev (devicep, + (void *) (tgt->tgt_start + + k->tgt_offset), + (void *) k->host_start, + k->host_end - k->host_start); for (j = i + 1; j < mapnum; j++) if (!GOMP_MAP_POINTER_P (get_kind (short_mapkind, kinds, @@ -729,12 +773,11 @@ gomp_map_vars (struct gomp_device_descr *devicep, break; case GOMP_MAP_FORCE_DEVICEPTR: assert (k->host_end - k->host_start == sizeof (void *)); - - devicep->host2dev_func (devicep->target_id, - (void *) (tgt->tgt_start - + k->tgt_offset), - (void *) k->host_start, - sizeof (void *)); + gomp_copy_host2dev (devicep, + (void *) (tgt->tgt_start + + k->tgt_offset), + (void *) k->host_start, + sizeof (void *)); break; default: gomp_mutex_unlock (&devicep->lock); @@ -752,11 +795,9 @@ gomp_map_vars (struct gomp_device_descr *devicep, { cur_node.tgt_offset = gomp_map_val (tgt, hostaddrs, i); /* FIXME: see above FIXME comment. */ - devicep->host2dev_func (devicep->target_id, - (void *) (tgt->tgt_start - + i * sizeof (void *)), - (void *) &cur_node.tgt_offset, - sizeof (void *)); + gomp_copy_host2dev (devicep, + (void *) (tgt->tgt_start + i * sizeof (void *)), + (void *) &cur_node.tgt_offset, sizeof (void *)); } } @@ -778,7 +819,7 @@ gomp_unmap_tgt (struct target_mem_desc *tgt) { /* Deallocate on target the tgt->tgt_start .. tgt->tgt_end region. */ if (tgt->tgt_end) - tgt->device_descr->free_func (tgt->device_descr->target_id, tgt->to_free); + gomp_free_device_memory (tgt->device_descr, tgt->to_free); free (tgt->array); free (tgt); @@ -810,9 +851,9 @@ gomp_copy_from_async (struct target_mem_desc *tgt) { splay_tree_key k = tgt->list[i].key; if (tgt->list[i].copy_from) - devicep->dev2host_func (devicep->target_id, (void *) k->host_start, - (void *) (k->tgt->tgt_start + k->tgt_offset), - k->host_end - k->host_start); + gomp_copy_dev2host (devicep, (void *) k->host_start, + (void *) (k->tgt->tgt_start + k->tgt_offset), + k->host_end - k->host_start); } gomp_mutex_unlock (&devicep->lock); @@ -858,11 +899,11 @@ gomp_unmap_vars (struct target_mem_desc *tgt, bool if ((do_unmap && do_copyfrom && tgt->list[i].copy_from) || tgt->list[i].always_copy_from) - devicep->dev2host_func (devicep->target_id, - (void *) (k->host_start + tgt->list[i].offset), - (void *) (k->tgt->tgt_start + k->tgt_offset - + tgt->list[i].offset), - tgt->list[i].length); + gomp_copy_dev2host (devicep, + (void *) (k->host_start + tgt->list[i].offset), + (void *) (k->tgt->tgt_start + k->tgt_offset + + tgt->list[i].offset), + tgt->list[i].length); if (do_unmap) { splay_tree_remove (&devicep->mem_map, k); @@ -916,22 +957,17 @@ gomp_update (struct gomp_device_descr *devicep, si (void *) n->host_start, (void *) n->host_end); } + + + void *hostaddr = (void *) cur_node.host_start; + void *devaddr = (void *) (n->tgt->tgt_start + n->tgt_offset + + cur_node.host_start - n->host_start); + size_t size = cur_node.host_end - cur_node.host_start; + if (GOMP_MAP_COPY_TO_P (kind & typemask)) - devicep->host2dev_func (devicep->target_id, - (void *) (n->tgt->tgt_start - + n->tgt_offset - + cur_node.host_start - - n->host_start), - (void *) cur_node.host_start, - cur_node.host_end - cur_node.host_start); + gomp_copy_host2dev (devicep, devaddr, hostaddr, size); if (GOMP_MAP_COPY_FROM_P (kind & typemask)) - devicep->dev2host_func (devicep->target_id, - (void *) cur_node.host_start, - (void *) (n->tgt->tgt_start - + n->tgt_offset - + cur_node.host_start - - n->host_start), - cur_node.host_end - cur_node.host_start); + gomp_copy_dev2host (devicep, hostaddr, devaddr, size); } } gomp_mutex_unlock (&devicep->lock); @@ -1181,7 +1217,11 @@ attribute_hidden void gomp_init_device (struct gomp_device_descr *devicep) { int i; - devicep->init_device_func (devicep->target_id); + if (!devicep->init_device_func (devicep->target_id)) + { + gomp_mutex_unlock (&devicep->lock); + gomp_fatal ("device initialization failed"); + } /* Load to device all images registered by the moment. */ for (i = 0; i < num_offload_images; i++) @@ -1238,9 +1278,15 @@ attribute_hidden void gomp_fini_device (struct gomp_device_descr *devicep) { if (devicep->is_initialized) - devicep->fini_device_func (devicep->target_id); + { + if (!devicep->fini_device_func (devicep->target_id)) + { + gomp_mutex_unlock (&devicep->lock); + gomp_fatal ("device finalization failed"); + } - devicep->is_initialized = false; + devicep->is_initialized = false; + } } /* Host fallback for GOMP_target{,_ext} routines. */ @@ -1623,12 +1669,11 @@ gomp_exit_data (struct gomp_device_descr *devicep, if ((kind == GOMP_MAP_FROM && k->refcount == 0) || kind == GOMP_MAP_ALWAYS_FROM) - devicep->dev2host_func (devicep->target_id, - (void *) cur_node.host_start, - (void *) (k->tgt->tgt_start + k->tgt_offset - + cur_node.host_start - - k->host_start), - cur_node.host_end - cur_node.host_start); + gomp_copy_dev2host (devicep, (void *) cur_node.host_start, + (void *) (k->tgt->tgt_start + k->tgt_offset + + cur_node.host_start + - k->host_start), + cur_node.host_end - cur_node.host_start); if (k->refcount == 0) { splay_tree_remove (&devicep->mem_map, k); @@ -1842,7 +1887,7 @@ omp_target_free (void *device_ptr, int device_num) } gomp_mutex_lock (&devicep->lock); - devicep->free_func (devicep->target_id, device_ptr); + gomp_free_device_memory (devicep, device_ptr); gomp_mutex_unlock (&devicep->lock); } @@ -1882,6 +1927,7 @@ omp_target_memcpy (void *dst, void *src, size_t le size_t src_offset, int dst_device_num, int src_device_num) { struct gomp_device_descr *dst_devicep = NULL, *src_devicep = NULL; + bool ret; if (dst_device_num != GOMP_DEVICE_HOST_FALLBACK) { @@ -1915,29 +1961,29 @@ omp_target_memcpy (void *dst, void *src, size_t le if (src_devicep == NULL) { gomp_mutex_lock (&dst_devicep->lock); - dst_devicep->host2dev_func (dst_devicep->target_id, - (char *) dst + dst_offset, - (char *) src + src_offset, length); + ret = dst_devicep->host2dev_func (dst_devicep->target_id, + (char *) dst + dst_offset, + (char *) src + src_offset, length); gomp_mutex_unlock (&dst_devicep->lock); - return 0; + return (ret ? 0 : EINVAL); } if (dst_devicep == NULL) { gomp_mutex_lock (&src_devicep->lock); - src_devicep->dev2host_func (src_devicep->target_id, - (char *) dst + dst_offset, - (char *) src + src_offset, length); + ret = src_devicep->dev2host_func (src_devicep->target_id, + (char *) dst + dst_offset, + (char *) src + src_offset, length); gomp_mutex_unlock (&src_devicep->lock); - return 0; + return (ret ? 0 : EINVAL); } if (src_devicep == dst_devicep) { gomp_mutex_lock (&src_devicep->lock); - src_devicep->dev2dev_func (src_devicep->target_id, - (char *) dst + dst_offset, - (char *) src + src_offset, length); + ret = src_devicep->dev2dev_func (src_devicep->target_id, + (char *) dst + dst_offset, + (char *) src + src_offset, length); gomp_mutex_unlock (&src_devicep->lock); - return 0; + return (ret ? 0 : EINVAL); } return EINVAL; } @@ -1964,22 +2010,25 @@ omp_target_memcpy_rect_worker (void *dst, void *sr || __builtin_mul_overflow (element_size, src_offsets[0], &src_off)) return EINVAL; if (dst_devicep == NULL && src_devicep == NULL) - memcpy ((char *) dst + dst_off, (char *) src + src_off, length); + { + memcpy ((char *) dst + dst_off, (char *) src + src_off, length); + ret = 1; + } else if (src_devicep == NULL) - dst_devicep->host2dev_func (dst_devicep->target_id, - (char *) dst + dst_off, - (char *) src + src_off, length); + ret = dst_devicep->host2dev_func (dst_devicep->target_id, + (char *) dst + dst_off, + (char *) src + src_off, length); else if (dst_devicep == NULL) - src_devicep->dev2host_func (src_devicep->target_id, - (char *) dst + dst_off, - (char *) src + src_off, length); + ret = src_devicep->dev2host_func (src_devicep->target_id, + (char *) dst + dst_off, + (char *) src + src_off, length); else if (src_devicep == dst_devicep) - src_devicep->dev2dev_func (src_devicep->target_id, - (char *) dst + dst_off, - (char *) src + src_off, length); + ret = src_devicep->dev2dev_func (src_devicep->target_id, + (char *) dst + dst_off, + (char *) src + src_off, length); else - return EINVAL; - return 0; + ret = 0; + return ret ? 0 : EINVAL; } /* FIXME: it would be nice to have some plugin function to handle ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 1/3, libgomp] Resolve libgomp plugin deadlock on exit, libgomp proper parts 2015-12-14 15:47 ` [PATCH 1/3, libgomp] Resolve libgomp plugin deadlock on exit, libgomp proper parts Chung-Lin Tang @ 2016-01-05 14:22 ` Chung-Lin Tang 2016-01-19 6:02 ` Chung-Lin Tang 0 siblings, 1 reply; 4+ messages in thread From: Chung-Lin Tang @ 2016-01-05 14:22 UTC (permalink / raw) To: Jakub Jelinek, Nathan Sidwell, Thomas Schwinge, Verbin, Ilya, gcc-patches [-- Attachment #1: Type: text/plain, Size: 3012 bytes --] Patch has been updated to accommodate the gomp_fini_device() removal changes. And ping. On 2015/12/14 11:47 PM, Chung-Lin Tang wrote: > [sorry, forgot to C gcc-patches in last send] > > Hi Jakub, > these patches are a revision of https://gcc.gnu.org/ml/gcc-patches/2015-08/msg01701.html > since that patch set have bitrotten by now. > > To recap the original situation, due to the way that device locks are held > when entering plugin code, a GOMP_PLUGIN_fatal() call will deadlock when the > GOMP_unregister_var() exit destructor tries to obtain the same device lock. > > This patch set revises many functions on libgomp plugin interface to return false on error, > and back to libgomp to release the lock and call gomp_fatal() there. > > This first patch is the changes for the machine independent libgomp proper. The entire patch > set was tested without regressions. Is this okay for trunk? > > Thanks, > Chung-Lin > > 2015-12-14 Chung-Lin Tang <cltang@codesourcery.com> > > * target.c (gomp_device_copy): New function. > (gomp_copy_host2dev): Likewise. > (gomp_copy_dev2host): Likewise. > (gomp_free_device_memory): Likewise. > (gomp_map_vars_existing): Adjust to call gomp_copy_host2dev(). > (gomp_map_pointer): Likewise. > (gomp_map_vars): Adjust to call gomp_copy_host2dev(), handle > NULL value from alloc_func plugin hook. > (gomp_unmap_tgt): Adjust to call gomp_free_device_memory(). > (gomp_copy_from_async): Adjust to call gomp_copy_dev2host(). > (gomp_unmap_vars): Likewise. > (gomp_update): Adjust to call gomp_copy_dev2host() and > gomp_copy_host2dev() functions. > (gomp_init_device): Handle false value from init_device_func > plugin hook. > (gomp_fini_device): Handle false value from fini_device_func > plugin hook. > (gomp_exit_data): Adjust to call gomp_copy_dev2host(). > (omp_target_free): Adjust to call gomp_free_device_memory(). > (omp_target_memcpy): Handle return values from host2dev_func, > dev2host_func, and dev2dev_func plugin hooks. > (omp_target_memcpy_rect_worker): Likewise. > * libgomp.h (struct gomp_device_descr): Adjust return type of > init_device_func, fini_device_func, free_func, dev2host_func, > host2dev_func, and dev2dev_func plugin hooks from 'void *' to > bool. > * oacc-host.c (host_init_device): Change return type to bool. > (host_fini_device): Likewise. > (host_free): Likewise. > (host_dev2host): Likewise. > (host_host2dev): Likewise. > * oacc-mem.c (acc_free): Handle plugin hook fatal error case. > (acc_memcpy_to_device): Likewise. > (acc_memcpy_from_device): Likewise. > (delete_copyout): Add libfnname parameter, handle free_func > hook fatal error case. > (acc_delete): Adjust delete_copyout call. > (acc_copyout): Likewise. > > > [-- Attachment #2: x.diff --] [-- Type: text/plain, Size: 20626 bytes --] Index: libgomp.h =================================================================== --- libgomp.h (revision 232047) +++ libgomp.h (working copy) @@ -927,16 +927,17 @@ struct gomp_device_descr unsigned int (*get_caps_func) (void); int (*get_type_func) (void); int (*get_num_devices_func) (void); - void (*init_device_func) (int); - void (*fini_device_func) (int); + bool (*init_device_func) (int); + bool (*fini_device_func) (int); unsigned (*version_func) (void); int (*load_image_func) (int, unsigned, const void *, struct addr_pair **); void (*unload_image_func) (int, unsigned, const void *); void *(*alloc_func) (int, size_t); - void (*free_func) (int, void *); - void *(*dev2host_func) (int, void *, const void *, size_t); - void *(*host2dev_func) (int, void *, const void *, size_t); - void *(*dev2dev_func) (int, void *, const void *, size_t); + bool (*free_func) (int, void *); + bool (*dev2host_func) (int, void *, const void *, size_t); + bool (*host2dev_func) (int, void *, const void *, size_t); + /*xxx*/ + bool (*dev2dev_func) (int, void *, const void *, size_t); void (*run_func) (int, void *, void *); void (*async_run_func) (int, void *, void *, void *); Index: oacc-host.c =================================================================== --- oacc-host.c (revision 232047) +++ oacc-host.c (working copy) @@ -60,14 +60,16 @@ host_get_num_devices (void) return 1; } -static void +static bool host_init_device (int n __attribute__ ((unused))) { + return true; } -static void +static bool host_fini_device (int n __attribute__ ((unused))) { + return true; } static unsigned @@ -98,28 +100,29 @@ host_alloc (int n __attribute__ ((unused)), size_t return gomp_malloc (s); } -static void +static bool host_free (int n __attribute__ ((unused)), void *p) { free (p); + return true; } -static void * +static bool host_dev2host (int n __attribute__ ((unused)), void *h __attribute__ ((unused)), const void *d __attribute__ ((unused)), size_t s __attribute__ ((unused))) { - return NULL; + return true; } -static void * +static bool host_host2dev (int n __attribute__ ((unused)), void *d __attribute__ ((unused)), const void *h __attribute__ ((unused)), size_t s __attribute__ ((unused))) { - return NULL; + return true; } static void Index: oacc-init.c =================================================================== --- oacc-init.c (revision 232047) +++ oacc-init.c (working copy) @@ -300,8 +300,8 @@ acc_shutdown_1 (acc_device_t d) gomp_mutex_unlock (&goacc_thread_lock); - /* Close all the devices of this type that have been opened. */ + bool ret = true; for (i = 0; i < ndevs; i++) { struct gomp_device_descr *acc_dev = &base_dev[i]; @@ -309,12 +309,15 @@ acc_shutdown_1 (acc_device_t d) if (acc_dev->state == GOMP_DEVICE_INITIALIZED) { devices_active = true; - acc_dev->fini_device_func (acc_dev->target_id); + ret &= acc_dev->fini_device_func (acc_dev->target_id); acc_dev->state = GOMP_DEVICE_UNINITIALIZED; } gomp_mutex_unlock (&acc_dev->lock); } + if (!ret) + gomp_fatal ("device finalization failed"); + if (!devices_active) gomp_fatal ("no device initialized"); } Index: oacc-mem.c =================================================================== --- oacc-mem.c (revision 232047) +++ oacc-mem.c (working copy) @@ -142,7 +142,8 @@ acc_free (void *d) else gomp_mutex_unlock (&acc_dev->lock); - acc_dev->free_func (acc_dev->target_id, d); + if (!acc_dev->free_func (acc_dev->target_id, d)) + gomp_fatal ("error in freeing device memory in %s", __FUNCTION__); } void @@ -154,7 +155,8 @@ acc_memcpy_to_device (void *d, void *h, size_t s) assert (thr && thr->dev); - thr->dev->host2dev_func (thr->dev->target_id, d, h, s); + if (!thr->dev->host2dev_func (thr->dev->target_id, d, h, s)) + gomp_fatal ("error in %s", __FUNCTION__); } void @@ -166,7 +168,8 @@ acc_memcpy_from_device (void *h, void *d, size_t s assert (thr && thr->dev); - thr->dev->dev2host_func (thr->dev->target_id, h, d, s); + if (!thr->dev->dev2host_func (thr->dev->target_id, h, d, s)) + gomp_fatal ("error in %s", __FUNCTION__); } /* Return the device pointer that corresponds to host data H. Or NULL @@ -488,7 +491,7 @@ acc_present_or_copyin (void *h, size_t s) #define FLAG_COPYOUT (1 << 0) static void -delete_copyout (unsigned f, void *h, size_t s) +delete_copyout (unsigned f, void *h, size_t s, const char *libfnname) { size_t host_size; splay_tree_key n; @@ -527,18 +530,20 @@ static void acc_unmap_data (h); - acc_dev->free_func (acc_dev->target_id, d); + if (!acc_dev->free_func (acc_dev->target_id, d)) + gomp_fatal ("error in freeing device memory in %s", libfnname); } void acc_delete (void *h , size_t s) { - delete_copyout (0, h, s); + delete_copyout (0, h, s, __FUNCTION__); } -void acc_copyout (void *h, size_t s) +void +acc_copyout (void *h, size_t s) { - delete_copyout (FLAG_COPYOUT, h, s); + delete_copyout (FLAG_COPYOUT, h, s, __FUNCTION__); } static void Index: target.c =================================================================== --- target.c (revision 232047) +++ target.c (working copy) @@ -162,6 +162,45 @@ gomp_map_0len_lookup (splay_tree mem_map, splay_tr return n; } +static inline void +gomp_device_copy (struct gomp_device_descr *devicep, + bool (*copy_func) (int, void *, const void *, size_t), + const char *dst, void *dstaddr, + const char *src, const void *srcaddr, + size_t size) +{ + if (!copy_func (devicep->target_id, dstaddr, srcaddr, size)) + { + gomp_mutex_unlock (&devicep->lock); + gomp_fatal ("Copying of %s object [%p..%p) to %s object [%p..%p) failed", + src, srcaddr, srcaddr + size, dst, dstaddr, dstaddr + size); + } +} + +static void +gomp_copy_host2dev (struct gomp_device_descr *devicep, + void *d, const void *h, size_t sz) +{ + gomp_device_copy (devicep, devicep->host2dev_func, "dev", d, "host", h, sz); +} + +static void +gomp_copy_dev2host (struct gomp_device_descr *devicep, + void *h, const void *d, size_t sz) +{ + gomp_device_copy (devicep, devicep->dev2host_func, "host", h, "dev", d, sz); +} + +static void +gomp_free_device_memory (struct gomp_device_descr *devicep, void *devptr) +{ + if (!devicep->free_func (devicep->target_id, devptr)) + { + gomp_mutex_unlock (&devicep->lock); + gomp_fatal ("error in freeing device memory block at %p", devptr); + } +} + /* Handle the case where gomp_map_lookup, splay_tree_lookup or gomp_map_0len_lookup found oldn for newn. Helper function of gomp_map_vars. */ @@ -189,11 +228,12 @@ gomp_map_vars_existing (struct gomp_device_descr * } if (GOMP_MAP_ALWAYS_TO_P (kind)) - devicep->host2dev_func (devicep->target_id, - (void *) (oldn->tgt->tgt_start + oldn->tgt_offset - + newn->host_start - oldn->host_start), - (void *) newn->host_start, - newn->host_end - newn->host_start); + gomp_copy_host2dev (devicep, + (void *) (oldn->tgt->tgt_start + oldn->tgt_offset + + newn->host_start - oldn->host_start), + (void *) newn->host_start, + newn->host_end - newn->host_start); + if (oldn->refcount != REFCOUNT_INFINITY) oldn->refcount++; } @@ -218,10 +258,10 @@ gomp_map_pointer (struct target_mem_desc *tgt, uin { cur_node.tgt_offset = (uintptr_t) NULL; /* FIXME: see comment about coalescing host/dev transfers below. */ - devicep->host2dev_func (devicep->target_id, - (void *) (tgt->tgt_start + target_offset), - (void *) &cur_node.tgt_offset, - sizeof (void *)); + gomp_copy_host2dev (devicep, + (void *) (tgt->tgt_start + target_offset), + (void *) &cur_node.tgt_offset, + sizeof (void *)); return; } /* Add bias to the pointer value. */ @@ -241,10 +281,8 @@ gomp_map_pointer (struct target_mem_desc *tgt, uin to initialize the pointer with. */ cur_node.tgt_offset -= bias; /* FIXME: see comment about coalescing host/dev transfers below. */ - devicep->host2dev_func (devicep->target_id, - (void *) (tgt->tgt_start + target_offset), - (void *) &cur_node.tgt_offset, - sizeof (void *)); + gomp_copy_host2dev (devicep, (void *) (tgt->tgt_start + target_offset), + (void *) &cur_node.tgt_offset, sizeof (void *)); } static void @@ -515,6 +553,12 @@ gomp_map_vars (struct gomp_device_descr *devicep, memory. */ tgt->to_free = devicep->alloc_func (devicep->target_id, tgt_size + tgt_align - 1); + if (!tgt->to_free) + { + gomp_mutex_unlock (&devicep->lock); + gomp_fatal ("device memory allocation fail"); + } + tgt->tgt_start = (uintptr_t) tgt->to_free; tgt->tgt_start = (tgt->tgt_start + tgt_align - 1) & ~(tgt_align - 1); tgt->tgt_end = tgt->tgt_start + tgt_size; @@ -554,9 +598,9 @@ gomp_map_vars (struct gomp_device_descr *devicep, tgt_size = (tgt_size + align - 1) & ~(align - 1); tgt->list[i].offset = tgt_size; len = sizes[i]; - devicep->host2dev_func (devicep->target_id, - (void *) (tgt->tgt_start + tgt_size), - (void *) hostaddrs[i], len); + gomp_copy_host2dev (devicep, + (void *) (tgt->tgt_start + tgt_size), + (void *) hostaddrs[i], len); tgt_size += len; continue; case GOMP_MAP_FIRSTPRIVATE_INT: @@ -608,13 +652,13 @@ gomp_map_vars (struct gomp_device_descr *devicep, cur_node.tgt_offset = gomp_map_val (tgt, hostaddrs, i - 1); if (cur_node.tgt_offset) cur_node.tgt_offset -= sizes[i]; - devicep->host2dev_func (devicep->target_id, - (void *) (n->tgt->tgt_start - + n->tgt_offset - + cur_node.host_start - - n->host_start), - (void *) &cur_node.tgt_offset, - sizeof (void *)); + gomp_copy_host2dev (devicep, + (void *) (n->tgt->tgt_start + + n->tgt_offset + + cur_node.host_start + - n->host_start), + (void *) &cur_node.tgt_offset, + sizeof (void *)); cur_node.tgt_offset = n->tgt->tgt_start + n->tgt_offset + cur_node.host_start - n->host_start; continue; @@ -685,11 +729,11 @@ gomp_map_vars (struct gomp_device_descr *devicep, /* FIXME: Perhaps add some smarts, like if copying several adjacent fields from host to target, use some host buffer to avoid sending each var individually. */ - devicep->host2dev_func (devicep->target_id, - (void *) (tgt->tgt_start - + k->tgt_offset), - (void *) k->host_start, - k->host_end - k->host_start); + gomp_copy_host2dev (devicep, + (void *) (tgt->tgt_start + + k->tgt_offset), + (void *) k->host_start, + k->host_end - k->host_start); break; case GOMP_MAP_POINTER: gomp_map_pointer (tgt, (uintptr_t) *(void **) k->host_start, @@ -697,11 +741,11 @@ gomp_map_vars (struct gomp_device_descr *devicep, break; case GOMP_MAP_TO_PSET: /* FIXME: see above FIXME comment. */ - devicep->host2dev_func (devicep->target_id, - (void *) (tgt->tgt_start - + k->tgt_offset), - (void *) k->host_start, - k->host_end - k->host_start); + gomp_copy_host2dev (devicep, + (void *) (tgt->tgt_start + + k->tgt_offset), + (void *) k->host_start, + k->host_end - k->host_start); for (j = i + 1; j < mapnum; j++) if (!GOMP_MAP_POINTER_P (get_kind (short_mapkind, kinds, @@ -748,12 +792,11 @@ gomp_map_vars (struct gomp_device_descr *devicep, break; case GOMP_MAP_FORCE_DEVICEPTR: assert (k->host_end - k->host_start == sizeof (void *)); - - devicep->host2dev_func (devicep->target_id, - (void *) (tgt->tgt_start - + k->tgt_offset), - (void *) k->host_start, - sizeof (void *)); + gomp_copy_host2dev (devicep, + (void *) (tgt->tgt_start + + k->tgt_offset), + (void *) k->host_start, + sizeof (void *)); break; default: gomp_mutex_unlock (&devicep->lock); @@ -781,11 +824,9 @@ gomp_map_vars (struct gomp_device_descr *devicep, { cur_node.tgt_offset = gomp_map_val (tgt, hostaddrs, i); /* FIXME: see above FIXME comment. */ - devicep->host2dev_func (devicep->target_id, - (void *) (tgt->tgt_start - + i * sizeof (void *)), - (void *) &cur_node.tgt_offset, - sizeof (void *)); + gomp_copy_host2dev (devicep, + (void *) (tgt->tgt_start + i * sizeof (void *)), + (void *) &cur_node.tgt_offset, sizeof (void *)); } } @@ -807,7 +848,7 @@ gomp_unmap_tgt (struct target_mem_desc *tgt) { /* Deallocate on target the tgt->tgt_start .. tgt->tgt_end region. */ if (tgt->tgt_end) - tgt->device_descr->free_func (tgt->device_descr->target_id, tgt->to_free); + gomp_free_device_memory (tgt->device_descr, tgt->to_free); free (tgt->array); free (tgt); @@ -839,9 +880,9 @@ gomp_copy_from_async (struct target_mem_desc *tgt) { splay_tree_key k = tgt->list[i].key; if (tgt->list[i].copy_from) - devicep->dev2host_func (devicep->target_id, (void *) k->host_start, - (void *) (k->tgt->tgt_start + k->tgt_offset), - k->host_end - k->host_start); + gomp_copy_dev2host (devicep, (void *) k->host_start, + (void *) (k->tgt->tgt_start + k->tgt_offset), + k->host_end - k->host_start); } gomp_mutex_unlock (&devicep->lock); @@ -894,11 +935,11 @@ gomp_unmap_vars (struct target_mem_desc *tgt, bool if ((do_unmap && do_copyfrom && tgt->list[i].copy_from) || tgt->list[i].always_copy_from) - devicep->dev2host_func (devicep->target_id, - (void *) (k->host_start + tgt->list[i].offset), - (void *) (k->tgt->tgt_start + k->tgt_offset - + tgt->list[i].offset), - tgt->list[i].length); + gomp_copy_dev2host (devicep, + (void *) (k->host_start + tgt->list[i].offset), + (void *) (k->tgt->tgt_start + k->tgt_offset + + tgt->list[i].offset), + tgt->list[i].length); if (do_unmap) { splay_tree_remove (&devicep->mem_map, k); @@ -961,22 +1002,17 @@ gomp_update (struct gomp_device_descr *devicep, si (void *) n->host_start, (void *) n->host_end); } + + + void *hostaddr = (void *) cur_node.host_start; + void *devaddr = (void *) (n->tgt->tgt_start + n->tgt_offset + + cur_node.host_start - n->host_start); + size_t size = cur_node.host_end - cur_node.host_start; + if (GOMP_MAP_COPY_TO_P (kind & typemask)) - devicep->host2dev_func (devicep->target_id, - (void *) (n->tgt->tgt_start - + n->tgt_offset - + cur_node.host_start - - n->host_start), - (void *) cur_node.host_start, - cur_node.host_end - cur_node.host_start); + gomp_copy_host2dev (devicep, devaddr, hostaddr, size); if (GOMP_MAP_COPY_FROM_P (kind & typemask)) - devicep->dev2host_func (devicep->target_id, - (void *) cur_node.host_start, - (void *) (n->tgt->tgt_start - + n->tgt_offset - + cur_node.host_start - - n->host_start), - cur_node.host_end - cur_node.host_start); + gomp_copy_dev2host (devicep, hostaddr, devaddr, size); } } gomp_mutex_unlock (&devicep->lock); @@ -1261,7 +1297,11 @@ attribute_hidden void gomp_init_device (struct gomp_device_descr *devicep) { int i; - devicep->init_device_func (devicep->target_id); + if (!devicep->init_device_func (devicep->target_id)) + { + gomp_mutex_unlock (&devicep->lock); + gomp_fatal ("device initialization failed"); + } /* Load to device all images registered by the moment. */ for (i = 0; i < num_offload_images; i++) @@ -1703,12 +1743,11 @@ gomp_exit_data (struct gomp_device_descr *devicep, if ((kind == GOMP_MAP_FROM && k->refcount == 0) || kind == GOMP_MAP_ALWAYS_FROM) - devicep->dev2host_func (devicep->target_id, - (void *) cur_node.host_start, - (void *) (k->tgt->tgt_start + k->tgt_offset - + cur_node.host_start - - k->host_start), - cur_node.host_end - cur_node.host_start); + gomp_copy_dev2host (devicep, (void *) cur_node.host_start, + (void *) (k->tgt->tgt_start + k->tgt_offset + + cur_node.host_start + - k->host_start), + cur_node.host_end - cur_node.host_start); if (k->refcount == 0) { splay_tree_remove (&devicep->mem_map, k); @@ -1926,7 +1965,7 @@ omp_target_free (void *device_ptr, int device_num) } gomp_mutex_lock (&devicep->lock); - devicep->free_func (devicep->target_id, device_ptr); + gomp_free_device_memory (devicep, device_ptr); gomp_mutex_unlock (&devicep->lock); } @@ -1966,6 +2005,7 @@ omp_target_memcpy (void *dst, void *src, size_t le size_t src_offset, int dst_device_num, int src_device_num) { struct gomp_device_descr *dst_devicep = NULL, *src_devicep = NULL; + bool ret; if (dst_device_num != GOMP_DEVICE_HOST_FALLBACK) { @@ -1999,29 +2039,29 @@ omp_target_memcpy (void *dst, void *src, size_t le if (src_devicep == NULL) { gomp_mutex_lock (&dst_devicep->lock); - dst_devicep->host2dev_func (dst_devicep->target_id, - (char *) dst + dst_offset, - (char *) src + src_offset, length); + ret = dst_devicep->host2dev_func (dst_devicep->target_id, + (char *) dst + dst_offset, + (char *) src + src_offset, length); gomp_mutex_unlock (&dst_devicep->lock); - return 0; + return (ret ? 0 : EINVAL); } if (dst_devicep == NULL) { gomp_mutex_lock (&src_devicep->lock); - src_devicep->dev2host_func (src_devicep->target_id, - (char *) dst + dst_offset, - (char *) src + src_offset, length); + ret = src_devicep->dev2host_func (src_devicep->target_id, + (char *) dst + dst_offset, + (char *) src + src_offset, length); gomp_mutex_unlock (&src_devicep->lock); - return 0; + return (ret ? 0 : EINVAL); } if (src_devicep == dst_devicep) { gomp_mutex_lock (&src_devicep->lock); - src_devicep->dev2dev_func (src_devicep->target_id, - (char *) dst + dst_offset, - (char *) src + src_offset, length); + ret = src_devicep->dev2dev_func (src_devicep->target_id, + (char *) dst + dst_offset, + (char *) src + src_offset, length); gomp_mutex_unlock (&src_devicep->lock); - return 0; + return (ret ? 0 : EINVAL); } return EINVAL; } @@ -2048,22 +2088,25 @@ omp_target_memcpy_rect_worker (void *dst, void *sr || __builtin_mul_overflow (element_size, src_offsets[0], &src_off)) return EINVAL; if (dst_devicep == NULL && src_devicep == NULL) - memcpy ((char *) dst + dst_off, (char *) src + src_off, length); + { + memcpy ((char *) dst + dst_off, (char *) src + src_off, length); + ret = 1; + } else if (src_devicep == NULL) - dst_devicep->host2dev_func (dst_devicep->target_id, - (char *) dst + dst_off, - (char *) src + src_off, length); + ret = dst_devicep->host2dev_func (dst_devicep->target_id, + (char *) dst + dst_off, + (char *) src + src_off, length); else if (dst_devicep == NULL) - src_devicep->dev2host_func (src_devicep->target_id, - (char *) dst + dst_off, - (char *) src + src_off, length); + ret = src_devicep->dev2host_func (src_devicep->target_id, + (char *) dst + dst_off, + (char *) src + src_off, length); else if (src_devicep == dst_devicep) - src_devicep->dev2dev_func (src_devicep->target_id, - (char *) dst + dst_off, - (char *) src + src_off, length); + ret = src_devicep->dev2dev_func (src_devicep->target_id, + (char *) dst + dst_off, + (char *) src + src_off, length); else - return EINVAL; - return 0; + ret = 0; + return ret ? 0 : EINVAL; } /* FIXME: it would be nice to have some plugin function to handle @@ -2374,14 +2417,17 @@ gomp_target_fini (void) int i; for (i = 0; i < num_devices; i++) { + bool ret = true; struct gomp_device_descr *devicep = &devices[i]; gomp_mutex_lock (&devicep->lock); if (devicep->state == GOMP_DEVICE_INITIALIZED) { - devicep->fini_device_func (devicep->target_id); + ret = devicep->fini_device_func (devicep->target_id); devicep->state = GOMP_DEVICE_FINALIZED; } gomp_mutex_unlock (&devicep->lock); + if (!ret) + gomp_fatal ("device finalization failed"); } } ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 1/3, libgomp] Resolve libgomp plugin deadlock on exit, libgomp proper parts 2016-01-05 14:22 ` Chung-Lin Tang @ 2016-01-19 6:02 ` Chung-Lin Tang 2016-01-22 10:31 ` Jakub Jelinek 0 siblings, 1 reply; 4+ messages in thread From: Chung-Lin Tang @ 2016-01-19 6:02 UTC (permalink / raw) To: Jakub Jelinek, Nathan Sidwell, Thomas Schwinge, Verbin, Ilya, gcc-patches Ping x 2. On 2016/1/5 11:22 PM, Chung-Lin Tang wrote: > Patch has been updated to accommodate the gomp_fini_device() removal changes. > And ping. > > On 2015/12/14 11:47 PM, Chung-Lin Tang wrote: >> [sorry, forgot to C gcc-patches in last send] >> >> Hi Jakub, >> these patches are a revision of https://gcc.gnu.org/ml/gcc-patches/2015-08/msg01701.html >> since that patch set have bitrotten by now. >> >> To recap the original situation, due to the way that device locks are held >> when entering plugin code, a GOMP_PLUGIN_fatal() call will deadlock when the >> GOMP_unregister_var() exit destructor tries to obtain the same device lock. >> >> This patch set revises many functions on libgomp plugin interface to return false on error, >> and back to libgomp to release the lock and call gomp_fatal() there. >> >> This first patch is the changes for the machine independent libgomp proper. The entire patch >> set was tested without regressions. Is this okay for trunk? >> >> Thanks, >> Chung-Lin >> >> 2015-12-14 Chung-Lin Tang <cltang@codesourcery.com> >> >> * target.c (gomp_device_copy): New function. >> (gomp_copy_host2dev): Likewise. >> (gomp_copy_dev2host): Likewise. >> (gomp_free_device_memory): Likewise. >> (gomp_map_vars_existing): Adjust to call gomp_copy_host2dev(). >> (gomp_map_pointer): Likewise. >> (gomp_map_vars): Adjust to call gomp_copy_host2dev(), handle >> NULL value from alloc_func plugin hook. >> (gomp_unmap_tgt): Adjust to call gomp_free_device_memory(). >> (gomp_copy_from_async): Adjust to call gomp_copy_dev2host(). >> (gomp_unmap_vars): Likewise. >> (gomp_update): Adjust to call gomp_copy_dev2host() and >> gomp_copy_host2dev() functions. >> (gomp_init_device): Handle false value from init_device_func >> plugin hook. >> (gomp_fini_device): Handle false value from fini_device_func >> plugin hook. >> (gomp_exit_data): Adjust to call gomp_copy_dev2host(). >> (omp_target_free): Adjust to call gomp_free_device_memory(). >> (omp_target_memcpy): Handle return values from host2dev_func, >> dev2host_func, and dev2dev_func plugin hooks. >> (omp_target_memcpy_rect_worker): Likewise. >> * libgomp.h (struct gomp_device_descr): Adjust return type of >> init_device_func, fini_device_func, free_func, dev2host_func, >> host2dev_func, and dev2dev_func plugin hooks from 'void *' to >> bool. >> * oacc-host.c (host_init_device): Change return type to bool. >> (host_fini_device): Likewise. >> (host_free): Likewise. >> (host_dev2host): Likewise. >> (host_host2dev): Likewise. >> * oacc-mem.c (acc_free): Handle plugin hook fatal error case. >> (acc_memcpy_to_device): Likewise. >> (acc_memcpy_from_device): Likewise. >> (delete_copyout): Add libfnname parameter, handle free_func >> hook fatal error case. >> (acc_delete): Adjust delete_copyout call. >> (acc_copyout): Likewise. >> >> >> > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 1/3, libgomp] Resolve libgomp plugin deadlock on exit, libgomp proper parts 2016-01-19 6:02 ` Chung-Lin Tang @ 2016-01-22 10:31 ` Jakub Jelinek 0 siblings, 0 replies; 4+ messages in thread From: Jakub Jelinek @ 2016-01-22 10:31 UTC (permalink / raw) To: Chung-Lin Tang; +Cc: Nathan Sidwell, Thomas Schwinge, Verbin, Ilya, gcc-patches On Tue, Jan 19, 2016 at 03:02:48PM +0900, Chung-Lin Tang wrote: > Ping x 2. I like the general idea of moving the gomp_fatal stuff up, out of the plugins, but we now have another plugin, so it can't apply as is. And in the intelmic plugin, adding return true; after abort (); looks wrong. So, can you please update this series and repost? Jakub ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2016-01-22 10:31 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <566EC2F4.1030109@codesourcery.com>
2015-12-14 15:47 ` [PATCH 1/3, libgomp] Resolve libgomp plugin deadlock on exit, libgomp proper parts Chung-Lin Tang
2016-01-05 14:22 ` Chung-Lin Tang
2016-01-19 6:02 ` Chung-Lin Tang
2016-01-22 10:31 ` Jakub Jelinek
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).