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: Mon, 24 Mar 2014 23:55:58 +0000 Message-ID: <5330C60E.7060501@citrix.com> 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> <532D9B9B.20705@schaman.hu> <20140322171451.GA78509@deinos.phlegethon.org> <532FEF1602000078000012A1@nat28.tlf.novell.com> <533023C5.8070704@citrix.com> <9AAE0902D5BC7E449B7C8E4E778ABCD02977EA@AMSPEX01CL01.citrite.net> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta14.messagelabs.com ([193.109.254.103]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1WSEir-0000Wb-PP for xen-devel@lists.xenproject.org; Mon, 24 Mar 2014 23:56:06 +0000 In-Reply-To: <9AAE0902D5BC7E449B7C8E4E778ABCD02977EA@AMSPEX01CL01.citrite.net> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Paul Durrant , Jan Beulich , Zoltan Kiss , "Tim (Xen.org)" Cc: "Keir (Xen.org)" , Ian Campbell , "freebsd-xen@freebsd.org" , Alan Somers , Manuel Bouyer , Stefano Stabellini , David Vrabel , "xen-devel@lists.xenproject.org" , Wei Liu , Boris Ostrovsky , Roger Pau Monne , John Suykerbuyk List-Id: xen-devel@lists.xenproject.org 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@freebsd.org; Alan Somers; Paul Durrant; David Vrabel; xen- >> devel@lists.xenproject.org; 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, 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. 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. Zoli > > 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