From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751930AbdCQEyx (ORCPT ); Fri, 17 Mar 2017 00:54:53 -0400 Received: from mx2.suse.de ([195.135.220.15]:56141 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751284AbdCQEyv (ORCPT ); Fri, 17 Mar 2017 00:54:51 -0400 Subject: Re: [PATCH v3 4/7] xen/9pfs: connect to the backend To: Stefano Stabellini References: <1489449019-13343-1-git-send-email-sstabellini@kernel.org> <1489449019-13343-4-git-send-email-sstabellini@kernel.org> Cc: xen-devel@lists.xenproject.org, linux-kernel@vger.kernel.org, boris.ostrovsky@oracle.com, Stefano Stabellini , Eric Van Hensbergen , Ron Minnich , Latchesar Ionkov , v9fs-developer@lists.sourceforge.net From: Juergen Gross Message-ID: <0f602607-df9c-85e1-e7df-eca76b306679@suse.com> Date: Fri, 17 Mar 2017 05:54:47 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.7.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 16/03/17 19:03, Stefano Stabellini wrote: > On Thu, 16 Mar 2017, Juergen Gross wrote: >> On 15/03/17 19:44, Stefano Stabellini wrote: >>> On Wed, 15 Mar 2017, Juergen Gross wrote: >>>> On 14/03/17 22:22, Stefano Stabellini wrote: >>>>> Hi Juergen, >>>>> >>>>> thank you for the review! >>>>> >>>>> On Tue, 14 Mar 2017, Juergen Gross wrote: >>>>>> On 14/03/17 00:50, Stefano Stabellini wrote: >>>>>>> Implement functions to handle the xenbus handshake. Upon connection, >>>>>>> allocate the rings according to the protocol specification. >>>>>>> >>>>>>> Initialize a work_struct and a wait_queue. The work_struct will be used >>>>>>> to schedule work upon receiving an event channel notification from the >>>>>>> backend. The wait_queue will be used to wait when the ring is full and >>>>>>> we need to send a new request. >>>>>>> >>>>>>> Signed-off-by: Stefano Stabellini >>>>>>> CC: boris.ostrovsky@oracle.com >>>>>>> CC: jgross@suse.com >>>>>>> CC: Eric Van Hensbergen >>>>>>> CC: Ron Minnich >>>>>>> CC: Latchesar Ionkov >>>>>>> CC: v9fs-developer@lists.sourceforge.net >>>>>>> --- >>>> >>>>>> Did you think about using request_threaded_irq() instead of a workqueue? >>>>>> For an example see e.g. drivers/scsi/xen-scsifront.c >>>>> >>>>> I like workqueues :-) It might come down to personal preferences, but I >>>>> think workqueues are more flexible and a better fit for this use case. >>>>> Not only it is easy to schedule work in a workqueue from the interrupt >>>>> handler, but also they can be used for sleeping in the request function >>>>> if there is not enough room on the ring. Besides, they can easily be >>>>> configured to share a single thread or to have multiple independent >>>>> threads. >>>> >>>> I'm fine with the workqueues as long as you have decided to use them >>>> considering the alternatives. :-) >>>> >>>>>> Can't you use xenbus_read_unsigned() instead of xenbus_read()? >>>>> >>>>> I can use xenbus_read_unsigned in the other cases below, but not here, >>>>> because versions is in the form: "1,3,4" >>>> >>>> Is this documented somewhere? >>>> >>>> Hmm, are any of the Xenstore entries documented? Shouldn't this be done >>>> in xen_9pfs.h ? >>> >>> They are documented in docs/misc/9pfs.markdown, under "Xenstore". Given >>> that it's all written there, especially the semantics, I didn't repeat >>> it in xen_9pfs.h >> >> Looking at it from the Linux kernel perspective this documentation is >> not really highly visible. For me it is okay, but there have been >> multiple examples in the past where documentation in the Xen repository >> wasn't regarded as being sufficient. >> >> I recommend moving the documentation regarding the interface into the >> header file like for the other pv interfaces. > > What about adding a link such as: > > http://xenbits.xen.org/gitweb/?p=xen.git;a=blob_plain;f=docs/misc/9pfs.markdown;hb=HEAD > > that should be easily accessible, right? For other specifications, such > as 9p, only links are provided (see Documentation/filesystems/9p.txt). > I am suggesting a link, because then we are sure the specs don't go out > of sync. I realize that older PV protocols were described in header > files, but that was before Xen Project had a formal process for getting > new specifications accepted, and a formal place where to publish them. Fine with me. Lets see if other maintainers are okay with it, too. Juergen