Date: Thu, 14 Mar 2013 14:13:43 -0600 From: Chris Torek <torek@torek.net> To: Peter Grehan <grehan@freebsd.org> Cc: freebsd-virtualization@freebsd.org Subject: Re: trivial improvement for usr.sbin/bhyve/pci_virtio_block.c Message-ID: <201303142013.r2EKDhAE081454@elf.torek.net> In-Reply-To: Your message of "Thu, 28 Feb 2013 18:15:54 -0700." <5130014A.3030607@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
>> I was looking through the bhyve code and noticed an obvious
>> easy (if trivial) code improvement.
>>
>> Tested "standalone" rather than inside bhyve (with both gcc and
>> clang, on FreeBSD 9.0).
>
> Thanks; I'll apply it.
>
>> Not sure where/how diffs should go, so I figured I would send this
>> here as a test. :-)
>
> This is as good a place as any :)
>
>later,
>
>Peter.
I looked at the svn commit and there's a glitch...
I only sent you one file (pci_virtio_block.c) of the two (not
having realized the code was duplicated in pci_virtio_net.c). You
applied the change to both files but missed something:
Index: pci_virtio_block.c
===================================================================
--- pci_virtio_block.c (revision 247865)
+++ pci_virtio_block.c (revision 247871)
@@ -164,14 +164,19 @@
static int
hq_num_avail(struct vring_hqueue *hq)
{
- int ndesc;
+ uint16_t ndesc;
This change (to use uint16_t) is important.
- if (*hq->hq_avail_idx >= hq->hq_cur_aidx)
- ndesc = *hq->hq_avail_idx - hq->hq_cur_aidx;
- else
- ndesc = UINT16_MAX - hq->hq_cur_aidx + *hq->hq_avail_idx + 1;
+ /*
+ * We're just computing (a-b) in GF(216).
(minor: this should read "2 caret 16" or maybe "2**16", presumably
some mail software ate it?)
+ *
+ * The only glitch here is that in standard C,
+ * uint16_t promotes to (signed) int when int has
+ * more than 16 bits (pretty much always now), so
+ * we have to force it back to unsigned.
+ */
+ ndesc = (unsigned)*hq->hq_avail_idx - (unsigned)hq->hq_cur_aidx;
The trick here is that if, e.g., avail is 3 and cur is 29,
we get 3 - 29 = 0xffffffe6, which should be considered "the
same as" 65510. The original calculation did (via the else
half):
ndesc = UINT16_MAX - hq->hq_cur_aidx + *hq->hq_avail_idx + 1
= 65535 - 29 + 3 + 1
which is 65510. The new one computes (in 32-bit unsigned)
0xffffffe6 but then assigns the result to a 16-bit unsigned
temporary (i.e., ndesc), chopping off the unwanted high bits
and resulting in 0xffe6 = 65510.
- assert(ndesc >= 0 && ndesc <= hq->hq_size);
+ assert(ndesc <= hq->hq_size);
return (ndesc);
}
Index: pci_virtio_net.c
===================================================================
--- pci_virtio_net.c (revision 247865)
+++ pci_virtio_net.c (revision 247871)
@@ -172,12 +172,17 @@
{
int ndesc;
Uh oh, here we (still) have a regular plain "int"...
- if (*hq->hq_avail_idx >= hq->hq_cur_aidx)
- ndesc = *hq->hq_avail_idx - hq->hq_cur_aidx;
- else
- ndesc = UINT16_MAX - hq->hq_cur_aidx + *hq->hq_avail_idx + 1;
+ /*
+ * We're just computing (a-b) in GF(216).
+ *
+ * The only glitch here is that in standard C,
+ * uint16_t promotes to (signed) int when int has
+ * more than 16 bits (pretty much always now), so
+ * we have to force it back to unsigned.
+ */
+ ndesc = (unsigned)*hq->hq_avail_idx - (unsigned)hq->hq_cur_aidx;
This time we'll do something like "3 - 29" in unsigned and get
0xffffffe6 as before, but then stick that in a (32 bit) int and
believe it means -26.
- assert(ndesc >= 0 && ndesc <= hq->hq_size);
+ assert(ndesc <= hq->hq_size);
Without the >= 0 part of the assert, we'll miss the underflow
since ndesc is signed, and hq_size is uint16_t and will widen
to (plain signed) int here.
(Of course it's more typical to have, e.g., not 3 and 29 but
0xfffc = 65532 and 24, where the subtraction will produce
-65508 when we wanted 28. I'd have to look closely at the
rest of the code to see what happens after that.)
return (ndesc);
}
The comment might be misleading -- the trick is to compute the
result in two steps, first in GF(2**{anything >= 16}), then
*reduce* it (via assigment to uint16_t) to GF(2**16). (I used
"**" this time instead of ^, just to be different :-) )
Chris
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201303142013.r2EKDhAE081454>