RE: [PATCH] tools/xentop: Add physical CPU statistics support

[Jahan Murudi <jahan.murudi.zg@xxxxxxxxxxx>]

Hi Andriy,

Thank you for your review.

>On 08/07/ 2025 18:26, Andriy Sultanov wrote:


> > diff --git a/tools/xentop/xentop.c b/tools/xentop/xentop.c  > index 
> > f5d6c19cf9..477299c883 100644  > --- a/tools/xentop/xentop.c  > +++ 
> > b/tools/xentop/xentop.c  > @@ -69,6 +70,12 @@  >  >  #define 
> > INT_FIELD_WIDTH(n) ((unsigned int)(log10(n) + 1))  >  > +/* TEMPORARY: 
> > Forward declare the internal structure */  > +struct xenstat_handle {  > +  
> >   xc_interface *xc_handle;  > +    /* Other members don't matter fo now */  
> > > +};  > +

> What makes this temporary? Is there a follow-up patch?

This was intended as a short-term solution to access the xc_handle. Latter may 
be move this to a shared header if multiple tools need access.

> Or should this be an [RFC] instead of a [PATCH]?

You're right - I'll resubmit this as an RFC patch.

> > @@ -240,6 +248,7 @@ static void usage(const char *program)  >             
> > "-r, --repeat-header  repeat table header before each domain\n"
 > >             "-v, --vcpus          output vcpu data\n"
> >             "-b, --batch         output in batch mode, no user input 
> >accepted\n"
> > +           "-p, --pcpus         show physical CPU stats\n"
> >             "-i, --iterations     number of iterations before exiting\n"
> >             "-f, --full-name      output the full domain name (not 
> >truncated)\n"
> >             "-z, --dom0-first     display dom0 first (ignore sorting)\n"

> Incorrect indentation here
You're correct. I'll fix both the -b and -p options to maintain consistent 
alignment with the other options in the v2 RFC patch

 >> @@ -1245,9 +1256,18 @@ static void top(void)  >              
 >> do_vbd(domains[i]);  >      }  >  > -    if (!batch)  > +    if (!batch && 
 >> !show_pcpus )  >          do_bottom_line();  >  > +    if (show_pcpus && 
 >> xhandle != NULL ) {  > +    if (update_pcpu_stats(xhandle->xc_handle) == 0) 
 >> {  > +        print_pcpu_stats();  > +    }  > +    else {  > +        
 >> print("Error getting PCPU stats\n");  > +    }  > +   }  > +

> and here
Good catch on the indentation issues.

Regards,
Jahan