linux-kernel - Re: [Xen-devel] [PATCH v3 4/7] xen/9pfs: connect to the backend

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20170317151357.GS7915@char.us.oracle.com>
Date:   Fri, 17 Mar 2017 11:13:57 -0400
From:   Konrad Rzeszutek Wilk <konrad.wilk@...cle.com>
To:     Juergen Gross <jgross@...e.com>
Cc:     Stefano Stabellini <sstabellini@...nel.org>,
        Latchesar Ionkov <lucho@...kov.net>,
        Eric Van Hensbergen <ericvh@...il.com>,
        linux-kernel@...r.kernel.org,
        Stefano Stabellini <stefano@...reto.com>,
        v9fs-developer@...ts.sourceforge.net,
        Ron Minnich <rminnich@...dia.gov>,
        xen-devel@...ts.xenproject.org, boris.ostrovsky@...cle.com
Subject: Re: [Xen-devel] [PATCH v3 4/7] xen/9pfs: connect to the backend

On Fri, Mar 17, 2017 at 05:54:47AM +0100, Juergen Gross wrote:
> 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 <stefano@...reto.com>
> >>>>>>> CC: boris.ostrovsky@...cle.com
> >>>>>>> CC: jgross@...e.com
> >>>>>>> CC: Eric Van Hensbergen <ericvh@...il.com>
> >>>>>>> CC: Ron Minnich <rminnich@...dia.gov>
> >>>>>>> CC: Latchesar Ionkov <lucho@...kov.net>
> >>>>>>> CC: v9fs-developer@...ts.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

Ewww.


How about https://xenbits.xen.org/docs/unstable/misc/9pfs.html

which gets updated daily.

> > 
> > 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
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@...ts.xen.org
> https://lists.xen.org/xen-devel