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

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: 24 March 2014 10:00
> To: Paul Durrant; Zoltan Kiss; Tim (Xen.org)
> Cc: David Vrabel; Ian Campbell; Roger Pau Monne; Stefano Stabellini; Wei Liu;
> freebsd-xen@xxxxxxxxxxx; xen-devel@xxxxxxxxxxxxxxxxxxxx; ManuelBouyer;
> Boris Ostrovsky; Konrad Rzeszutek Wilk; Alan Somers; John Suykerbuyk; Keir
> (Xen.org)
> Subject: RE: [PATCH RFC] xen/public/ring.h: simplify
> RING_HAS_UNCONSUMED_REQUESTS()
> 
> >>> On 24.03.14 at 10:39, <Paul.Durrant@xxxxxxxxxx> wrote:
> >>  -----Original Message-----
> >> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> >> Sent: 24 March 2014 07:39
> >> To: Zoltan Kiss; Tim (Xen.org)
> >> Cc: David Vrabel; Ian Campbell; Paul Durrant; Roger Pau Monne; Wei Liu;
> >> Stefano Stabellini; freebsd-xen@xxxxxxxxxxx; xen-
> >> devel@xxxxxxxxxxxxxxxxxxxx; Manuel Bouyer; Boris Ostrovsky; Konrad
> >> Rzeszutek Wilk; Alan Somers; John Suykerbuyk; Keir (Xen.org)
> >> Subject: Re: [PATCH RFC] xen/public/ring.h: simplify
> >> RING_HAS_UNCONSUMED_REQUESTS()
> >>
> >> >>> 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.
> >>
> >> Yet of course I can see that weakening the protection we have had
> >> in place for so many years may result in very undesirable fallout.
> >>
> >
> > But these are, of course, macros and so the protection is baked into any old
> > code.
> 
> Right, but the reduced protection won't be noticeable on a re-build,
> and syncing up header files may be a purely mechanical (perhaps even
> automated) operation.
> 

True. I have an auto-sync script for the Windows PV drivers but every time I 
update the tag to pull from I do check the diffs in the headers (not least 
because I have to post-process them to change any use of 'long' or 'unsigned 
long' to something that's actually 64-bits wide in 64-bit Windows).

> > I'm still in favour of changing the macro in the canonical header and
> > adding a comment to point out that older versions of the macro had the
> extra
> > check.
> 
> I too am in favor of changing the canonical header; I only wanted to
> point out that this isn't without risk of regressions.
> 

Yes. Fair enough.

  Paul

> Jan


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