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

[Juergen Gross]

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.


Juergen