|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH RFC] xen/public/ring.h: simplify RING_HAS_UNCONSUMED_REQUESTS()
[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@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
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |