Re: [PATCH v5 02/10] kvm: Support for querying fd-based stats

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* Paolo Bonzini (pbonzini@redhat.com) wrote:
> On 6/7/22 19:44, Dr. David Alan Gilbert wrote:
> 
> > > +        return NULL;
> > > +    }
> > > +    descriptors->kvm_stats_header = kvm_stats_header;
> > > +    descriptors->kvm_stats_desc = kvm_stats_desc;
> > > +    descriptors->ident = g_strdup(ident);
> > 
> > There's something that confuses me here; you check your set of
> > descriptors above to find any with the matching ident, and if you've
> > already got it you return it; OK.  Now, if you don't match then you
> > read some stats and store it with that ident - but I don't see
> > when you read the stats from the fd, what makes it read the stats that
> > correspond to 'ident' ?
> 
> If you mean why not some other source, each source has a different file
> descriptor:
> 
> +    int stats_fd = kvm_vcpu_ioctl(cpu, KVM_GET_STATS_FD, NULL);
> 
> but the descriptors are consistent every time KVM_GET_STATS_FD is called, so
> basically "ident" can be used as a cache key.

Ah OK, this is what I was after; it's a little weird that the caller
does the ioctl to get the stats-fd, but it does the lookup internally
with current_cpu for the ident.

Some comments would help!

Dave

> If you mean how does it access the right stat, here it uses the offset
> field in the descriptor
> 
>     ret = pread(stats_fd, stats_data, size_data,
> kvm_stats_header->data_offset);
>     ...
>     for (i = 0; i < kvm_stats_header->num_desc; ++i) {
>         uint64_t *stats;
>         pdesc = (void *)kvm_stats_desc + i * size_desc;
> 
>         /* Add entry to the list */
>         stats = (void *)stats_data + pdesc->offset;
> 
> Paolo
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK