From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752655AbdCPSDt (ORCPT ); Thu, 16 Mar 2017 14:03:49 -0400 Received: from mail.kernel.org ([198.145.29.136]:56082 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751954AbdCPSDs (ORCPT ); Thu, 16 Mar 2017 14:03:48 -0400 Date: Thu, 16 Mar 2017 11:03:43 -0700 (PDT) From: Stefano Stabellini X-X-Sender: sstabellini@sstabellini-ThinkPad-X260 To: Juergen Gross cc: Stefano Stabellini , 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 Subject: Re: [PATCH v3 4/7] xen/9pfs: connect to the backend In-Reply-To: Message-ID: References: <1489449019-13343-1-git-send-email-sstabellini@kernel.org> <1489449019-13343-4-git-send-email-sstabellini@kernel.org> 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 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.