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

[Doug Goldstein <cardoe@xxxxxxxxxx>]

On 5/18/16 9:34 AM, Konrad Rzeszutek Wilk wrote:
> 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
>>
> 
> 
> 

Gah. You're right.

Reviewed-by: Doug Goldstein <cardoe@xxxxxxxxxx>

-- 
Doug Goldstein

Attachment: signature.asc
Description: OpenPGP digital signature

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