On Fri, Dec 16, 2016 at 05:43:29PM +0100, Halil Pasic wrote: > > > On 12/16/2016 05:12 PM, Stefan Hajnoczi wrote: > >> You are not the first one complaining, so the sentence is definitively > >> bad. What disturbs me regarding your formulation is that we do not use > >> uint16_t to represent neither the ring size nor inuse. > >> > >> How about "Since max ring size < UINT16_MAX it's safe to use modulo > >> UINT16_MAX + 1 subtraction."? > > That doesn't mention "representing the size of the ring" so it's > > unclear what "safe" means. > > > > Stefan > > > > IMHO it is not about representation but about correct arithmetic. > We introduce the cast, not because representing the ring size as > int is necessarily an issue, but because we ended up with a wrong > result. In my opinion how can 'inuse' be represented correctly and > efficiently concerns the member of struct VirtQueue.
Fair enough, the type of VirtQueue.inuse doesn't need to be justified at
this point in the code.
> Here the important point is how conversions between signed unsigned
> integer types work in C.
>
> """
> 6.3.1.3 Signed and unsigned integers
> 1 When a value with integer type is converted to another integer
> type other than _Bool, if the value can be represented by the new
> type, it is unchanged.
> 2 Otherwise, if the new type is unsigned, the value is converted by
> repeatedly adding or subtracting one more than the maximum value that
> can be represented in the new type until the value is in the range
> of the new type.
> """
>
> That is we get mod UINT16_MAX + 1 subtraction which is what we
> need if we want to calculate the difference between the two counters
> under the assumption that the actual conceptual difference (that
> is if the counters where of type arbitrary natural) is less or equal that
> queue size which is less than UINT16_MAX.
- vdev->vq[i].inuse = vdev->vq[i].last_avail_idx -
- vdev->vq[i].used_idx;
+ vdev->vq[i].inuse = (uint16_t)(vdev->vq[i].last_avail_idx -
+ vdev->vq[i].used_idx);
Looking at C99 I learnt the cause of the bug is the integer promotion
performed on the uint16_t subtraction operands. Previously I was only
aware of integer promotion on varargs and function arguments where no
prototype is declared.
So the original statement behaves like:
vdev->vq[i].inuse = (int)vdev->vq[i].last_avail_idx -
(int)vdev->vq[i].used_idx;
This is the real gotcha for me.
If you feel it helps to explain the signed -> unsigned cast behavior,
feel free. I don't think it's necessary.
Stefan
signature.asc
Description: PGP signature
