From mboxrd@z Thu Jan 1 00:00:00 1970 From: Zoltan Kiss Subject: Re: [PATCH RFC] xen/public/ring.h: simplify RING_HAS_UNCONSUMED_REQUESTS() Date: Sat, 22 Mar 2014 14:18:03 +0000 Message-ID: <532D9B9B.20705@schaman.hu> References: <5318987C.3030303@citrix.com> <1394121221.13270.10.camel@hastur.hellion.org.uk> <5318A2D8.3090808@citrix.com> <20140306173057.GK11475@deinos.phlegethon.org> <20140313163806.GB41479@deinos.phlegethon.org> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta4.messagelabs.com ([85.158.143.247]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1WRMkS-000517-N7 for xen-devel@lists.xenproject.org; Sat, 22 Mar 2014 14:18:08 +0000 Received: by mail-ee0-f45.google.com with SMTP id d17so2757041eek.18 for ; Sat, 22 Mar 2014 07:18:06 -0700 (PDT) In-Reply-To: <20140313163806.GB41479@deinos.phlegethon.org> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Tim Deegan , xen-devel@lists.xenproject.org Cc: Wei Liu , Ian Campbell , Stefano Stabellini , freebsd-xen@freebsd.org, Alan Somers , Manuel Bouyer , David Vrabel , Jan Beulich , Keir Fraser , Boris Ostrovsky , =?ISO-8859-1?Q?Roger_Pau_Monn=E9?= , Paul Durrant , John Suykerbuyk List-Id: xen-devel@lists.xenproject.org Hi, I think I might have an explanation why do we need this, see this mailing: https://lkml.org/lkml/2014/3/20/710 https://lkml.org/lkml/2014/3/21/111 https://lkml.org/lkml/2014/3/21/390 Zoli On 13/03/14 16:38, Tim Deegan wrote: > [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 > Signed-off-by: Tim Deegan > Cc: Keir Fraser > Cc: Jan Beulich > Cc: Ian Campbell > Cc: Wei Liu > Cc: Konrad Rzeszutek Wilk > Cc: Boris Ostrovsky > Cc: David Vrabel > Cc: Alan Somers > Cc: John Suykerbuyk > Cc: Manuel Bouyer > Cc: Paul Durrant > Cc: Stefano Stabellini > > --- > 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) \ >