Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754223AbdCHTlU (ORCPT ); Wed, 8 Mar 2017 14:41:20 -0500 Received: from mail.kernel.org ([198.145.29.136]:55576 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752986AbdCHTlT (ORCPT ); Wed, 8 Mar 2017 14:41:19 -0500 Date: Wed, 8 Mar 2017 11:33:16 -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: 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> 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: 2183 Lines: 62 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?