From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751136AbdCPFys (ORCPT ); Thu, 16 Mar 2017 01:54:48 -0400 Received: from mx2.suse.de ([195.135.220.15]:35136 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750892AbdCPFyr (ORCPT ); Thu, 16 Mar 2017 01:54:47 -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: Date: Thu, 16 Mar 2017 06:54:12 +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 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. Juergen