Re: [Xen-devel] [PATCH RFC 1/4] tmem: Move global stats in a tmem_statistics structure

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

On Tue, May 17, 2016 at 09:01:00PM -0500, Doug Goldstein wrote:
> On 5/17/16 8:57 PM, Doug Goldstein wrote:
> > On 5/16/16 10:58 AM, Konrad Rzeszutek Wilk wrote:
> >> And adjust the macro: atomic_inc_and_max to update the structure.
> >>
> >> Sadly one entry: pool->pgp_count cannot use this macro anymore
> >> so unroll the macro for this instance.
> >>
> >> No functional change. The name has the 'tmem_stats' as it will
> >> be eventually non-local.
> >>
> >> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> >> ---
> >>  xen/common/tmem.c | 135 
> >> +++++++++++++++++++++++++++++-------------------------
> >>  1 file changed, 73 insertions(+), 62 deletions(-)
> >>
> >> diff --git a/xen/common/tmem.c b/xen/common/tmem.c
> >> index 16e249a..d362eae 100644
> >> --- a/xen/common/tmem.c
> >> +++ b/xen/common/tmem.c
> >> @@ -28,25 +28,32 @@
> >>  #define TMEM_SPEC_VERSION 1
> >>  
> >>  /* Global statistics (none need to be locked). */
> >> -static unsigned long total_tmem_ops = 0;
> >> -static unsigned long errored_tmem_ops = 0;
> >> -static unsigned long total_flush_pool = 0;
> >> -static unsigned long alloc_failed = 0, alloc_page_failed = 0;
> >> -static unsigned long evicted_pgs = 0, evict_attempts = 0;
> >> -static unsigned long relinq_pgs = 0, relinq_attempts = 0;
> >> -static unsigned long max_evicts_per_relinq = 0;
> >> -static unsigned long low_on_memory = 0;
> >> -static unsigned long deduped_puts = 0;
> >> -static unsigned long tot_good_eph_puts = 0;
> >> -static int global_obj_count_max = 0;
> >> -static int global_pgp_count_max = 0;
> >> -static int global_pcd_count_max = 0;
> >> -static int global_page_count_max = 0;
> >> -static int global_rtree_node_count_max = 0;
> >> -static long global_eph_count_max = 0;
> >> -static unsigned long failed_copies;
> >> -static unsigned long pcd_tot_tze_size = 0;
> >> -static unsigned long pcd_tot_csize = 0;
> >> +struct tmem_statistics {
> >> +    unsigned long total_tmem_ops;
> >> +    unsigned long errored_tmem_ops;
> >> +    unsigned long total_flush_pool;
> >> +    unsigned long alloc_failed;
> >> +    unsigned long alloc_page_failed;
> >> +    unsigned long evicted_pgs;
> >> +    unsigned long evict_attempts;
> >> +    unsigned long relinq_pgs;
> >> +    unsigned long relinq_attempts;
> >> +    unsigned long max_evicts_per_relinq;
> >> +    unsigned long low_on_memory;
> >> +    unsigned long deduped_puts;
> >> +    unsigned long tot_good_eph_puts;
> >> +    int global_obj_count_max;
> >> +    int global_pgp_count_max;
> >> +    int global_pcd_count_max;
> >> +    int global_page_count_max;
> >> +    int global_rtree_node_count_max;
> >> +    long global_eph_count_max;
> >> +    unsigned long failed_copies;
> >> +    unsigned long pcd_tot_tze_size;
> >> +    unsigned long pcd_tot_csize;
> >> +};
> >> +
> >> +static struct tmem_statistics tmem_stats;
> >>  
> >>  /************ CORE DATA STRUCTURES ************************************/
> >>  
> >> @@ -225,8 +232,8 @@ static atomic_t global_rtree_node_count = 
> >> ATOMIC_INIT(0);
> >>  
> >>  #define atomic_inc_and_max(_c) do { \
> >>      atomic_inc(&_c); \
> >> -    if ( _atomic_read(_c) > _c##_max ) \
> >> -        _c##_max = _atomic_read(_c); \
> >> +    if ( _atomic_read(_c) > tmem_stats._c##_max ) \
> >> +        tmem_stats._c##_max = _atomic_read(_c); \
> >>  } while (0)
> >>  
> >>  #define atomic_dec_and_assert(_c) do { \
> >> @@ -273,7 +280,7 @@ static void *tmem_malloc(size_t size, struct tmem_pool 
> >> *pool)
> >>          v = xmem_pool_alloc(size, tmem_mempool);
> >>      }
> >>      if ( v == NULL )
> >> -        alloc_failed++;
> >> +        tmem_stats.alloc_failed++;
> >>      return v;
> >>  }
> >>  
> >> @@ -300,7 +307,7 @@ static struct page_info *tmem_alloc_page(struct 
> >> tmem_pool *pool)
> >>      else
> >>          pfp = __tmem_alloc_page();
> >>      if ( pfp == NULL )
> >> -        alloc_page_failed++;
> >> +        tmem_stats.alloc_page_failed++;
> >>      else
> >>          atomic_inc_and_max(global_page_count);
> > 
> > So the code was previously like this but this change made me notice
> > this. How come tmem_stats.global_page_count needs to be atomically
> > incremented but not tmem_stats.alloc_page_failed? Its also a little
> > weird that global_page_count is in the struct now but not really visible
> > here while alloc_page_count is in the struct but has to be explicitly
> > called out.
> > 
> > 
> 
> Actually I just realized "global_page_count" but this patch actually
> deals with "global_page_count_max". So ignore the original email.
> 
> But does this patch compile (for bisect-ability) due to the change to
> atomic_inc_and_max() but not moving "global_page_count" into the structure?

Yup:

 #define atomic_inc_and_max(_c) do { \
     atomic_inc(&_c); \
-    if ( _atomic_read(_c) > _c##_max ) \
-        _c##_max = _atomic_read(_c); \
+    if ( _atomic_read(_c) > tmem_stats._c##_max ) \
+        tmem_stats._c##_max = _atomic_read(_c); \
 } while (0)
 
As the global_page_count is still an atomic outside the structure.
And the global_page_count_max is inside the structure.

> 
> -- 
> Doug Goldstein
> 




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