Re: [PATCH v3 4/7] xen/9pfs: connect to the backend

[Juergen Gross]

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@xxxxxxxxxxx>
>>>>>>> CC: boris.ostrovsky@xxxxxxxxxx
>>>>>>> CC: jgross@xxxxxxxx
>>>>>>> CC: Eric Van Hensbergen <ericvh@xxxxxxxxx>
>>>>>>> CC: Ron Minnich <rminnich@xxxxxxxxxx>
>>>>>>> CC: Latchesar Ionkov <lucho@xxxxxxxxxx>
>>>>>>> CC: v9fs-developer@xxxxxxxxxxxxxxxxxxxxx
>>>>>>> ---
>>>>
>>>>>> 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