Hi Jakub, Julian! Can you please help me understand the following:
On 2018-06-19T10:01:20-0700, Cesar Philippidis <ce...@codesourcery.com> wrote: > This patch implements the OpenACC 2.5 data clause semantics in libgomp. (This got committed as r261813, 2018-06-20. The code has seen some changes in the mean time, but the underlying issue remains.) > --- a/libgomp/oacc-mem.c > +++ b/libgomp/oacc-mem.c > @@ -347,6 +347,7 @@ acc_map_data (void *h, void *d, size_t s) > > tgt = gomp_map_vars (acc_dev, mapnum, &hostaddrs, &devaddrs, &sizes, > &kinds, true, GOMP_MAP_VARS_OPENACC); > + tgt->list[0].key->refcount = REFCOUNT_INFINITY; > } > > gomp_mutex_lock (&acc_dev->lock); Without 'acc_dev->lock' locked, we here touch the 'refcount' (via 'tgt->list[0].key->refcount'), as returned from 'gomp_map_vars' in the case when entering a new mapping. > @@ -483,6 +492,8 @@ present_create_copy (unsigned f, void *h, size_t s) > > tgt = gomp_map_vars (acc_dev, mapnum, &hostaddrs, NULL, &s, &kinds, > true, > GOMP_MAP_VARS_OPENACC); > + /* Initialize dynamic refcount. */ > + tgt->list[0].key->dynamic_refcount = 1; > > gomp_mutex_lock (&acc_dev->lock); Likewise here, for the 'dynamic_refcount' (via 'tgt->list[0].key->dynamic_refcount'), as returned from 'gomp_map_vars' in the case when entering a new mapping. By construction, it is safe to assume that 'tgt->list[0].key' is the 'n' we're looking to modify: this is the case where we're entering a new mapping. But: is it safe to access this unlocked? It may seem so, as the new mapping has not yet been exposed to user code, so only exists internal in the respective libgomp functions. Yet, could still a concurrent 'acc_unmap_data'/'acc_delete'/etc. already "see" it (that is, look it up, as it already has been entered into the mapping table), and unmap while we're accessing 'tgt->list[0].key' here? (Hmm, and actually a similar issue, if we consider the case of two 'gomp_map_vars' running concurrently?) In that case, I suppose we should change the 'gomp_map_vars' interface and all callers so that 'gomp_map_vars' always takes the device locked. That doesn't appear problematic: locking the device is one of the first things 'gomp_map_vars' does anyway (just not in the 'mapnum == 0' case, but I suppose it's OK to pessimize that one?), and it remains locked until the end of the function. Grüße Thomas ----------------- Mentor Graphics (Deutschland) GmbH, Arnulfstraße 201, 80634 München / Germany Registergericht München HRB 106955, Geschäftsführer: Thomas Heurung, Alexander Walter
