| Message ID | 20191129213505.18472-4-peterx@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: Dirty ring interface | expand |
On Fri, Nov 29, 2019 at 04:34:53PM -0500, Peter Xu wrote: > It's already going to reach 2400 Bytes (which is over half of page > size on 4K page archs), so maybe it's good to have this build-time > check in case it overflows when adding new fields. Please explain why exceeding PAGE_SIZE is a bad thing. I realize it's almost absurdly obvious when looking at the code, but a) the patch itself does not provide that context and b) the changelog should hold up on its own, e.g. in a mostly hypothetical case where the allocation of vcpu->run were changed to something else. > Signed-off-by: Peter Xu <peterx@redhat.com> > --- > virt/kvm/kvm_main.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > index 8f8940cc4b84..681452d288cd 100644 > --- a/virt/kvm/kvm_main.c > +++ b/virt/kvm/kvm_main.c > @@ -352,6 +352,8 @@ int kvm_vcpu_init(struct kvm_vcpu *vcpu, struct kvm *kvm, unsigned id) > } > vcpu->run = page_address(page); > > + BUILD_BUG_ON(sizeof(struct kvm_run) > PAGE_SIZE); > + > kvm_vcpu_set_in_spin_loop(vcpu, false); > kvm_vcpu_set_dy_eligible(vcpu, false); > vcpu->preempted = false; > -- > 2.21.0 >
On Mon, Dec 02, 2019 at 11:30:27AM -0800, Sean Christopherson wrote: > On Fri, Nov 29, 2019 at 04:34:53PM -0500, Peter Xu wrote: > > It's already going to reach 2400 Bytes (which is over half of page > > size on 4K page archs), so maybe it's good to have this build-time > > check in case it overflows when adding new fields. > > Please explain why exceeding PAGE_SIZE is a bad thing. I realize it's > almost absurdly obvious when looking at the code, but a) the patch itself > does not provide that context and b) the changelog should hold up on its > own, Right, I'll enhance the commit message. > e.g. in a mostly hypothetical case where the allocation of vcpu->run > were changed to something else. And that's why I added BUILD_BUG_ON right beneath that allocation. :) It's just a helper for developers when adding new kvm_run fields, not a risk for anyone who wants to start allocating more pages for it. Thanks, > > > Signed-off-by: Peter Xu <peterx@redhat.com> > > --- > > virt/kvm/kvm_main.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > > index 8f8940cc4b84..681452d288cd 100644 > > --- a/virt/kvm/kvm_main.c > > +++ b/virt/kvm/kvm_main.c > > @@ -352,6 +352,8 @@ int kvm_vcpu_init(struct kvm_vcpu *vcpu, struct kvm *kvm, unsigned id) > > } > > vcpu->run = page_address(page); > > > > + BUILD_BUG_ON(sizeof(struct kvm_run) > PAGE_SIZE); > > + > > kvm_vcpu_set_in_spin_loop(vcpu, false); > > kvm_vcpu_set_dy_eligible(vcpu, false); > > vcpu->preempted = false; > > -- > > 2.21.0 > > >
On Mon, Dec 02, 2019 at 03:53:15PM -0500, Peter Xu wrote: > On Mon, Dec 02, 2019 at 11:30:27AM -0800, Sean Christopherson wrote: > > On Fri, Nov 29, 2019 at 04:34:53PM -0500, Peter Xu wrote: > > > It's already going to reach 2400 Bytes (which is over half of page > > > size on 4K page archs), so maybe it's good to have this build-time > > > check in case it overflows when adding new fields. > > > > Please explain why exceeding PAGE_SIZE is a bad thing. I realize it's > > almost absurdly obvious when looking at the code, but a) the patch itself > > does not provide that context and b) the changelog should hold up on its > > own, > > Right, I'll enhance the commit message. > > > e.g. in a mostly hypothetical case where the allocation of vcpu->run > > were changed to something else. > > And that's why I added BUILD_BUG_ON right beneath that allocation. :) My point is that if the allocation were changed to no longer be a straightforward alloc_page() then someone reading the combined code would have no idea why the BUILD_BUG_ON() exists. It's a bit ridiculous for this case because the specific constraints of vcpu->run make it highly unlikely to use anything else, but that's beside the point. > It's just a helper for developers when adding new kvm_run fields, not > a risk for anyone who wants to start allocating more pages for it. But by adding a BUILD_BUG_ON without explaining *why*, you're placing an extra burden on someone that wants to increase the size of kvm->run, e.g. it's not at all obvious from the changelog whether this patch is adding the BUILD_BUG_ON purely because the code allocates memory for vcpu->run via alloc_page(), or if there is some fundamental aspect of vcpu->run that requires it to never span multiple pages.
On Mon, Dec 02, 2019 at 02:19:49PM -0800, Sean Christopherson wrote: > On Mon, Dec 02, 2019 at 03:53:15PM -0500, Peter Xu wrote: > > On Mon, Dec 02, 2019 at 11:30:27AM -0800, Sean Christopherson wrote: > > > On Fri, Nov 29, 2019 at 04:34:53PM -0500, Peter Xu wrote: > > > > It's already going to reach 2400 Bytes (which is over half of page > > > > size on 4K page archs), so maybe it's good to have this build-time > > > > check in case it overflows when adding new fields. > > > > > > Please explain why exceeding PAGE_SIZE is a bad thing. I realize it's > > > almost absurdly obvious when looking at the code, but a) the patch itself > > > does not provide that context and b) the changelog should hold up on its > > > own, > > > > Right, I'll enhance the commit message. > > > > > e.g. in a mostly hypothetical case where the allocation of vcpu->run > > > were changed to something else. > > > > And that's why I added BUILD_BUG_ON right beneath that allocation. :) > > My point is that if the allocation were changed to no longer be a > straightforward alloc_page() then someone reading the combined code would > have no idea why the BUILD_BUG_ON() exists. It's a bit ridiculous for > this case because the specific constraints of vcpu->run make it highly > unlikely to use anything else, but that's beside the point. > > > It's just a helper for developers when adding new kvm_run fields, not > > a risk for anyone who wants to start allocating more pages for it. > > But by adding a BUILD_BUG_ON without explaining *why*, you're placing an > extra burden on someone that wants to increase the size of kvm->run, e.g. > it's not at all obvious from the changelog whether this patch is adding > the BUILD_BUG_ON purely because the code allocates memory for vcpu->run > via alloc_page(), or if there is some fundamental aspect of vcpu->run that > requires it to never span multiple pages. How about I add a comment above it? /* * Currently kvm_run only uses one physical page. Warn the develper * if kvm_run accidentaly grows more than that. */ BUILD_BUG_ON(...); Thanks,
On Mon, Dec 02, 2019 at 05:40:34PM -0500, Peter Xu wrote: > On Mon, Dec 02, 2019 at 02:19:49PM -0800, Sean Christopherson wrote: > > On Mon, Dec 02, 2019 at 03:53:15PM -0500, Peter Xu wrote: > > > On Mon, Dec 02, 2019 at 11:30:27AM -0800, Sean Christopherson wrote: > > > > On Fri, Nov 29, 2019 at 04:34:53PM -0500, Peter Xu wrote: > > > > > It's already going to reach 2400 Bytes (which is over half of page > > > > > size on 4K page archs), so maybe it's good to have this build-time > > > > > check in case it overflows when adding new fields. > > > > > > > > Please explain why exceeding PAGE_SIZE is a bad thing. I realize it's > > > > almost absurdly obvious when looking at the code, but a) the patch itself > > > > does not provide that context and b) the changelog should hold up on its > > > > own, > > > > > > Right, I'll enhance the commit message. > > > > > > > e.g. in a mostly hypothetical case where the allocation of vcpu->run > > > > were changed to something else. > > > > > > And that's why I added BUILD_BUG_ON right beneath that allocation. :) > > > > My point is that if the allocation were changed to no longer be a > > straightforward alloc_page() then someone reading the combined code would > > have no idea why the BUILD_BUG_ON() exists. It's a bit ridiculous for > > this case because the specific constraints of vcpu->run make it highly > > unlikely to use anything else, but that's beside the point. > > > > > It's just a helper for developers when adding new kvm_run fields, not > > > a risk for anyone who wants to start allocating more pages for it. > > > > But by adding a BUILD_BUG_ON without explaining *why*, you're placing an > > extra burden on someone that wants to increase the size of kvm->run, e.g. > > it's not at all obvious from the changelog whether this patch is adding > > the BUILD_BUG_ON purely because the code allocates memory for vcpu->run > > via alloc_page(), or if there is some fundamental aspect of vcpu->run that > > requires it to never span multiple pages. > > How about I add a comment above it? > > /* > * Currently kvm_run only uses one physical page. Warn the develper > * if kvm_run accidentaly grows more than that. > */ > BUILD_BUG_ON(...); No need for a comment, adding a blurb in the changelog is sufficient. The lengthy response was just trying to explain why it's helpful to explicitly justify a change that may seem obvious in the current codebase. Apologies if it only confused things.
On 02/12/19 23:19, Sean Christopherson wrote: >>> e.g. in a mostly hypothetical case where the allocation of vcpu->run >>> were changed to something else. >> And that's why I added BUILD_BUG_ON right beneath that allocation. :) It's not exactly beneath it (it's out of the patch context at least). I think a comment is not strictly necessary, but a better commit message is and, since you are at it, I would put the BUILD_BUG_ON *before* the allocation. That makes it more obvious that you are checking the invariant before allocating. Paolo
On Tue, Dec 03, 2019 at 02:41:58PM +0100, Paolo Bonzini wrote: > On 02/12/19 23:19, Sean Christopherson wrote: > >>> e.g. in a mostly hypothetical case where the allocation of vcpu->run > >>> were changed to something else. > >> And that's why I added BUILD_BUG_ON right beneath that allocation. :) > > It's not exactly beneath it (it's out of the patch context at least). I > think a comment is not strictly necessary, but a better commit message > is and, since you are at it, I would put the BUILD_BUG_ON *before* the > allocation. That makes it more obvious that you are checking the > invariant before allocating. Makes sense, will do. Thanks for both of your reviews.
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index 8f8940cc4b84..681452d288cd 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -352,6 +352,8 @@ int kvm_vcpu_init(struct kvm_vcpu *vcpu, struct kvm *kvm, unsigned id) } vcpu->run = page_address(page); + BUILD_BUG_ON(sizeof(struct kvm_run) > PAGE_SIZE); + kvm_vcpu_set_in_spin_loop(vcpu, false); kvm_vcpu_set_dy_eligible(vcpu, false); vcpu->preempted = false;
It's already going to reach 2400 Bytes (which is over half of page size on 4K page archs), so maybe it's good to have this build-time check in case it overflows when adding new fields. Signed-off-by: Peter Xu <peterx@redhat.com> --- virt/kvm/kvm_main.c | 2 ++ 1 file changed, 2 insertions(+)