[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()

On 24/03/14 13:52, Paul Durrant wrote:
-----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

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:

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.
My opinion is that the name of the macro implies the caller wants to know if there are unconsumed requests on the ring (and it doesn't imply it, but the return value is actually how many unconsumed requests you have). Returning req_prod - req_cons seems to be logical at first (and second and ...) thought, but - and that's not implied in the name indeed, but I think it's sensible, and should be documented in a comment - if you want to protect the backend from abusive frontends, you should check if req_prod is not overrunning rsp_prod_pvt. You could check rsp_prod, but rsp_prod_pvt is better, because the latter should be always equal to or ahead of the former, and from the backend point of view the responses between the two (if any) doesn't matter. It's not about the backend making sure it's rsp_prod_pvt is valid or not, it's about protection from a frontend. And I think it should be a general thing for the ring users to have this safety check, rather than something backend-specific, as all of them could be affected.



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


Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.