Re: [PATCH 3/7] tools: Refactor the console/io.c to avoid using xc_domain_getinfo()

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 26/04/2023 3:59 pm, Alejandro Vallejo wrote:
> It has 2 avoidable occurences
>
> * Check whether a domain is valid, which can be done faster with
>     xc_domain_getinfo_single()
> * Domain discovery, which can be done much faster with the sysctl
>     interface through xc_domain_getinfolist().
>
> Signed-off-by: Alejandro Vallejo <alejandro.vallejo@xxxxxxxxx>
> ---
> Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Cc: Wei Liu <wl@xxxxxxx>
> Cc: Anthony PERARD <anthony.perard@xxxxxxxxxx>
> ---
>  tools/console/daemon/io.c | 31 +++++++++++++++++--------------
>  1 file changed, 17 insertions(+), 14 deletions(-)
>
> diff --git a/tools/console/daemon/io.c b/tools/console/daemon/io.c
> index 6bfe96715b..1fc56f6643 100644
> --- a/tools/console/daemon/io.c
> +++ b/tools/console/daemon/io.c
> @@ -405,13 +405,7 @@ static void buffer_advance(struct buffer *buffer, size_t 
> len)
>  
>  static bool domain_is_valid(int domid)
>  {
> -     bool ret;
> -     xc_dominfo_t info;
> -
> -     ret = (xc_domain_getinfo(xc, domid, 1, &info) == 1 &&
> -            info.domid == domid);
> -             
> -     return ret;
> +     return xc_domain_getinfo_single(xc, domid, NULL) == 0;
>  }
>  
>  static int create_hv_log(void)
> @@ -959,26 +953,35 @@ static void shutdown_domain(struct domain *d)
>  
>  static unsigned enum_pass = 0;
>  
> +/**
> + * Memory set aside to query the state of every
> + * domain in the hypervisor in a single hypercall.
> + */
> +static xc_domaininfo_t domaininfo[DOMID_FIRST_RESERVED - 1];
> +

We prefer to reduce scope where possible, and in this case it's fine to
have this declared inside enum_domains().  Preferred style for that
would be:

static void enum_domains(void)
{
    /**
     * Memory set aside to query the state of every
     * domain in the hypervisor in a single hypercall.
     */
    static xc_domaininfo_t domaininfo[DOMID_FIRST_RESERVED - 1];

    int ret;
    struct domain *dom;


i.e. one blank line between the static and local variable declarations.

>  static void enum_domains(void)
>  {
> -     int domid = 1;
> -     xc_dominfo_t dominfo;
> +     int ret;
>       struct domain *dom;
>  
>       enum_pass++;
>  
> -     while (xc_domain_getinfo(xc, domid, 1, &dominfo) == 1) {
> -             dom = lookup_domain(dominfo.domid);
> -             if (dominfo.dying) {
> +     /* Fetch info on every valid domain except for dom0 */
> +     ret = xc_domain_getinfolist(xc, 1, DOMID_FIRST_RESERVED - 1, 
> domaininfo);

This is a correct translation of the prior logic.

But it does highlight that xenconsoled currently depends on running in
dom0.  I bet this is going to be fun bug for someone down the line...

Also, while going for 32k entries is absolutely the right thing to do,
the entire buffer will be bounced twice to make it hypercall safe. 
Bordering on 4M of data, I think this is quickly going to become a
second improvement to work on.

~Andrew