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

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Zoltan Kiss
> Sent: 24 March 2014 12:24
> To: Jan Beulich; Zoltan Kiss; Tim (Xen.org)
> Cc: Wei Liu; Ian Campbell; Stefano Stabellini; Keir (Xen.org); freebsd-
> xen@xxxxxxxxxxx; Alan Somers; Paul Durrant; David Vrabel; xen-
> devel@xxxxxxxxxxxxxxxxxxxx; Boris Ostrovsky; John Suykerbuyk; Manuel
> Bouyer; Roger Pau Monne
> Subject: Re: [Xen-devel] [PATCH RFC] xen/public/ring.h: simplify
> RING_HAS_UNCONSUMED_REQUESTS()
> 
> On 24/03/14 07:38, Jan Beulich wrote:
> >>>> On 22.03.14 at 18:14, <tim@xxxxxxx> wrote:
> >> At 14:18 +0000 on 22 Mar (1395494283), Zoltan Kiss wrote:
> >>> 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
> >>
> >> Quoting from the third of these:
> >>
> >> | But consuming overrunning requests after rsp_prod_pvt is a problem:
> >> | - NAPI instance races with dealloc thread over the slots. The first
> >> | reads them as requests, the second writes them as responses
> >> | - the NAPI instance overwrites used pending slots as well, so skb frag
> >> | release go wrong etc.
> >>
> >> OK, so the backend needs to be careful not to follow the frontend into
> >> overrun, not because of the ring itself being corrupted but because it
> >> will mess up the backend's internal bookkeeping.
> >
> > With s/will/may/ I'm not sure that's a reason to withdraw the patch:
> > The generic macros in ring.h imo shouldn't dictate any particular
> > protection policy beyond protecting the ring itself. I.e. I'd think if
> > netback need protection beyond the one provided by ring.h macros,
> > it should take care to implement them itself.
> 
> It's not "may", it is a "will". In case of Linux netback for sure, but I
> think it's reasonable for any backend to rely on the fact that the ring
> macros protect them from abusive frontends. Protecting a backend to read
> requests from the [rsp_prod_pvt, req_cons] range is a sensible thing to
> do in the ring macros.

I disagree. That's not what the name of the macro implies; it implies a range 
for req_prod - req_cons. The backend is responsible for its own rsp_prod_pvt 
value and should make sure it's safe. If, to that end, it wants to have its own 
variant of the macro then that's reasonable, but adding such a clause to the 
macro in the generic header is (as has been proven) confusing.

  Paul

> Also, RING_FINAL_CHECK_FOR_REQUESTS relies on this macro, removing
> this
> protection may cause other issues, e.g. netback keeps the NAPI instance
> spinning while it's not consuming any requests.
> 
> Zoli

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