On Thursday, October 27, 2022 5:23 AM, David Matlack: > On Mon, Oct 24, 2022 at 07:34:27PM +0800, Wei Wang wrote: > > This patch series intends to improve kvm selftests with better code > > consolidation using the helper functions to perform vcpu and thread > > related operations. > > > > In general, several aspects are improved: > > 1) The current users allocate an array of vcpu pointers to the vcpus that > > are added to a vm, and an array of vcpu threads. This isn't necessary > > as kvm_vm already maintains a list of added vcpus. This series changes > > the list of vcpus in the kvm_vm struct to a vcpu array for users to > > work with and removes each user's own allocation of such vcpu arrays. > > Aslo add the vcpu thread to the kvm_vcpu struct, so that users don't > > need to explicitly allocate a thread array to manage vcpu threads on > > their own. > > 2) Change the users to use the helper functions provided by this series > > with the following enhancements: > > - Many users working with pthread_create/join forgot to check if > > error on returning. The helper functions have handled thoses inside, > > so users don't need to handle them by themselves; > > - The vcpu threads created via the helper functions are named in > > "vcpu-##id" format. Naming the threads facilitates debugging, > > performance tuning, runtime pining etc; > > - helper functions named with "vm_vcpu_threads_" iterates over all the > > vcpus that have been added to the vm. Users don't need a explicit > > loop to go through the added cpus by themselves. > > 3) kvm_vcpu is used as the interface parameter to the vcpu thread's > > start routine, and the user specific data is made to be the private > > data in kvm_vcpu. This can simplify the user specific data structures, > > as kvm_vcpu has already included the required info for the thread, for > > example, in patch 13, the cpu_idx field from "struct vcpu_thread" > > is a duplicate of vcpu->id. > > I haven't dug too much into the actual code yet, but I have some high level > feedback based on a quick look through the series: > > - Use the format "KVM: selftests: <Decsription>" for the shortlog. I know it's not common to see so far, but curious is this the required format? I didn't find where it's documented. If it's indeed a requirement, probably we also need to enhance checkpatch.pl to detect this. If it's not required, I think it is more obvious to have /sub_field in the title, e.g. selftests/hardware_disable_test, to outline which specific part of selftests the patch is changing. (the selftests are growing larger with many usages independent of each other). > > - Make the shortlog more specific. "vcpu related code consolidation" is > vague. > > - Do not introduce bugs and then fix them in subsequent commits. This > breaks bisection. For example, kvm_page_table_test is broken at "KVM: > selftests/kvm_util: use vm->vcpus[] when create vm with vcpus" and > then fixed by "KVM: selftests/kvm_page_table_test: vcpu related code > consolidation". > > - Try to limit each patch to one logical change. This is somewhat more > art than science, but the basic idea is to avoid changing too much at > once so that the code is easier to review and bisect. For example, > "KVM: selftests/perf_test_util: vcpu related code consolidation" has > a list of 6 different changes being made in the commit description. > This is a sure sign this commit should be broken up. The same applies > to many of the other patches. This will also make it easier to come > up with more specific shortlogs. OK, will re-organize the patches.
On Thu, Oct 27, 2022, Wang, Wei W wrote: > On Thursday, October 27, 2022 5:23 AM, David Matlack: > > I haven't dug too much into the actual code yet, but I have some high level > > feedback based on a quick look through the series: > > > > - Use the format "KVM: selftests: <Decsription>" for the shortlog. > > I know it's not common to see so far, but curious is this the required format? It's definitely the preferred format. > I didn't find where it's documented. Heh, for all shortlog scopes, the "documentation" is `git log --pretty=oneline` :-) > If it's indeed a requirement, probably we also need to enhance checkpatch.pl > to detect this. I like the idea in theory, but that'd be a daunting task to set up, and quite the maintenance nightmare. There are probably thousands of file => scope mappings throughout the kernel, with any number of exceptions and arbitrary rules. > If it's not required, I think it is more obvious to have /sub_field in the title, > e.g. selftests/hardware_disable_test, to outline which specific part of > selftests the patch is changing. (the selftests are growing larger with many > usages independent of each other). I agree that "KVM: selftests:" is a rather large umbrella, but it's not obvious that "KVM: selfetest/<test>" is unequivocally better, e.g. if someone is making a change to x86_64/vmx_exception_with_invalid_guest_state.c, the scope will be KVM: selftests/x86_64/vmx_exception_with_invalid_guest_state: or KVM: selftests/vmx_exception_with_invalid_guest_state: which doesn't leave a whole lot of room for an actual shortlog. When reviewing selftests patches, I do find myself pausing sometimes to see exactly what file/test is being modified, but in almost all cases a quick glance at the diffstat provides the answer. And on the flip side, having all selftests patches exactly match "KVM: selftests:" makes it super easy to identify selftest changes, i.e. it's mostly a wash overall in terms of efficiency (for me at least).
On Thu, Oct 27, 2022 at 8:44 AM Sean Christopherson <seanjc@google.com> wrote: > > On Thu, Oct 27, 2022, Wang, Wei W wrote: > > On Thursday, October 27, 2022 5:23 AM, David Matlack: > > > I haven't dug too much into the actual code yet, but I have some high level > > > feedback based on a quick look through the series: > > > > > > - Use the format "KVM: selftests: <Decsription>" for the shortlog. > > > > I know it's not common to see so far, but curious is this the required format? > > It's definitely the preferred format. > > > I didn't find where it's documented. > > Heh, for all shortlog scopes, the "documentation" is `git log --pretty=oneline` :-) > > > If it's indeed a requirement, probably we also need to enhance checkpatch.pl > > to detect this. > > I like the idea in theory, but that'd be a daunting task to set up, and quite the > maintenance nightmare. There are probably thousands of file => scope mappings > throughout the kernel, with any number of exceptions and arbitrary rules. I was thinking about proposing this in checkpatch.pl, or in some KVM-specific check script. It seems like the following rule: If a commit only modifies files in tools/testing/selftests/kvm/*, then requires the shortlog match the regex "KVM: selftests: .*". That would handle the vast majority of cases without affecting other subsystems. Sean are you more concerned that if we start validating shortlogs in checkpatch.pl then eventually it will get too out of hand? (i.e. not so concerned with this specific case, but the general problem?) Either way, we should at least document the preferred KVM shortlog styles in Documentation/virtual/kvm/.
On Thu, Oct 27, 2022, David Matlack wrote: > On Thu, Oct 27, 2022 at 8:44 AM Sean Christopherson <seanjc@google.com> wrote: > > I like the idea in theory, but that'd be a daunting task to set up, and quite the > > maintenance nightmare. There are probably thousands of file => scope mappings > > throughout the kernel, with any number of exceptions and arbitrary rules. > > I was thinking about proposing this in checkpatch.pl, or in some > KVM-specific check script. It seems like the following rule: If a > commit only modifies files in tools/testing/selftests/kvm/*, then > requires the shortlog match the regex "KVM: selftests: .*". That would > handle the vast majority of cases without affecting other subsystems. > > Sean are you more concerned that if we start validating shortlogs in > checkpatch.pl then eventually it will get too out of hand? (i.e. not > so concerned with this specific case, but the general problem?) Ya, the general problem. Hardcoding anything KVM specific in checkpatch.pl isn't going to fly. The checkpatch maintainers most definitely don't want to take on the burden of maintaining subsystem rules. Letting one subsystem add custom rules effectively opens the flood gates to all subsystems adding custom rules. And from a KVM perspective, I don't want to have to get an Acked-by from a checkpatch maintiainer just to tweak a KVM rule. The only somewhat feasible approach I can think of would be to provide a generic "language" for shortlog scope rules, and have checkpatch look for a well-known file in relevant directories, e.g. add arch/x86/kvm/SCOPES or whatever. But even that is a non-trivial problem to solve, as it means coming up with a "language" that has a reasonable chance of working for many subsystems without generating too many false positives. It's definitely doable, and likely not actually a maintenance nightmare (I wrote that thinking of modifying a common rules file). But it's still fairly daunting as getting buy-in on something that affects the kernel at large tends to be easier said then done. Then again, I'm probably being pessimistic due to my sub-par regex+scripting skills :-)
On Thu, Oct 27, 2022 at 06:27:39PM +0000, Sean Christopherson wrote: > On Thu, Oct 27, 2022, David Matlack wrote: > > On Thu, Oct 27, 2022 at 8:44 AM Sean Christopherson <seanjc@google.com> wrote: > > > I like the idea in theory, but that'd be a daunting task to set up, and quite the > > > maintenance nightmare. There are probably thousands of file => scope mappings > > > throughout the kernel, with any number of exceptions and arbitrary rules. > > > > I was thinking about proposing this in checkpatch.pl, or in some > > KVM-specific check script. It seems like the following rule: If a > > commit only modifies files in tools/testing/selftests/kvm/*, then > > requires the shortlog match the regex "KVM: selftests: .*". That would > > handle the vast majority of cases without affecting other subsystems. > > > > Sean are you more concerned that if we start validating shortlogs in > > checkpatch.pl then eventually it will get too out of hand? (i.e. not > > so concerned with this specific case, but the general problem?) > > Ya, the general problem. Hardcoding anything KVM specific in checkpatch.pl isn't > going to fly. The checkpatch maintainers most definitely don't want to take on > the burden of maintaining subsystem rules. Letting one subsystem add custom rules > effectively opens the flood gates to all subsystems adding custom rules. And from > a KVM perspective, I don't want to have to get an Acked-by from a checkpatch > maintiainer just to tweak a KVM rule. > > The only somewhat feasible approach I can think of would be to provide a generic > "language" for shortlog scope rules, and have checkpatch look for a well-known > file in relevant directories, e.g. add arch/x86/kvm/SCOPES or whatever. But even > that is a non-trivial problem to solve, as it means coming up with a "language" > that has a reasonable chance of working for many subsystems without generating too > many false positives. > > It's definitely doable, and likely not actually a maintenance nightmare (I wrote > that thinking of modifying a common rules file). But it's still fairly daunting > as getting buy-in on something that affects the kernel at large tends to be easier > said then done. Then again, I'm probably being pessimistic due to my sub-par > regex+scripting skills :-) How about adding support for checkpatch extension plugins? If we could add a plugin script, e.g. tools/testing/selftests/kvm/.checkpatch, and modify checkpatch to run .checkpatch scripts in the patched files' directories (and recursively in the parent directories) when found, then we'd get custom checkpatch behaviors. The scripts wouldn't even have to be written in Perl (but I say that a bit sadly, because I like Perl). Thanks, drew
On Fri, Oct 28, 2022, Andrew Jones wrote: > On Thu, Oct 27, 2022 at 06:27:39PM +0000, Sean Christopherson wrote: > > On Thu, Oct 27, 2022, David Matlack wrote: > > > On Thu, Oct 27, 2022 at 8:44 AM Sean Christopherson <seanjc@google.com> wrote: > > > > I like the idea in theory, but that'd be a daunting task to set up, and quite the > > > > maintenance nightmare. There are probably thousands of file => scope mappings > > > > throughout the kernel, with any number of exceptions and arbitrary rules. > > > > > > I was thinking about proposing this in checkpatch.pl, or in some > > > KVM-specific check script. It seems like the following rule: If a > > > commit only modifies files in tools/testing/selftests/kvm/*, then > > > requires the shortlog match the regex "KVM: selftests: .*". That would > > > handle the vast majority of cases without affecting other subsystems. > > > > > > Sean are you more concerned that if we start validating shortlogs in > > > checkpatch.pl then eventually it will get too out of hand? (i.e. not > > > so concerned with this specific case, but the general problem?) > > > > Ya, the general problem. Hardcoding anything KVM specific in checkpatch.pl isn't > > going to fly. The checkpatch maintainers most definitely don't want to take on > > the burden of maintaining subsystem rules. Letting one subsystem add custom rules > > effectively opens the flood gates to all subsystems adding custom rules. And from > > a KVM perspective, I don't want to have to get an Acked-by from a checkpatch > > maintiainer just to tweak a KVM rule. > > > > The only somewhat feasible approach I can think of would be to provide a generic > > "language" for shortlog scope rules, and have checkpatch look for a well-known > > file in relevant directories, e.g. add arch/x86/kvm/SCOPES or whatever. But even > > that is a non-trivial problem to solve, as it means coming up with a "language" > > that has a reasonable chance of working for many subsystems without generating too > > many false positives. > > > > It's definitely doable, and likely not actually a maintenance nightmare (I wrote > > that thinking of modifying a common rules file). But it's still fairly daunting > > as getting buy-in on something that affects the kernel at large tends to be easier > > said then done. Then again, I'm probably being pessimistic due to my sub-par > > regex+scripting skills :-) > > How about adding support for checkpatch extension plugins? If we could add > a plugin script, e.g. tools/testing/selftests/kvm/.checkpatch, and modify > checkpatch to run .checkpatch scripts in the patched files' directories > (and recursively in the parent directories) when found, then we'd get > custom checkpatch behaviors. The scripts wouldn't even have to be written > in Perl (but I say that a bit sadly, because I like Perl). That will work for simple cases, but patches that touch files in multiple directories will be messy. E.g. a patch that touches virt/kvm/ and arch/x86/kvm/ will have two separate custom rules enforcing two different scopes. Recursively executing plugins will also be problematic, e.g. except for KVM, arch/x86/ is maintained by the tip-tree folks, and the tip-tree is quite opinionated on all sorts of things, whereas KVM tends to be a bit more relaxed. Enforcing scope through plugins would also lead to some amount of duplicate code throught subsystems. Anyways, if someone wants to pursue this, these ideas and the "requirement" should be run by the checkpatch maintainers. They have far more experience and authority in this area, and I suspect we aren't the first people to want checkpatch to get involved in enforcing shortlog scope.
On Fri, Oct 28, 2022 at 8:49 AM Sean Christopherson <seanjc@google.com> wrote: > > On Fri, Oct 28, 2022, Andrew Jones wrote: > > On Thu, Oct 27, 2022 at 06:27:39PM +0000, Sean Christopherson wrote: > > > On Thu, Oct 27, 2022, David Matlack wrote: > > > > On Thu, Oct 27, 2022 at 8:44 AM Sean Christopherson <seanjc@google.com> wrote: > > > > > I like the idea in theory, but that'd be a daunting task to set up, and quite the > > > > > maintenance nightmare. There are probably thousands of file => scope mappings > > > > > throughout the kernel, with any number of exceptions and arbitrary rules. > > > > > > > > I was thinking about proposing this in checkpatch.pl, or in some > > > > KVM-specific check script. It seems like the following rule: If a > > > > commit only modifies files in tools/testing/selftests/kvm/*, then > > > > requires the shortlog match the regex "KVM: selftests: .*". That would > > > > handle the vast majority of cases without affecting other subsystems. > > > > > > > > Sean are you more concerned that if we start validating shortlogs in > > > > checkpatch.pl then eventually it will get too out of hand? (i.e. not > > > > so concerned with this specific case, but the general problem?) > > > > > > Ya, the general problem. Hardcoding anything KVM specific in checkpatch.pl isn't > > > going to fly. The checkpatch maintainers most definitely don't want to take on > > > the burden of maintaining subsystem rules. Letting one subsystem add custom rules > > > effectively opens the flood gates to all subsystems adding custom rules. And from > > > a KVM perspective, I don't want to have to get an Acked-by from a checkpatch > > > maintiainer just to tweak a KVM rule. > > > > > > The only somewhat feasible approach I can think of would be to provide a generic > > > "language" for shortlog scope rules, and have checkpatch look for a well-known > > > file in relevant directories, e.g. add arch/x86/kvm/SCOPES or whatever. But even > > > that is a non-trivial problem to solve, as it means coming up with a "language" > > > that has a reasonable chance of working for many subsystems without generating too > > > many false positives. > > > > > > It's definitely doable, and likely not actually a maintenance nightmare (I wrote > > > that thinking of modifying a common rules file). But it's still fairly daunting > > > as getting buy-in on something that affects the kernel at large tends to be easier > > > said then done. Then again, I'm probably being pessimistic due to my sub-par > > > regex+scripting skills :-) > > > > How about adding support for checkpatch extension plugins? If we could add > > a plugin script, e.g. tools/testing/selftests/kvm/.checkpatch, and modify > > checkpatch to run .checkpatch scripts in the patched files' directories > > (and recursively in the parent directories) when found, then we'd get > > custom checkpatch behaviors. The scripts wouldn't even have to be written > > in Perl (but I say that a bit sadly, because I like Perl). > > That will work for simple cases, but patches that touch files in multiple directories > will be messy. E.g. a patch that touches virt/kvm/ and arch/x86/kvm/ will have > two separate custom rules enforcing two different scopes. > > Recursively executing plugins will also be problematic, e.g. except for KVM, arch/x86/ > is maintained by the tip-tree folks, and the tip-tree is quite opinionated on all > sorts of things, whereas KVM tends to be a bit more relaxed. > > Enforcing scope through plugins would also lead to some amount of duplicate code > throught subsystems. > > Anyways, if someone wants to pursue this, these ideas and the "requirement" should > be run by the checkpatch maintainers. They have far more experience and authority > in this area, and I suspect we aren't the first people to want checkpatch to get > involved in enforcing shortlog scope. Documenting would at least be an improvement over what we have today since it would eliminate the need to re-explain the preferred rules every time. We can just point to the documentation when reviewing patches. `git log --pretty=oneline` is not a great way to document shortlog scopes because it does not explain the rules (e.g. when to use "KVM: x86: " vs "KVM: x86/mmu: "), does not explain why things the way they are, and is inconsistent since we don't always catch every patch that goes by with a non-preferred shortlog scope.
On Mon, Nov 07, 2022, David Matlack wrote: > On Fri, Oct 28, 2022 at 8:49 AM Sean Christopherson <seanjc@google.com> wrote: > > Anyways, if someone wants to pursue this, these ideas and the "requirement" should > > be run by the checkpatch maintainers. They have far more experience and authority > > in this area, and I suspect we aren't the first people to want checkpatch to get > > involved in enforcing shortlog scope. > > Documenting would at least be an improvement over what we have today > since it would eliminate the need to re-explain the preferred rules > every time. We can just point to the documentation when reviewing > patches. Agreed. And there are many other things I want to formalize for KVM x86, e.g. testing expectations, health requirements for the various branches, what each branch is used for etc... If you want to send a patch for the shortlogs thing, maybe create Documentation/process/maintainer-kvm-x86.rst and link it into Documentation/process/maintainer-handbooks.rst? > `git log --pretty=oneline` is not a great way to document shortlog > scopes because it does not explain the rules (e.g. when to use "KVM: > x86: " vs "KVM: x86/mmu: "), does not explain why things the way they > are, and is inconsistent since we don't always catch every patch that > goes by with a non-preferred shortlog scope.
On Mon, Nov 7, 2022 at 10:19 AM Sean Christopherson <seanjc@google.com> wrote: > > On Mon, Nov 07, 2022, David Matlack wrote: > > On Fri, Oct 28, 2022 at 8:49 AM Sean Christopherson <seanjc@google.com> wrote: > > > Anyways, if someone wants to pursue this, these ideas and the "requirement" should > > > be run by the checkpatch maintainers. They have far more experience and authority > > > in this area, and I suspect we aren't the first people to want checkpatch to get > > > involved in enforcing shortlog scope. > > > > Documenting would at least be an improvement over what we have today > > since it would eliminate the need to re-explain the preferred rules > > every time. We can just point to the documentation when reviewing > > patches. > > Agreed. And there are many other things I want to formalize for KVM x86, e.g. > testing expectations, health requirements for the various branches, what each > branch is used for etc... > > If you want to send a patch for the shortlogs thing, maybe create > > Documentation/process/maintainer-kvm-x86.rst > > and link it into Documentation/process/maintainer-handbooks.rst? Can do. I'll try to take a look later this week or next week.
© 2016 - 2026 Red Hat, Inc.