| Message ID | 20240710085259.2125131-2-ilstam@amazon.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: Improve MMIO Coalescing API | expand |
On 10/07/2024 10:52, Ilias Stamatis wrote: > The following calculation used in coalesced_mmio_has_room() to check > whether the ring buffer is full is wrong and only allows half the buffer > to be used. > > avail = (ring->first - last - 1) % KVM_COALESCED_MMIO_MAX; > if (avail == 0) > /* full */ > > The % operator in C is not the modulo operator but the remainder > operator. Modulo and remainder operators differ with respect to negative > values. But all values are unsigned in this case anyway. > > The above might have worked as expected in python for example: >>>> (-86) % 170 > 84 > > However it doesn't work the same way in C. > > printf("avail: %d\n", (-86) % 170); > printf("avail: %u\n", (-86) % 170); > printf("avail: %u\n", (-86u) % 170u); > > Using gcc-11 these print: > > avail: -86 > avail: 4294967210 > avail: 0 > > Fix the calculation and allow all but one entries in the buffer to be > used as originally intended. > > Fixes: 105f8d40a737 ("KVM: Calculate available entries in coalesced mmio ring") > Signed-off-by: Ilias Stamatis <ilstam@amazon.com> > --- > virt/kvm/coalesced_mmio.c | 4 +--- > 1 file changed, 1 insertion(+), 3 deletions(-) > Reviewed-by: Paul Durrant <paul@xen.org>
On Wed, Jul 10, 2024 at 09:52:54AM +0100, Ilias Stamatis wrote: > The following calculation used in coalesced_mmio_has_room() to check > whether the ring buffer is full is wrong and only allows half the buffer > to be used. > > avail = (ring->first - last - 1) % KVM_COALESCED_MMIO_MAX; > if (avail == 0) > /* full */ > > The % operator in C is not the modulo operator but the remainder > operator. Modulo and remainder operators differ with respect to negative > values. But all values are unsigned in this case anyway. > > The above might have worked as expected in python for example: > >>> (-86) % 170 > 84 > > However it doesn't work the same way in C. > > printf("avail: %d\n", (-86) % 170); > printf("avail: %u\n", (-86) % 170); > printf("avail: %u\n", (-86u) % 170u); > > Using gcc-11 these print: > > avail: -86 > avail: 4294967210 > avail: 0 Where exactly do you see a problem? As you correctly point out, all values are unsigned, so unsigned arithmetics with wraparound applies, and then % operator is applied to the resulting unsigned value. Out your three examples, only the last one is relevant, and it's perfectly the intended behavior. Thanks, Roman. Amazon Web Services Development Center Germany GmbH Krausenstr. 38 10117 Berlin Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss Eingetragen am Amtsgericht Charlottenburg unter HRB 257764 B Sitz: Berlin Ust-ID: DE 365 538 597
On Fri, 2024-07-12 at 17:55 +0200, Roman Kagan wrote: > On Wed, Jul 10, 2024 at 09:52:54AM +0100, Ilias Stamatis wrote: > > The following calculation used in coalesced_mmio_has_room() to check > > whether the ring buffer is full is wrong and only allows half the buffer > > to be used. > > > > avail = (ring->first - last - 1) % KVM_COALESCED_MMIO_MAX; > > if (avail == 0) > > /* full */ > > > > The % operator in C is not the modulo operator but the remainder > > operator. Modulo and remainder operators differ with respect to negative > > values. But all values are unsigned in this case anyway. > > > > The above might have worked as expected in python for example: > > > > > (-86) % 170 > > 84 > > > > However it doesn't work the same way in C. > > > > printf("avail: %d\n", (-86) % 170); > > printf("avail: %u\n", (-86) % 170); > > printf("avail: %u\n", (-86u) % 170u); > > > > Using gcc-11 these print: > > > > avail: -86 > > avail: 4294967210 > > avail: 0 > > Where exactly do you see a problem? As you correctly point out, all > values are unsigned, so unsigned arithmetics with wraparound applies, > and then % operator is applied to the resulting unsigned value. Out > your three examples, only the last one is relevant, and it's perfectly > the intended behavior. > > Thanks, > Roman. KVM_COALESCED_MMIO_MAX on x86 is 170, which means the ring buffer has 170 entries (169 of which should be usable). If first = 0 and last = 85 then the calculation gives 0 available entries in which case we consider the buffer to be full and we exit to userspace. But we shouldn't as there are still 84 unused entries. So I don't see how this could have been the intended behaviour. Ilias
On Fri, Jul 12, 2024 at 09:03:32PM +0200, Stamatis, Ilias wrote: > On Fri, 2024-07-12 at 17:55 +0200, Roman Kagan wrote: > > On Wed, Jul 10, 2024 at 09:52:54AM +0100, Ilias Stamatis wrote: > > > The following calculation used in coalesced_mmio_has_room() to check > > > whether the ring buffer is full is wrong and only allows half the buffer > > > to be used. > > > > > > avail = (ring->first - last - 1) % KVM_COALESCED_MMIO_MAX; > > > if (avail == 0) > > > /* full */ > > > > > > The % operator in C is not the modulo operator but the remainder > > > operator. Modulo and remainder operators differ with respect to negative > > > values. But all values are unsigned in this case anyway. > > > > > > The above might have worked as expected in python for example: > > > > > > (-86) % 170 > > > 84 > > > > > > However it doesn't work the same way in C. > > > > > > printf("avail: %d\n", (-86) % 170); > > > printf("avail: %u\n", (-86) % 170); > > > printf("avail: %u\n", (-86u) % 170u); > > > > > > Using gcc-11 these print: > > > > > > avail: -86 > > > avail: 4294967210 > > > avail: 0 > > > > Where exactly do you see a problem? As you correctly point out, all > > values are unsigned, so unsigned arithmetics with wraparound applies, > > and then % operator is applied to the resulting unsigned value. Out > > your three examples, only the last one is relevant, and it's perfectly > > the intended behavior. > > > > Thanks, > > Roman. > > KVM_COALESCED_MMIO_MAX on x86 is 170, which means the ring buffer has > 170 entries (169 of which should be usable). > > If first = 0 and last = 85 then the calculation gives 0 available > entries in which case we consider the buffer to be full and we exit to > userspace. But we shouldn't as there are still 84 unused entries. You want to add a stride instead avail = (ring->first + KVM_COALESCED_MMIO_MAX - 1 - last) % KVM_COALESCED_MMIO_MAX; As long as the stride is smaller than half the wraparound, you can think of the values being on an infinite non-negative axis. Roman. Amazon Web Services Development Center Germany GmbH Krausenstr. 38 10117 Berlin Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss Eingetragen am Amtsgericht Charlottenburg unter HRB 257764 B Sitz: Berlin Ust-ID: DE 365 538 597
diff --git a/virt/kvm/coalesced_mmio.c b/virt/kvm/coalesced_mmio.c index 1b90acb6e3fe..184c5c40c9c1 100644 --- a/virt/kvm/coalesced_mmio.c +++ b/virt/kvm/coalesced_mmio.c @@ -43,7 +43,6 @@ static int coalesced_mmio_in_range(struct kvm_coalesced_mmio_dev *dev, static int coalesced_mmio_has_room(struct kvm_coalesced_mmio_dev *dev, u32 last) { struct kvm_coalesced_mmio_ring *ring; - unsigned avail; /* Are we able to batch it ? */ @@ -52,8 +51,7 @@ static int coalesced_mmio_has_room(struct kvm_coalesced_mmio_dev *dev, u32 last) * there is always one unused entry in the buffer */ ring = dev->kvm->coalesced_mmio_ring; - avail = (ring->first - last - 1) % KVM_COALESCED_MMIO_MAX; - if (avail == 0) { + if ((last + 1) % KVM_COALESCED_MMIO_MAX == READ_ONCE(ring->first)) { /* full */ return 0; }