| Message ID | 20191210044829.180122-1-gshan@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | tools/kvm_stat: Fix kvm_exit filter name | expand |
Gavin Shan <gshan@redhat.com> writes: > The filter name is fixed to "exit_reason" for some kvm_exit events, no > matter what architect we have. Actually, the filter name ("exit_reason") > is only applicable to x86, meaning it's broken on other architects > including aarch64. > > This fixes the issue by providing various kvm_exit filter names, depending > on architect we're on. Afterwards, the variable filter name is picked and > applied through ioctl(fd, SET_FILTER). Would it actually make sense to standardize (to certain extent) kvm_exit tracepoints instead? > > Reported-by: Andrew Jones <drjones@redhat.com> > Signed-off-by: Gavin Shan <gshan@redhat.com> > --- > tools/kvm/kvm_stat/kvm_stat | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/tools/kvm/kvm_stat/kvm_stat b/tools/kvm/kvm_stat/kvm_stat > index ad1b9e646c49..f9273614b7e3 100755 > --- a/tools/kvm/kvm_stat/kvm_stat > +++ b/tools/kvm/kvm_stat/kvm_stat > @@ -270,6 +270,7 @@ class ArchX86(Arch): > def __init__(self, exit_reasons): > self.sc_perf_evt_open = 298 > self.ioctl_numbers = IOCTL_NUMBERS > + self.exit_field = 'exit_reason' Also, 'exit_field' name is confusing (probably because I'm thinking of VMCS fields) > self.exit_reasons = exit_reasons > > def debugfs_is_child(self, field): > @@ -289,6 +290,7 @@ class ArchPPC(Arch): > # numbers depend on the wordsize. > char_ptr_size = ctypes.sizeof(ctypes.c_char_p) > self.ioctl_numbers['SET_FILTER'] = 0x80002406 | char_ptr_size << 16 > + self.exit_field = 'exit_nr' > self.exit_reasons = {} > > def debugfs_is_child(self, field): > @@ -300,6 +302,7 @@ class ArchA64(Arch): > def __init__(self): > self.sc_perf_evt_open = 241 > self.ioctl_numbers = IOCTL_NUMBERS > + self.exit_field = 'ret' And this is the most confusing part. Why do we have 'ret' as an exit reason in the first place? > self.exit_reasons = AARCH64_EXIT_REASONS > > def debugfs_is_child(self, field): > @@ -311,6 +314,7 @@ class ArchS390(Arch): > def __init__(self): > self.sc_perf_evt_open = 331 > self.ioctl_numbers = IOCTL_NUMBERS > + self.exit_field = None > self.exit_reasons = None > > def debugfs_is_child(self, field): > @@ -541,8 +545,8 @@ class TracepointProvider(Provider): > """ > filters = {} > filters['kvm_userspace_exit'] = ('reason', USERSPACE_EXIT_REASONS) > - if ARCH.exit_reasons: > - filters['kvm_exit'] = ('exit_reason', ARCH.exit_reasons) > + if ARCH.exit_field and ARCH.exit_reasons: > + filters['kvm_exit'] = (ARCH.exit_field, ARCH.exit_reasons) > return filters > > def _get_available_fields(self):
On 12/10/19 7:45 PM, Vitaly Kuznetsov wrote: > Gavin Shan <gshan@redhat.com> writes: > >> The filter name is fixed to "exit_reason" for some kvm_exit events, no >> matter what architect we have. Actually, the filter name ("exit_reason") >> is only applicable to x86, meaning it's broken on other architects >> including aarch64. >> >> This fixes the issue by providing various kvm_exit filter names, depending >> on architect we're on. Afterwards, the variable filter name is picked and >> applied through ioctl(fd, SET_FILTER). > > Would it actually make sense to standardize (to certain extent) kvm_exit > tracepoints instead? > Yes, It makes sense, but it's something for future if you agree. Besides, It seems that other kvm tracepoints need standardization either. >> >> Reported-by: Andrew Jones <drjones@redhat.com> >> Signed-off-by: Gavin Shan <gshan@redhat.com> >> --- >> tools/kvm/kvm_stat/kvm_stat | 8 ++++++-- >> 1 file changed, 6 insertions(+), 2 deletions(-) >> >> diff --git a/tools/kvm/kvm_stat/kvm_stat b/tools/kvm/kvm_stat/kvm_stat >> index ad1b9e646c49..f9273614b7e3 100755 >> --- a/tools/kvm/kvm_stat/kvm_stat >> +++ b/tools/kvm/kvm_stat/kvm_stat >> @@ -270,6 +270,7 @@ class ArchX86(Arch): >> def __init__(self, exit_reasons): >> self.sc_perf_evt_open = 298 >> self.ioctl_numbers = IOCTL_NUMBERS >> + self.exit_field = 'exit_reason' > > Also, 'exit_field' name is confusing (probably because I'm thinking of > VMCS fields) > Ok. How about to have 'exit_reason_field' or 'exit_filter_name'? >> self.exit_reasons = exit_reasons >> >> def debugfs_is_child(self, field): >> @@ -289,6 +290,7 @@ class ArchPPC(Arch): >> # numbers depend on the wordsize. >> char_ptr_size = ctypes.sizeof(ctypes.c_char_p) >> self.ioctl_numbers['SET_FILTER'] = 0x80002406 | char_ptr_size << 16 >> + self.exit_field = 'exit_nr' >> self.exit_reasons = {} >> >> def debugfs_is_child(self, field): >> @@ -300,6 +302,7 @@ class ArchA64(Arch): >> def __init__(self): >> self.sc_perf_evt_open = 241 >> self.ioctl_numbers = IOCTL_NUMBERS >> + self.exit_field = 'ret' > > And this is the most confusing part. Why do we have 'ret' as an exit > reason in the first place? > ah, it turns to be 'esr_ec', which is the class code from ESR_EL2. Will fix in v2. The field name isn't meaningful and it's to be sorted out in future standardization by the way :) >> self.exit_reasons = AARCH64_EXIT_REASONS >> >> def debugfs_is_child(self, field): >> @@ -311,6 +314,7 @@ class ArchS390(Arch): >> def __init__(self): >> self.sc_perf_evt_open = 331 >> self.ioctl_numbers = IOCTL_NUMBERS >> + self.exit_field = None >> self.exit_reasons = None >> >> def debugfs_is_child(self, field): >> @@ -541,8 +545,8 @@ class TracepointProvider(Provider): >> """ >> filters = {} >> filters['kvm_userspace_exit'] = ('reason', USERSPACE_EXIT_REASONS) >> - if ARCH.exit_reasons: >> - filters['kvm_exit'] = ('exit_reason', ARCH.exit_reasons) >> + if ARCH.exit_field and ARCH.exit_reasons: >> + filters['kvm_exit'] = (ARCH.exit_field, ARCH.exit_reasons) >> return filters >> >> def _get_available_fields(self): > Regards, Gavin
Gavin Shan <gshan@redhat.com> writes: > On 12/10/19 7:45 PM, Vitaly Kuznetsov wrote: >> Gavin Shan <gshan@redhat.com> writes: >> >>> The filter name is fixed to "exit_reason" for some kvm_exit events, no >>> matter what architect we have. Actually, the filter name ("exit_reason") >>> is only applicable to x86, meaning it's broken on other architects >>> including aarch64. >>> >>> This fixes the issue by providing various kvm_exit filter names, depending >>> on architect we're on. Afterwards, the variable filter name is picked and >>> applied through ioctl(fd, SET_FILTER). >> >> Would it actually make sense to standardize (to certain extent) kvm_exit >> tracepoints instead? >> > > Yes, It makes sense, but it's something for future if you agree. Besides, > It seems that other kvm tracepoints need standardization either. If we change kvm_stat the way you suggest we'll have reverse issues after changing tracepoints fields: updated kvm_stat won't work with the fixed kernel. I understand that kvm_stat doesn't have to work with anything but the corresponding kernel tree and so we can change it back in the same series, but wouldn't it be an unnecessary churn? I'd suggest we standardize 'exit_reason' field name now just to fix the immediate issue, it shouldn't be a big change, probably the same size as this patch. Changing other tracepoints to match can be left for future generations :-) What do you say?
On 12/11/19 11:15 PM, Vitaly Kuznetsov wrote: > Gavin Shan <gshan@redhat.com> writes: > >> On 12/10/19 7:45 PM, Vitaly Kuznetsov wrote: >>> Gavin Shan <gshan@redhat.com> writes: >>> >>>> The filter name is fixed to "exit_reason" for some kvm_exit events, no >>>> matter what architect we have. Actually, the filter name ("exit_reason") >>>> is only applicable to x86, meaning it's broken on other architects >>>> including aarch64. >>>> >>>> This fixes the issue by providing various kvm_exit filter names, depending >>>> on architect we're on. Afterwards, the variable filter name is picked and >>>> applied through ioctl(fd, SET_FILTER). >>> >>> Would it actually make sense to standardize (to certain extent) kvm_exit >>> tracepoints instead? >>> >> >> Yes, It makes sense, but it's something for future if you agree. Besides, >> It seems that other kvm tracepoints need standardization either. > > If we change kvm_stat the way you suggest we'll have reverse issues > after changing tracepoints fields: updated kvm_stat won't work with the > fixed kernel. I understand that kvm_stat doesn't have to work with > anything but the corresponding kernel tree and so we can change it back > in the same series, but wouldn't it be an unnecessary churn? > > I'd suggest we standardize 'exit_reason' field name now just to fix the > immediate issue, it shouldn't be a big change, probably the same size as > this patch. Changing other tracepoints to match can be left for future > generations :-) What do you say? > Fair enough. A series of patches ("Standardize kvm exit reason field") has been posted for review. Regards, Gavin
On 10/12/19 05:48, Gavin Shan wrote: > The filter name is fixed to "exit_reason" for some kvm_exit events, no > matter what architect we have. Actually, the filter name ("exit_reason") > is only applicable to x86, meaning it's broken on other architects > including aarch64. > > This fixes the issue by providing various kvm_exit filter names, depending > on architect we're on. Afterwards, the variable filter name is picked and > applied through ioctl(fd, SET_FILTER). > > Reported-by: Andrew Jones <drjones@redhat.com> > Signed-off-by: Gavin Shan <gshan@redhat.com> > --- > tools/kvm/kvm_stat/kvm_stat | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/tools/kvm/kvm_stat/kvm_stat b/tools/kvm/kvm_stat/kvm_stat > index ad1b9e646c49..f9273614b7e3 100755 > --- a/tools/kvm/kvm_stat/kvm_stat > +++ b/tools/kvm/kvm_stat/kvm_stat > @@ -270,6 +270,7 @@ class ArchX86(Arch): > def __init__(self, exit_reasons): > self.sc_perf_evt_open = 298 > self.ioctl_numbers = IOCTL_NUMBERS > + self.exit_field = 'exit_reason' > self.exit_reasons = exit_reasons > > def debugfs_is_child(self, field): > @@ -289,6 +290,7 @@ class ArchPPC(Arch): > # numbers depend on the wordsize. > char_ptr_size = ctypes.sizeof(ctypes.c_char_p) > self.ioctl_numbers['SET_FILTER'] = 0x80002406 | char_ptr_size << 16 > + self.exit_field = 'exit_nr' > self.exit_reasons = {} > > def debugfs_is_child(self, field): > @@ -300,6 +302,7 @@ class ArchA64(Arch): > def __init__(self): > self.sc_perf_evt_open = 241 > self.ioctl_numbers = IOCTL_NUMBERS > + self.exit_field = 'ret' > self.exit_reasons = AARCH64_EXIT_REASONS > > def debugfs_is_child(self, field): > @@ -311,6 +314,7 @@ class ArchS390(Arch): > def __init__(self): > self.sc_perf_evt_open = 331 > self.ioctl_numbers = IOCTL_NUMBERS > + self.exit_field = None > self.exit_reasons = None > > def debugfs_is_child(self, field): > @@ -541,8 +545,8 @@ class TracepointProvider(Provider): > """ > filters = {} > filters['kvm_userspace_exit'] = ('reason', USERSPACE_EXIT_REASONS) > - if ARCH.exit_reasons: > - filters['kvm_exit'] = ('exit_reason', ARCH.exit_reasons) > + if ARCH.exit_field and ARCH.exit_reasons: > + filters['kvm_exit'] = (ARCH.exit_field, ARCH.exit_reasons) > return filters > > def _get_available_fields(self): > Queued, thanks. Paolo
On 1/19/20 6:18 AM, Paolo Bonzini wrote: > On 10/12/19 05:48, Gavin Shan wrote: >> The filter name is fixed to "exit_reason" for some kvm_exit events, no >> matter what architect we have. Actually, the filter name ("exit_reason") >> is only applicable to x86, meaning it's broken on other architects >> including aarch64. >> >> This fixes the issue by providing various kvm_exit filter names, depending >> on architect we're on. Afterwards, the variable filter name is picked and >> applied through ioctl(fd, SET_FILTER). >> >> Reported-by: Andrew Jones <drjones@redhat.com> >> Signed-off-by: Gavin Shan <gshan@redhat.com> >> --- >> tools/kvm/kvm_stat/kvm_stat | 8 ++++++-- >> 1 file changed, 6 insertions(+), 2 deletions(-) >> >> diff --git a/tools/kvm/kvm_stat/kvm_stat b/tools/kvm/kvm_stat/kvm_stat >> index ad1b9e646c49..f9273614b7e3 100755 >> --- a/tools/kvm/kvm_stat/kvm_stat >> +++ b/tools/kvm/kvm_stat/kvm_stat >> @@ -270,6 +270,7 @@ class ArchX86(Arch): >> def __init__(self, exit_reasons): >> self.sc_perf_evt_open = 298 >> self.ioctl_numbers = IOCTL_NUMBERS >> + self.exit_field = 'exit_reason' >> self.exit_reasons = exit_reasons >> >> def debugfs_is_child(self, field): >> @@ -289,6 +290,7 @@ class ArchPPC(Arch): >> # numbers depend on the wordsize. >> char_ptr_size = ctypes.sizeof(ctypes.c_char_p) >> self.ioctl_numbers['SET_FILTER'] = 0x80002406 | char_ptr_size << 16 >> + self.exit_field = 'exit_nr' >> self.exit_reasons = {} >> >> def debugfs_is_child(self, field): >> @@ -300,6 +302,7 @@ class ArchA64(Arch): >> def __init__(self): >> self.sc_perf_evt_open = 241 >> self.ioctl_numbers = IOCTL_NUMBERS >> + self.exit_field = 'ret' >> self.exit_reasons = AARCH64_EXIT_REASONS >> >> def debugfs_is_child(self, field): >> @@ -311,6 +314,7 @@ class ArchS390(Arch): >> def __init__(self): >> self.sc_perf_evt_open = 331 >> self.ioctl_numbers = IOCTL_NUMBERS >> + self.exit_field = None >> self.exit_reasons = None >> >> def debugfs_is_child(self, field): >> @@ -541,8 +545,8 @@ class TracepointProvider(Provider): >> """ >> filters = {} >> filters['kvm_userspace_exit'] = ('reason', USERSPACE_EXIT_REASONS) >> - if ARCH.exit_reasons: >> - filters['kvm_exit'] = ('exit_reason', ARCH.exit_reasons) >> + if ARCH.exit_field and ARCH.exit_reasons: >> + filters['kvm_exit'] = (ARCH.exit_field, ARCH.exit_reasons) >> return filters >> >> def _get_available_fields(self): >> > > Queued, thanks. > > Paolo > Paolo, actually the v2 instead of this v1 patch needs to be merged: https://www.spinics.net/lists/kvm/msg202841.html Thanks, Gavin
diff --git a/tools/kvm/kvm_stat/kvm_stat b/tools/kvm/kvm_stat/kvm_stat index ad1b9e646c49..f9273614b7e3 100755 --- a/tools/kvm/kvm_stat/kvm_stat +++ b/tools/kvm/kvm_stat/kvm_stat @@ -270,6 +270,7 @@ class ArchX86(Arch): def __init__(self, exit_reasons): self.sc_perf_evt_open = 298 self.ioctl_numbers = IOCTL_NUMBERS + self.exit_field = 'exit_reason' self.exit_reasons = exit_reasons def debugfs_is_child(self, field): @@ -289,6 +290,7 @@ class ArchPPC(Arch): # numbers depend on the wordsize. char_ptr_size = ctypes.sizeof(ctypes.c_char_p) self.ioctl_numbers['SET_FILTER'] = 0x80002406 | char_ptr_size << 16 + self.exit_field = 'exit_nr' self.exit_reasons = {} def debugfs_is_child(self, field): @@ -300,6 +302,7 @@ class ArchA64(Arch): def __init__(self): self.sc_perf_evt_open = 241 self.ioctl_numbers = IOCTL_NUMBERS + self.exit_field = 'ret' self.exit_reasons = AARCH64_EXIT_REASONS def debugfs_is_child(self, field): @@ -311,6 +314,7 @@ class ArchS390(Arch): def __init__(self): self.sc_perf_evt_open = 331 self.ioctl_numbers = IOCTL_NUMBERS + self.exit_field = None self.exit_reasons = None def debugfs_is_child(self, field): @@ -541,8 +545,8 @@ class TracepointProvider(Provider): """ filters = {} filters['kvm_userspace_exit'] = ('reason', USERSPACE_EXIT_REASONS) - if ARCH.exit_reasons: - filters['kvm_exit'] = ('exit_reason', ARCH.exit_reasons) + if ARCH.exit_field and ARCH.exit_reasons: + filters['kvm_exit'] = (ARCH.exit_field, ARCH.exit_reasons) return filters def _get_available_fields(self):
The filter name is fixed to "exit_reason" for some kvm_exit events, no matter what architect we have. Actually, the filter name ("exit_reason") is only applicable to x86, meaning it's broken on other architects including aarch64. This fixes the issue by providing various kvm_exit filter names, depending on architect we're on. Afterwards, the variable filter name is picked and applied through ioctl(fd, SET_FILTER). Reported-by: Andrew Jones <drjones@redhat.com> Signed-off-by: Gavin Shan <gshan@redhat.com> --- tools/kvm/kvm_stat/kvm_stat | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)