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

From: "Jan Beulich" <JBeulich@suse.com>
To: Paul Durrant <Paul.Durrant@citrix.com>,
	Zoltan Kiss <zoltan.kiss@schaman.hu>,
	"Tim (Xen.org)" <tim@xen.org>
Cc: Wei Liu <wei.liu2@citrix.com>,
	Ian Campbell <Ian.Campbell@citrix.com>,
	"Keir(Xen.org)" <keir@xen.org>,
	"freebsd-xen@freebsd.org" <freebsd-xen@freebsd.org>,
	Alan Somers <alans@spectralogic.com>,
	ManuelBouyer <bouyer@NetBSD.org>,
	Stefano Stabellini <Stefano.Stabellini@citrix.com>,
	David Vrabel <david.vrabel@citrix.com>,
	"xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>,
	Boris Ostrovsky <boris.ostrovsky@oracle.com>,
	John Suykerbuyk <johns@spectralogic.com>,
	Roger Pau Monne <roger.pau@citrix.com>
Subject: Re: [PATCH RFC] xen/public/ring.h: simplify RING_HAS_UNCONSUMED_REQUESTS()
Date: Mon, 24 Mar 2014 09:59:38 +0000	[thread overview]
Message-ID: <5330101A0200007800001422@nat28.tlf.novell.com> (raw)
In-Reply-To: <9AAE0902D5BC7E449B7C8E4E778ABCD0296A52@AMSPEX01CL01.citrite.net>

>>> On 24.03.14 at 10:39, <Paul.Durrant@citrix.com> wrote:
>>  -----Original Message-----
>> From: Jan Beulich [mailto:JBeulich@suse.com]
>> 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@freebsd.org; xen-
>> devel@lists.xenproject.org; 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@xen.org> 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.

> 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.

Jan