Re: [PATCH 3/3] xenpm: use new Cx statistics interface

[Boris Ostrovsky <boris.ostrovsky@oracle.com>]

On 03/05/2014 10:53 AM, Jan Beulich wrote:
>>>> On 05.03.14 at 16:47, Boris Ostrovsky <boris.ostrovsky@oracle.com> wrote:
>> On 03/05/2014 05:37 AM, Jan Beulich wrote:
>>> @@ -331,7 +346,7 @@ void pxstat_func(int argc, char *argv[])
>>>    }
>>>    
>>>    static uint64_t usec_start, usec_end;
>>> -static struct xc_cx_stat *cxstat, *cxstat_start, *cxstat_end;
>>> +static struct xc_cx_stat_v2 *cxstat, *cxstat_start, *cxstat_end;
>>>    static struct xc_px_stat *pxstat, *pxstat_start, *pxstat_end;
>>>    static int *avgfreq;
>>>    static uint64_t *sum, *sum_cx, *sum_px;
>>> @@ -482,25 +497,26 @@ static void signal_int_handler(int signo
>>>                /* print out CC? and PC? */
>>>                for ( i = 0; i < socket_nr; i++ )
>>>                {
>>> +                unsigned int n;
>>>                    uint64_t res;
>>> +
>>>                    for ( j = 0; j <= info.max_cpu_index; j++ )
>>>                    {
>>>                        if ( cpu_to_socket[j] == socket_ids[i] )
>>>                            break;
>>>                    }
>>>                    printf("\nSocket %d\n", socket_ids[i]);
>>> -                res = cxstat_end[j].pc2 - cxstat_start[j].pc2;
>>> -                printf("\tPC2\t%"PRIu64" ms\t%.2f%%\n",  res / 1000000UL,
>>> -                       100UL * res / (double)sum_cx[j]);
>>> -                res = cxstat_end[j].pc3 - cxstat_start[j].pc3;
>>> -                printf("\tPC3\t%"PRIu64" ms\t%.2f%%\n",  res / 1000000UL,
>>> -                       100UL * res / (double)sum_cx[j]);
>>> -                res = cxstat_end[j].pc6 - cxstat_start[j].pc6;
>>> -                printf("\tPC6\t%"PRIu64" ms\t%.2f%%\n",  res / 1000000UL,
>>> -                       100UL * res / (double)sum_cx[j]);
>>> -                res = cxstat_end[j].pc7 - cxstat_start[j].pc7;
>>> -                printf("\tPC7\t%"PRIu64" ms\t%.2f%%\n",  res / 1000000UL,
>>> -                       100UL * res / (double)sum_cx[j]);
>>> +                for ( n = 0; n < MAX_PKG_RESIDENCIES; ++n )
>>> +                {
>>> +                    if ( n >= cxstat_end[j].nr_pc )
>>> +                        continue;
>>> +                    res = cxstat_end[j].pc[n];
>>> +                    if ( n < cxstat_start[j].nr_pc )
>>> +                        res -= cxstat_start[j].pc[n];
>> Is it possible to have  cxstat_end[j].nr_pc != cxstat_start[j].nr_pc ?
> Yes - see the previous patch: It bumps the count only if the
> respective hw_res field was non-zero.

You mean this?

+
+#define PUT_xC(what, n) do { \
+        if ( stat->nr_##what >= n && \
+             copy_to_guest_offset(stat->what, n - 1, &hw_res.what##n, 1) ) \
+            return -EFAULT; \
+        if ( hw_res.what##n ) \
+            nr_##what = n; \
+    } while ( 0 )
+#define PUT_PC(n) PUT_xC(pc, n)

This reminds me of another question I had about this patch: this 
fragment appears to assume that you call it in order. In other words, 
will it work as intended if your call sequence is

PUT_PC(10)
..
PUT_PC(1)

-boris


>
> But even if the current implementation didn't allow for this, I'd
> still consider it good practice to cope with the possibility.
>