OpenACC reference count consistency checking

This patch provides self-checking for consistency of the OpenACC
reference-counting implementation in libgomp.  Tested alongside (and
dependent on) the patch posted adjacent to this one that overhauls said
reference-counting implementation.

Tested (with RC_CHECKING enabled) with offloading to NVPTX. OK for trunk?

Thanks,

Julian

2019-10-02  Julian Brown  <julian@codesourcery.com>

	libgomp/
	* libgomp.h (RC_CHECKING): New macro, disabled by default, guarding all
	hunks in this patch.
	(target_mem_desc): Add refcount_chk, mark fields.
	(splay_tree_key_s): Add refcount_chk field.
	(dump_tgt, gomp_rc_check): Add prototypes.
	* oacc-parallel.c (GOACC_parallel_keyed): Add refcount self-check code.
	(GOACC_data_start, GOACC_data_end, GOACC_enter_exit_data): Likewise.
	* target.c (stdio.h): Include.
	(dump_tgt, rc_check_clear, rc_check_count, rc_check_verify,
	gomp_rc_check): New functions to consistency-check reference counts.
---
 libgomp/libgomp.h       |  18 ++++
 libgomp/oacc-parallel.c |  33 ++++++++
 libgomp/target.c        | 178 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 229 insertions(+)

Hi!

On 2019-10-03T09:35:05-0700, Julian Brown <julian@codesourcery.com> wrote:
> This patch provides self-checking for consistency of the OpenACC
> reference-counting implementation in libgomp.

Earlier submissions included description what exactly this is doing,
<http://mid.mail-archive.com/1543578624-1511-1-git-send-email-julian@codesourcery.com>.
Will be good to get that into the libgomp manual -- or into the code as
comments, given this is an internal-only thing?

> Tested alongside (and
> dependent on) the patch posted adjacent to this one that overhauls said
> reference-counting implementation.

(I'm reviewing that one.)

> Tested (with RC_CHECKING enabled) with offloading to NVPTX. OK for trunk?

So, for this patch, we'll first need to agree on what approach we
generally want to take.  (Jakub?)

Adding "dead" code ('#ifdef RC_CHECKING', off by default) is not a good
thing for maintenance, might easily become out of date, etc.  On the
other hand, I do understand that the functionality is useful --
supposedly not just for us developers, but also for users, to detect
wrong OpenACC data/region directives, API calls, etc.?

In a different context (OpenMP 5 OMPT), the option was mentioned to have
"a variant library that is a replacement for libgomp if more detailed
instrumentation is needed".  (That shouldn't be difficult to implement
given the Automake build system.)  Is that an option for this thing here,
too?  That is, an ABI-compatible "debugging" libgomp, with more run-time
consistency checking etc., with this one here being a start, and more can
be added later?


> 2019-10-02  Julian Brown  <julian@codesourcery.com>
>
> 	libgomp/
> 	* libgomp.h (RC_CHECKING): New macro, disabled by default, guarding all
> 	hunks in this patch.
> 	(target_mem_desc): Add refcount_chk, mark fields.
> 	(splay_tree_key_s): Add refcount_chk field.
> 	(dump_tgt, gomp_rc_check): Add prototypes.
> 	* oacc-parallel.c (GOACC_parallel_keyed): Add refcount self-check code.
> 	(GOACC_data_start, GOACC_data_end, GOACC_enter_exit_data): Likewise.
> 	* target.c (stdio.h): Include.
> 	(dump_tgt, rc_check_clear, rc_check_count, rc_check_verify,
> 	gomp_rc_check): New functions to consistency-check reference counts.
> ---
>  libgomp/libgomp.h       |  18 ++++
>  libgomp/oacc-parallel.c |  33 ++++++++
>  libgomp/target.c        | 178 ++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 229 insertions(+)

Just to make sure: earlier versions submitted (see link above) included
more changes, for example to "async" code, a new 'struct async_tgt_use'
etc., which is no longer present here.  Is that because that's no longer
needed given the "recent" "Async-rework"?

> diff --git a/libgomp/libgomp.h b/libgomp/libgomp.h
> index 6b7ed7248a1..19553a37c13 100644
> --- a/libgomp/libgomp.h
> +++ b/libgomp/libgomp.h
> @@ -873,9 +873,17 @@ struct target_var_desc {
>    uintptr_t length;
>  };
>  
> +/* Uncomment to enable reference-count consistency checking (for development
> +   use only).  */
> +//#define RC_CHECKING 1
> +
>  struct target_mem_desc {
>    /* Reference count.  */
>    uintptr_t refcount;
> +#ifdef RC_CHECKING
> +  uintptr_t refcount_chk;
> +  bool mark;
> +#endif
>    /* All the splay nodes allocated together.  */
>    splay_tree_node array;
>    /* Start of the target region.  */
> @@ -930,6 +938,10 @@ struct splay_tree_key_s {
>       If set to VREFCOUNT_LINK_KEY (for OpenMP, where this field is not otherwise
>       needed), the union below represents a link key.  */
>    uintptr_t virtual_refcount;
> +#ifdef RC_CHECKING
> +  /* The recalculated reference count, for verification.  */
> +  uintptr_t refcount_chk;
> +#endif
>    union {
>      /* Pointer to the original mapping of "omp declare target link" object.
>         Only used for OpenMP.  */
> @@ -1084,6 +1096,12 @@ extern void gomp_copy_dev2host (struct gomp_device_descr *,
>  				struct goacc_asyncqueue *, void *, const void *,
>  				size_t);
>  
> +#ifdef RC_CHECKING
> +extern void dump_tgt (const char *, struct target_mem_desc *);
> +extern void gomp_rc_check (struct gomp_device_descr *,
> +			   struct target_mem_desc *);
> +#endif
> +
>  extern struct target_mem_desc *gomp_map_vars (struct gomp_device_descr *,
>  					      size_t, void **, void **,
>  					      size_t *, void *, bool,
> diff --git a/libgomp/oacc-parallel.c b/libgomp/oacc-parallel.c
> index 7e72d9c6b24..18feac5f31c 100644
> --- a/libgomp/oacc-parallel.c
> +++ b/libgomp/oacc-parallel.c
> @@ -339,6 +339,15 @@ GOACC_parallel_keyed (int flags_m, void (*fn) (void *),
>  				&api_info);
>      }
>    
> +#ifdef RC_CHECKING
> +  gomp_mutex_lock (&acc_dev->lock);
> +  assert (tgt);
> +  dump_tgt (__FUNCTION__, tgt);
> +  tgt->prev = thr->mapped_data;
> +  gomp_rc_check (acc_dev, tgt);
> +  gomp_mutex_unlock (&acc_dev->lock);
> +#endif
> +
>    devaddrs = gomp_alloca (sizeof (void *) * mapnum);
>    for (i = 0; i < mapnum; i++)
>      if (tgt->list[i].key != NULL)
> @@ -389,6 +398,12 @@ GOACC_parallel_keyed (int flags_m, void (*fn) (void *),
>        thr->prof_info = NULL;
>        thr->api_info = NULL;
>      }
> +
> +#ifdef RC_CHECKING
> +  gomp_mutex_lock (&acc_dev->lock);
> +  gomp_rc_check (acc_dev, thr->mapped_data);
> +  gomp_mutex_unlock (&acc_dev->lock);
> +#endif
>  }
>  
>  /* Legacy entry point (GCC 5).  Only provide host fallback execution.  */
> @@ -522,6 +537,12 @@ GOACC_data_start (int flags_m, size_t mapnum,
>        thr->prof_info = NULL;
>        thr->api_info = NULL;
>      }
> +
> +#ifdef RC_CHECKING
> +  gomp_mutex_lock (&acc_dev->lock);
> +  gomp_rc_check (acc_dev, thr->mapped_data);
> +  gomp_mutex_unlock (&acc_dev->lock);
> +#endif
>  }
>  
>  void
> @@ -595,6 +616,12 @@ GOACC_data_end (void)
>        thr->prof_info = NULL;
>        thr->api_info = NULL;
>      }
> +
> +#ifdef RC_CHECKING
> +  gomp_mutex_lock (&thr->dev->lock);
> +  gomp_rc_check (thr->dev, thr->mapped_data);
> +  gomp_mutex_unlock (&thr->dev->lock);
> +#endif
>  }
>  
>  void
> @@ -832,6 +859,12 @@ GOACC_enter_exit_data (int flags_m, size_t mapnum,
>        thr->prof_info = NULL;
>        thr->api_info = NULL;
>      }
> +
> +#ifdef RC_CHECKING
> +  gomp_mutex_lock (&acc_dev->lock);
> +  gomp_rc_check (acc_dev, thr->mapped_data);
> +  gomp_mutex_unlock (&acc_dev->lock);
> +#endif
>  }
>  
>  static void
> diff --git a/libgomp/target.c b/libgomp/target.c
> index b42b4ad2448..29cb7ca8348 100644
> --- a/libgomp/target.c
> +++ b/libgomp/target.c
> @@ -38,6 +38,9 @@
>  #include <string.h>
>  #include <assert.h>
>  #include <errno.h>
> +#ifdef RC_CHECKING
> +#include <stdio.h>
> +#endif
>  
>  #ifdef PLUGIN_SUPPORT
>  #include <dlfcn.h>
> @@ -346,6 +349,181 @@ gomp_free_device_memory (struct gomp_device_descr *devicep, void *devptr)
>      }
>  }
>  
> +#ifdef RC_CHECKING
> +void
> +dump_tgt (const char *where, struct target_mem_desc *tgt)
> +{
> +  if (!getenv ("GOMP_DEBUG_TGT"))
> +    return;
> +
> +  fprintf (stderr, "%s: %s: tgt=%p\n", __FUNCTION__, where, (void*) tgt);
> +  fprintf (stderr, "refcount=%d\n", (int) tgt->refcount);
> +  fprintf (stderr, "tgt_start=%p\n", (void*) tgt->tgt_start);
> +  fprintf (stderr, "tgt_end=%p\n", (void*) tgt->tgt_end);
> +  fprintf (stderr, "to_free=%p\n", tgt->to_free);
> +  fprintf (stderr, "list_count=%d\n", (int) tgt->list_count);
> +  for (int i = 0; i < tgt->list_count; i++)
> +    {
> +      fprintf (stderr, "list item %d:\n", i);
> +      fprintf (stderr, "  key: %p\n", (void*) tgt->list[i].key);
> +      if (tgt->list[i].key)
> +	{
> +	  fprintf (stderr, "  key.host_start=%p\n",
> +		   (void*) tgt->list[i].key->host_start);
> +	  fprintf (stderr, "  key.host_end=%p\n",
> +		   (void*) tgt->list[i].key->host_end);
> +	  fprintf (stderr, "  key.tgt=%p\n", (void*) tgt->list[i].key->tgt);
> +	  fprintf (stderr, "  key.offset=%d\n",
> +		   (int) tgt->list[i].key->tgt_offset);
> +	  fprintf (stderr, "  key.refcount=%d\n",
> +		   (int) tgt->list[i].key->refcount);
> +	  if (tgt->list[i].key->virtual_refcount == VREFCOUNT_LINK_KEY)
> +	    fprintf (stderr, "  key.u.link_key=%p\n",
> +		     (void*) tgt->list[i].key->u.link_key);
> +	  else
> +	    fprintf (stderr, "  key.virtual_refcount=%d\n",
> +		     (int) tgt->list[i].key->virtual_refcount);
> +	}
> +    }
> +  fprintf (stderr, "\n");
> +}
> +
> +static void
> +rc_check_clear (splay_tree_node node)
> +{
> +  splay_tree_key k = &node->key;
> +
> +  k->refcount_chk = 0;
> +  k->tgt->refcount_chk = 0;
> +  k->tgt->mark = false;
> +
> +  if (node->left)
> +    rc_check_clear (node->left);
> +  if (node->right)
> +    rc_check_clear (node->right);
> +}
> +
> +static void
> +rc_check_count (splay_tree_node node)
> +{
> +  splay_tree_key k = &node->key;
> +  struct target_mem_desc *t;
> +
> +  /* Add virtual reference counts ("acc enter data", etc.) for this key.  */
> +  k->refcount_chk += k->virtual_refcount;
> +
> +  t = k->tgt;
> +  t->refcount_chk++;
> +
> +  if (!t->mark)
> +    {
> +      for (int i = 0; i < t->list_count; i++)
> +	if (t->list[i].key)
> +	  t->list[i].key->refcount_chk++;
> +
> +      t->mark = true;
> +    }
> +
> +  if (node->left)
> +    rc_check_count (node->left);
> +  if (node->right)
> +    rc_check_count (node->right);
> +}
> +
> +static bool
> +rc_check_verify (splay_tree_node node, bool noisy, bool errors)
> +{
> +  splay_tree_key k = &node->key;
> +  struct target_mem_desc *t;
> +
> +  if (k->refcount != REFCOUNT_INFINITY)
> +    {
> +      if (noisy)
> +	fprintf (stderr, "key %p (%p..+%d): rc=%d/%d, virt_rc=%d\n", k,
> +		 (void *) k->host_start, (int) (k->host_end - k->host_start),
> +		 (int) k->refcount, (int) k->refcount_chk,
> +		 (int) k->virtual_refcount);
> +
> +      if (k->refcount != k->refcount_chk)
> +	{
> +	  if (noisy)
> +	    fprintf (stderr, "  -- key refcount mismatch!\n");
> +	  errors = true;
> +	}
> +
> +      t = k->tgt;
> +
> +      if (noisy)
> +	fprintf (stderr, "tgt %p: rc=%d/%d\n", t, (int) t->refcount,
> +		 (int) t->refcount_chk);
> +
> +      if (t->refcount != t->refcount_chk)
> +	{
> +	  if (noisy)
> +	    fprintf (stderr,
> +		     "  -- target memory descriptor refcount mismatch!\n");
> +	  errors = true;
> +	}
> +    }
> +
> +  if (node->left)
> +    errors |= rc_check_verify (node->left, noisy, errors);
> +  if (node->right)
> +    errors |= rc_check_verify (node->right, noisy, errors);
> +
> +  return errors;
> +}
> +
> +/* Call with device locked.  */
> +
> +attribute_hidden void
> +gomp_rc_check (struct gomp_device_descr *devicep, struct target_mem_desc *tgt)
> +{
> +  splay_tree sp = &devicep->mem_map;
> +
> +  bool noisy = getenv ("GOMP_DEBUG_TGT") != 0;
> +
> +  if (noisy)
> +    fprintf (stderr, "\n*** GOMP_RC_CHECK ***\n\n");
> +
> +  if (sp->root)
> +    {
> +      rc_check_clear (sp->root);
> +
> +      for (struct target_mem_desc *t = tgt; t; t = t->prev)
> +	{
> +	  t->refcount_chk = 0;
> +	  t->mark = false;
> +	}
> +
> +      /* Add references for interconnected splay-tree keys.  */
> +      rc_check_count (sp->root);
> +
> +      /* Add references for the tgt for a currently-executing kernel and/or
> +	 any enclosing data directives.  */
> +      for (struct target_mem_desc *t = tgt; t; t = t->prev)
> +	{
> +	  t->refcount_chk++;
> +
> +	  if (!t->mark)
> +	    {
> +	      for (int i = 0; i < t->list_count; i++)
> +		if (t->list[i].key)
> +		  t->list[i].key->refcount_chk++;
> +
> +	      t->mark = true;
> +	    }
> +	}
> +
> +      if (rc_check_verify (sp->root, noisy, false))
> +	{
> +	  gomp_mutex_unlock (&devicep->lock);
> +	  gomp_fatal ("refcount checking failure");
> +	}
> +    }
> +}
> +#endif
> +
>  /* 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.  */


Grüße
 Thomas