Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754611AbdCHVVW (ORCPT ); Wed, 8 Mar 2017 16:21:22 -0500 Received: from mail.kernel.org ([198.145.29.136]:40230 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754464AbdCHVVT (ORCPT ); Wed, 8 Mar 2017 16:21:19 -0500 Date: Wed, 8 Mar 2017 12:56:17 -0800 (PST) From: Stefano Stabellini X-X-Sender: sstabellini@sstabellini-ThinkPad-X260 To: Boris Ostrovsky cc: Stefano Stabellini , xen-devel@lists.xenproject.org, linux-kernel@vger.kernel.org, Stefano Stabellini , jgross@suse.com, Eric Van Hensbergen , Ron Minnich , Latchesar Ionkov , v9fs-developer@lists.sourceforge.net Subject: Re: [PATCH 5/7] xen/9pfs: send requests to the backend In-Reply-To: <0d2ab9ca-47b1-700a-741e-82ffdc7bcfad@oracle.com> Message-ID: References: <1488830488-18506-1-git-send-email-sstabellini@kernel.org> <1488830488-18506-5-git-send-email-sstabellini@kernel.org> <5351d729-6b53-aa30-55e8-dd3f55324831@oracle.com> <0d2ab9ca-47b1-700a-741e-82ffdc7bcfad@oracle.com> User-Agent: Alpine 2.10 (DEB 1266 2009-07-14) MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2904 Lines: 79 On Wed, 8 Mar 2017, Boris Ostrovsky wrote: > On 03/08/2017 02:33 PM, Stefano Stabellini wrote: > > On Wed, 8 Mar 2017, Boris Ostrovsky wrote: > >>>>> +} > >>>>> + > >>>>> +static int p9_xen_write_todo(struct xen_9pfs_dataring *ring, RING_IDX size) > >>>>> +{ > >>>>> + RING_IDX cons, prod; > >>>>> + > >>>>> + cons = ring->intf->out_cons; > >>>>> + prod = ring->intf->out_prod; > >>>>> + mb(); > >>>>> + > >>>>> + if (XEN_9PFS_RING_SIZE - xen_9pfs_queued(prod, cons, XEN_9PFS_RING_SIZE) >= size) > >>>>> + return 1; > >>>>> + else > >>>>> + return 0; > >>>>> } > >>>>> > >>>>> static int p9_xen_request(struct p9_client *client, struct p9_req_t *p9_req) > >>>>> { > >>>>> + struct xen_9pfs_front_priv *priv = NULL; > >>>>> + RING_IDX cons, prod, masked_cons, masked_prod; > >>>>> + unsigned long flags; > >>>>> + uint32_t size = p9_req->tc->size; > >>>>> + struct xen_9pfs_dataring *ring; > >>>>> + int num; > >>>>> + > >>>>> + list_for_each_entry(priv, &xen_9pfs_devs, list) { > >>>>> + if (priv->client == client) > >>>>> + break; > >>>>> + } > >>>>> + if (priv == NULL || priv->client != client) > >>>>> + return -EINVAL; > >>>>> + > >>>>> + num = p9_req->tc->tag % priv->num_rings; > >>>>> + ring = &priv->rings[num]; > >>>>> + > >>>>> +again: > >>>>> + while (wait_event_interruptible(ring->wq, > >>>>> + p9_xen_write_todo(ring, size) > 0) != 0); > >>>>> + > >>>>> + spin_lock_irqsave(&ring->lock, flags); > >>>>> + cons = ring->intf->out_cons; > >>>>> + prod = ring->intf->out_prod; > >>>>> + mb(); > >>>>> + > >>>>> + if (XEN_9PFS_RING_SIZE - xen_9pfs_queued(prod, cons, XEN_9PFS_RING_SIZE) < size) { > >>>> This looks like p9_xen_write_todo(). > >>> p9_xen_write_todo is just a wrapper around xen_9pfs_queued to provide > >>> a return value that works well with wait_event_interruptible. > >>> > >>> I would prefer not to call p9_xen_write_todo here, because it's simpler > >>> if we don't read prod and cons twice. > >> I was referring to the whole code fragment after spin_lock_irqsave(), > >> not just the last line. Isn't it exactly !p9_xen_write_todo()? > > Yes, it is true they are almost the same. The difference, and the reason > > for p9_xen_write_todo to exist, is that p9_xen_write_todo is called in > > the wait_event_interruptible loop, as such it needs to read prod and > > cons every time. On the other end, here we want to read them once. Does > > it make sense? > > > I am clearly being particularly dense here but what I was thinking was: > > again: > while (wait_event_interruptible(ring->wq, > p9_xen_write_todo(ring, size) > 0) != 0); > > spin_lock_irqsave(&ring->lock, flags); > if (!p9_xen_write_todo(ring, size)) { > spin_unlock_irqrestore(&ring->lock, flags); > goto again; > } > > There is no extra read of prod/cons. Yes, there are: just after this if statement we would have to read them again to calculate masked_prod and masked_cons.