[Xen-devel] [PATCH RFC] xen/public/ring.h: simplify RING_HAS_UNCONSUMED_REQUESTS()

[Tim Deegan <tim@xxxxxxx>]

[RFC] xen/public/ring.h: simplify RING_HAS_UNCONSUMED_REQUESTS()

Remove a useless (though harmless) extra check.

Code inspection
---------------

Ian Campbell and I looked at this today and can't find any case where
the existing 'rsp' test would be useful.  In today's ring.h,

#define RING_HAS_UNCONSUMED_REQUESTS(_r) ({                             \
    unsigned int req = (_r)->sring->req_prod - (_r)->req_cons;          \
    unsigned int rsp = RING_SIZE(_r) -                                  \
        ((_r)->req_cons - (_r)->rsp_prod_pvt);                          \
    req < rsp ? req : rsp;                                              \
})

'req' is the number of requests that the F/E has published and we have
not yet consumed.  'rsp' is an odd fish, everything _except_ what we
might call requests locally in flight, that is requests we've consumed
but not produced a response for.  We could only think of two things we
might be trying to test for here: 

(a) req_cons runs ahead of rsp_prod_pvt, as would be expected from the 
    way these rings normally work.  In that case, as Zoltan pointed
    out, rsp must be >= req, for a well-behaved frontend: otherwise
    we'd have req_prod > rsp_prod_pvt + RING_SIZE, implying that
    req_prod > rsp_cons + RING_SIZE, i.e. the frontend has overrun
    the ring.  I don't think this even usefully protects against a
    malicious/buggy frontend.

(b) rsp_prod_pvt runs ahead of req_cons, which seems wrong but I'm
    told might happen in linux netback?  In that case, we might plausibly
    want to try to limit RING_HAS_UNCONSUMED_REQUESTS to max of 
    (req_prod - req_cons) and (req_prod - rsp_prod_pvt), but that's
    not what this does.  Instead, rsp will underflow to 
    RING_SIZE + (rsp_prod_pvt - req_cons), which will always be >= req. 

So in both of those cases, 'rsp' is always >= 'req' and is useless.


Code archaeology
----------------

In the original ring.h, the test was a boolean, as the name implies.
Cset 99812f40 ([NET] back: Add SG support) extended it to a count in
the obvious manner.  Looking at the original (0b788798):

#define RING_HAS_UNCONSUMED_REQUESTS(_p, _r)                            \
   ( ((_r)->req_cons != (_r)->sring->req_prod ) &&                      \
     (((_r)->req_cons - (_r)->rsp_prod_pvt) !=                          \
      SRING_SIZE((_p), (_r)->sring)) )

it seems to be testing for 'there are unconsumed requests _and_ we
have not got a full ring of consumed-but-not-responded requests'.
This also seems to be effectively harmless: if there are unconsumed
requests, we can't possibly have a ring full of c-b-n-r requests
unless the frontend's request producer has overrun its response
consumer.

That code was introduced with no callers, but seems to have been
copied from the existing netback code, which switched to use it in
b242b208.  All later users of it in the xenolinux trees are either
brand new code or replacing a simple (req_cons - req_prod) test.  The
netback code goes back to Keir's original implementation, where it
looked like this, in inverted form (1de448f4):

        /* Work to do? */
        i = netif->tx_req_cons;
        if ( (i == netif->tx->req_prod) ||
             ((i-netif->tx_resp_prod) == NETIF_TX_RING_SIZE) )
        {
            netif_put(netif);
            continue;
        }

Again, I don't think this test is useful: it's only interesting if
netfront has overrun the ring, but it doesn't reliably detect that.

So let's remove it. 

Suggested-by: Zoltan Kiss <zoltan.kiss@xxxxxxxxxx>
Signed-off-by: Tim Deegan <tim@xxxxxxx>
Cc: Keir Fraser <keir@xxxxxxx>
Cc: Jan Beulich <JBeulich@xxxxxxxx>
Cc: Ian Campbell <Ian.Campbell@xxxxxxxxxx>
Cc: Wei Liu <wei.liu2@xxxxxxxxxx>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
Cc: Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>
Cc: David Vrabel <david.vrabel@xxxxxxxxxx>
Cc: Alan Somers <alans@xxxxxxxxxxxxxxxx>
Cc: John Suykerbuyk <johns@xxxxxxxxxxxxxxxx>
Cc: Manuel Bouyer <bouyer@xxxxxxxxxx>
Cc: Paul Durrant <Paul.Durrant@xxxxxxxxxx>
Cc: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>

---
RFC because
- I might well be missing something here; and
- this is a change to a public header that could be in use in any
  number of implementations; since the extra test is very cheap, and
  seems to be harmess, we might consider leaving it in place.
- I haven't tested it at all yet.

CC'ing a whole bunch of people whose code might be using this macro.

diff --git a/xen/include/public/io/ring.h b/xen/include/public/io/ring.h
index 73e13d7..d6e23f1 100644
--- a/xen/include/public/io/ring.h
+++ b/xen/include/public/io/ring.h
@@ -192,21 +192,8 @@ typedef struct __name##_back_ring __name##_back_ring_t
 #define RING_HAS_UNCONSUMED_RESPONSES(_r)                               \
     ((_r)->sring->rsp_prod - (_r)->rsp_cons)
 
-#ifdef __GNUC__
-#define RING_HAS_UNCONSUMED_REQUESTS(_r) ({                             \
-    unsigned int req = (_r)->sring->req_prod - (_r)->req_cons;          \
-    unsigned int rsp = RING_SIZE(_r) -                                  \
-        ((_r)->req_cons - (_r)->rsp_prod_pvt);                          \
-    req < rsp ? req : rsp;                                              \
-})
-#else
-/* Same as above, but without the nice GCC ({ ... }) syntax. */
 #define RING_HAS_UNCONSUMED_REQUESTS(_r)                                \
-    ((((_r)->sring->req_prod - (_r)->req_cons) <                        \
-      (RING_SIZE(_r) - ((_r)->req_cons - (_r)->rsp_prod_pvt))) ?        \
-     ((_r)->sring->req_prod - (_r)->req_cons) :                         \
-     (RING_SIZE(_r) - ((_r)->req_cons - (_r)->rsp_prod_pvt)))
-#endif
+    ((_r)->sring->req_prod - (_r)->req_cons)
 
 /* Direct access to individual ring elements, by index. */
 #define RING_GET_REQUEST(_r, _idx)                                      \

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel