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
