Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756133AbcK2Lal (ORCPT ); Tue, 29 Nov 2016 06:30:41 -0500 Received: from smtp02.citrix.com ([66.165.176.63]:62373 "EHLO SMTP02.CITRIX.COM" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755222AbcK2L3Q (ORCPT ); Tue, 29 Nov 2016 06:29:16 -0500 X-IronPort-AV: E=Sophos;i="5.31,568,1473120000"; d="scan'208";a="400653370" Subject: Re: [Xen-devel] [PATCH] xen/scsifront: don't advance ring request pointer in case of error To: Juergen Gross , Jan Beulich References: <20161125202650.GK6266@mwanda> <20161129105013.7419-1-jgross@suse.com> <583D712F0200007800123283@suse.com> CC: , , , , , , , From: David Vrabel Message-ID: Date: Tue, 29 Nov 2016 11:28:39 +0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Icedove/45.4.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 1414 Lines: 36 On 29/11/16 11:19, Juergen Gross wrote: > On 29/11/16 12:14, Jan Beulich wrote: >>>>> On 29.11.16 at 11:50, wrote: >>> --- a/drivers/scsi/xen-scsifront.c >>> +++ b/drivers/scsi/xen-scsifront.c >>> @@ -184,8 +184,6 @@ static struct vscsiif_request *scsifront_pre_req(struct vscsifrnt_info *info) >>> >>> ring_req = RING_GET_REQUEST(&(info->ring), ring->req_prod_pvt); >>> >>> - ring->req_prod_pvt++; >> >> Please note the "_pvt" suffix, which stands for "private": This field is >> not visible to the backend. Only ring->sring fields are shared, and >> the updating of the shared field happens in RING_PUSH_REQUESTS() >> and RING_PUSH_REQUESTS_AND_CHECK_NOTIFY(). > > Sure, but RING_PUSH_REQUESTS() will copy req_prod_pvt to req_prod. In > the case corrected this would advance req_prod by two after the error > case before, even if only one request would have made it to the ring. > > As an alternative I could have decremented req_prod_pvt in case of an > error, but I like my current solution better. FWIW, I found the commit message a bit misleading and also came to the same conclusion as Jan initially. Perhaps, "When adding a new request to the ring, an error may cause the (partially constructed) request to be discarded and used for the next. Thus ring->req_prod_pvt should not be advanced until we know the request will be successfully added to the ring." Or similar. David