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

[Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>]

On Thu, Jun 02, 2016 at 04:16:51PM +0100, Andrew Cooper wrote:
> 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.

Yes.
> 
> 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.

Subsequent. There were sooo many things I itched to do - but for right
now just want to do this simple cleanup.
> 
> ~Andrew

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