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

[Tim Deegan <tim@xen.org>]

[CC's clipped]

At 17:38 +0100 on 13 Mar (1394728686), Tim Deegan wrote:
> [RFC] xen/public/ring.h: simplify RING_HAS_UNCONSUMED_REQUESTS()
> 
> Remove a useless (though harmless) extra check.

What shall we do about this?  AFAICT, Jan and Paul are in favour of the
change; Zoltan is against it.  I am wavering.  Keir, any opinion?

Tim.

> 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@citrix.com>
> Signed-off-by: Tim Deegan <tim@xen.org>
> Cc: Keir Fraser <keir@xen.org>
> Cc: Jan Beulich <JBeulich@suse.com>
> Cc: Ian Campbell <Ian.Campbell@citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> Cc: David Vrabel <david.vrabel@citrix.com>
> Cc: Alan Somers <alans@spectralogic.com>
> Cc: John Suykerbuyk <johns@spectralogic.com>
> Cc: Manuel Bouyer <bouyer@NetBSD.org>
> Cc: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> 
> ---
> 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@lists.xen.org
> http://lists.xen.org/xen-devel