Re: [Qemu-devel] [PATCH V8 1/4] mem: add share parameter to memory-backend-ram

[Marcel Apfelbaum]

On 01/02/2018 20:18, Eduardo Habkost wrote:
> On Thu, Feb 01, 2018 at 07:58:10PM +0200, Marcel Apfelbaum wrote:
>> On 01/02/2018 19:36, Eduardo Habkost wrote:
>>> On Thu, Feb 01, 2018 at 07:12:45PM +0200, Michael S. Tsirkin wrote:
>>>> On Thu, Feb 01, 2018 at 03:01:36PM -0200, Eduardo Habkost wrote:
>>>>> On Thu, Feb 01, 2018 at 06:59:07PM +0200, Michael S. Tsirkin wrote:
>>>>>> On Thu, Feb 01, 2018 at 02:57:39PM -0200, Eduardo Habkost wrote:
>>>>>>> On Thu, Feb 01, 2018 at 06:48:54PM +0200, Michael S. Tsirkin wrote:
>>>>>>>> On Thu, Feb 01, 2018 at 02:31:32PM -0200, Eduardo Habkost wrote:
>>>>>>>>> On Thu, Feb 01, 2018 at 04:24:30PM +0200, Michael S. Tsirkin wrote:
>>>>>>>>> [...]
>>>>>>>>>> The full fix would be to allow QEMU to map a list of
>>>>>>>>>> pages to a guest supplied IOVA.
>>>>>>>>>
>>>>>>>>> Thanks, that's what I expected.
>>>>>>>>>
>>>>>>>>> While this is not possible, the only requests I have for this
>>>>>>>>> patch is that we clearly document:
>>>>>>>>> * What's the only purpose of share=on on a host-memory-backend
>>>>>>>>>   object (due to pvrdma limitations).
>>>>>>>>> * The potential undesirable side-effects of setting share=on.
>>>>>>>>> * On the commit message and other comments, clearly distinguish
>>>>>>>>>   HVAs in the QEMU address-space from IOVAs, to avoid confusion.
>>>>>>>>
>>>>>>>> Looking forward, when we do support it, how will management find out
>>>>>>>> it no longer needs to pass the share parameter?
>>>>>>>>
>>>>>>>> Further, if the side effects of the share parameter go away,
>>>>>>>> how will it know these no longer hold?
>>>>>>>
>>>>>>> A query-host-capabilities or similar QMP command seems necessary
>>>>>>> for that.
>>>>>>
>>>>>> Is anyone working on that?
>>>>>
>>>>> Not yet.
>>>>>
>>>>> -- 
>>>>> Eduardo
>>>>
>>>> Do these patches need to wait until we do have that command?
>>>
>>> I don't think so.  The command will be needed only when
>>> support for pvrdma without share=on gets implemented.
>>>
>>> Right now, all we need is clear documentation.
>>>
>>>>
>>>> I'm thinking it's better to have "share=on required with rdma"
>>>> and "hugetlbfs not supported with rdma"
>>>> than the reverse, this way new hosts do not need to carry
>>>> thus stuff around forever.
>>>
>>> What do you mean by "the reverse"?
>>>
>>> IIUC, the requirements/limitations are:
>>>
>>> * share=on required for pvrdma.  Already documented and enforced
>>>   by pvrdma code in this series.
>>
>> Right.
>>
>>> * hugetlbfs not supported with rdma. Is this detected/reported by
>>>   QEMU?  Is it documented?
>>
>> Yes, enforced by the pvrdma device initialization and documented in the
>> corresponding pvrdma doc.
>>
>>> * side-effects of share=on.  This is not detected nor documented,
>>>   and probably already applies to other memory backends.
>>>   * Nice to have: document when share=on is useful (answer:
>>>     because of pvrdma), when adding share=on support to
>>>     host-memory-backend.
>>>
>>
>> The documentation is part of the pvrdma doc.
>> What are the side-effects of share=on? I missed that.
>> (share=on is new for the memory backed RAM, the file
>> backed RAM already had the share parameter)
>>
>> One can just grep for "share=on" in the docs directory
>> and can easily see the only current usage. But maybe will
>> be more, maybe we don't want to limit it for now.
>>
>> I am planning to re-spin today/tomorrow before sending
>> a pull-request, can you please point me on what documentation
>> to add and what side-effects I should document?
>>
> 
> The full list of side-effects is not clear to me.  For some of
> them, see Documentation/vm/numa_memory_policy.txt on the kernel
> tree.
> 
> The documentation for memory backend options is at
> qemu-options.hx.  Maybe something like this, extending the
> existing paragraph:
> 
>   The @option{share} boolean option determines whether the memory
>   region is marked as private to QEMU, or shared (mapped using
>   the MAP_SHARED flag).  The latter allows a co-operating
>   external process to access the QEMU memory region.
> 
>   @option{share} is also required for pvrdma devices due to
>   limitations in the RDMA API provided by Linux.
> 
>   Setting share=on might affect the ability to configure NUMA
>   bindings for the memory backend under some circumstances, see
>   Documentation/vm/numa_memory_policy.txt on the Linux kernel
>   source tree for additional details.
> 
> I hate to point users to low-level documentation on the kernel
> tree, but it's better than nothing.
> 
> We also need to list "share" as a valid option at the
> "@item -object memory-backend-ram,[...]" line.
> 

Thanks for the help Eduardo, I'll be sure to update the docs as
advised.
Marcel