Re: [PATCH v1 2/4] tmem: Wrap atomic_t in struct tmem_statistics as well.

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 02/06/16 16:04, Konrad Rzeszutek Wilk wrote:
> diff --git a/xen/common/tmem.c b/xen/common/tmem.c
> index d362eae..b58ab4d 100644
> --- a/xen/common/tmem.c
> +++ b/xen/common/tmem.c
> @@ -51,9 +51,32 @@ struct tmem_statistics {
>      unsigned long failed_copies;
>      unsigned long pcd_tot_tze_size;
>      unsigned long pcd_tot_csize;
> +    /* Global counters (should use long_atomic_t access). */
> +    atomic_t global_obj_count;
> +    atomic_t global_pgp_count;
> +    atomic_t global_pcd_count;
> +    atomic_t global_page_count;
> +    atomic_t global_rtree_node_count;
>  };
>  
> -static struct tmem_statistics tmem_stats;
> +#define atomic_inc_and_max(_c) do { \
> +    atomic_inc(&tmem_stats._c); \
> +    if ( _atomic_read(tmem_stats._c) > tmem_stats._c##_max ) \
> +        tmem_stats._c##_max = _atomic_read(tmem_stats._c); \
> +} while (0)
> +
> +#define atomic_dec_and_assert(_c) do { \
> +    atomic_dec(&tmem_stats._c); \
> +    ASSERT(_atomic_read(tmem_stats._c) >= 0); \
> +} while (0)

I know you are just modifying existing code (thus this complain doesn't
count against the patch), but these constructs are racy and broken.

You need to use atomic_{inc,dec}_and_test() for these to function as
intended.

Up to you whether you want to fix that in this patch, or a subsequent
non-cleanup patch.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel