From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754217AbdCHUFe convert rfc822-to-8bit (ORCPT ); Wed, 8 Mar 2017 15:05:34 -0500 Received: from aserp1050.oracle.com ([141.146.126.70]:27272 "EHLO aserp1050.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752986AbdCHUFb (ORCPT ); Wed, 8 Mar 2017 15:05:31 -0500 Subject: Re: [PATCH 5/7] xen/9pfs: send requests to the backend To: Stefano Stabellini 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> Cc: 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 From: Boris Ostrovsky Message-ID: <0d2ab9ca-47b1-700a-741e-82ffdc7bcfad@oracle.com> Date: Wed, 8 Mar 2017 15:02:50 -0500 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.6.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 8BIT X-Source-IP: aserp1040.oracle.com [141.146.126.69] Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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. -boris