From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S971224AbdEZPc1 (ORCPT ); Fri, 26 May 2017 11:32:27 -0400 Received: from userp1040.oracle.com ([156.151.31.81]:40226 "EHLO userp1040.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750892AbdEZPcZ (ORCPT ); Fri, 26 May 2017 11:32:25 -0400 Subject: Re: [PATCH v2 06/18] xen/pvcalls: handle commands from the frontend To: Stefano Stabellini , xen-devel@lists.xen.org References: <1495236179-27776-1-git-send-email-sstabellini@kernel.org> <1495236179-27776-6-git-send-email-sstabellini@kernel.org> Cc: linux-kernel@vger.kernel.org, jgross@suse.com, Stefano Stabellini From: Boris Ostrovsky Message-ID: <73d522d3-bec2-09ef-e8ab-b2014ff87d5d@oracle.com> Date: Fri, 26 May 2017 11:32:11 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 MIME-Version: 1.0 In-Reply-To: <1495236179-27776-6-git-send-email-sstabellini@kernel.org> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-Source-IP: aserv0021.oracle.com [141.146.126.233] Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 05/19/2017 07:22 PM, Stefano Stabellini wrote: > + > static void pvcalls_back_work(struct work_struct *work) > { > + struct pvcalls_back_priv *priv = container_of(work, > + struct pvcalls_back_priv, register_work); > + int notify, notify_all = 0, more = 1; > + struct xen_pvcalls_request req; > + struct xenbus_device *dev = priv->dev; > + > + atomic_set(&priv->work, 1); > + > + while (more || !atomic_dec_and_test(&priv->work)) { > + while (RING_HAS_UNCONSUMED_REQUESTS(&priv->ring)) { > + RING_COPY_REQUEST(&priv->ring, > + priv->ring.req_cons++, > + &req); > + > + if (!pvcalls_back_handle_cmd(dev, &req)) { > + RING_PUSH_RESPONSES_AND_CHECK_NOTIFY( > + &priv->ring, notify); > + notify_all += notify; > + } > + } > + > + if (notify_all) > + notify_remote_via_irq(priv->irq); > + > + RING_FINAL_CHECK_FOR_REQUESTS(&priv->ring, more); > + } > } > > static irqreturn_t pvcalls_back_event(int irq, void *dev_id) > { > + struct xenbus_device *dev = dev_id; > + struct pvcalls_back_priv *priv = NULL; > + > + if (dev == NULL) > + return IRQ_HANDLED; > + > + priv = dev_get_drvdata(&dev->dev); > + if (priv == NULL) > + return IRQ_HANDLED; > + > + atomic_inc(&priv->work); I will paste you response here from v1 --- I thought I understood it and now I don't anymore. >> >> Is this really needed? We have a new entry on the ring, so the outer loop in >> pvcalls_back_work() will pick this up (by setting 'more'). > > This is to avoid race conditions. A notification could be delivered > after RING_FINAL_CHECK_FOR_REQUESTS is called, returning more == 0, but > before pvcalls_back_work completes. In that case, without priv->work, > pvcalls_back_work wouldn't be rescheduled because it is still running > and the work would be left undone. How is this different from the case when new work comes after the outer loop is done but we still haven't returned from pvcalls_back_work()? -boris > + queue_work(priv->wq, &priv->register_work); > + > return IRQ_HANDLED; > } >