Re: [PATCH 1/6] xenbus: Support HVM backends

[Daniel De Graaf <dgdegra@tycho.nsa.gov>]

On 12/19/2011 01:16 PM, Konrad Rzeszutek Wilk wrote:
> On Mon, Dec 19, 2011 at 12:51:15PM -0500, Daniel De Graaf wrote:
>> On 12/16/2011 02:56 PM, Konrad Rzeszutek Wilk wrote:
>>> On Wed, Dec 14, 2011 at 03:12:09PM -0500, Daniel De Graaf wrote:
>>>> Add HVM implementations of xenbus_(map,unmap)_ring_v(alloc,free) so
>>>> that ring mappings can be done without using GNTMAP_contains_pte which
>>>> is not supported on HVM.
>>>>
>>>> Signed-off-by: Daniel De Graaf<dgdegra@tycho.nsa.gov>
>>>> ---
>>>>   drivers/xen/xenbus/xenbus_client.c |  155 +++++++++++++++++++++++++++++-------
>>>>   1 files changed, 125 insertions(+), 30 deletions(-)
>>>>
>>>> diff --git a/drivers/xen/xenbus/xenbus_client.c b/drivers/xen/xenbus/xenbus_client.c
>>>> index 1906125..ecd695d 100644
>>>> --- a/drivers/xen/xenbus/xenbus_client.c
>>>> +++ b/drivers/xen/xenbus/xenbus_client.c
>>>> @@ -32,16 +32,27 @@
>>>>
>>>>   #include<linux/slab.h>
>>>>   #include<linux/types.h>
>>>> +#include<linux/spinlock.h>
>>>>   #include<linux/vmalloc.h>
>>>>   #include<linux/export.h>
>>>>   #include<asm/xen/hypervisor.h>
>>>>   #include<asm/xen/page.h>
>>>>   #include<xen/interface/xen.h>
>>>>   #include<xen/interface/event_channel.h>
>>>> +#include<xen/balloon.h>
>>>>   #include<xen/events.h>
>>>>   #include<xen/grant_table.h>
>>>>   #include<xen/xenbus.h>
>>>>
>>>> +struct xenbus_map_node {
>>>> +	struct list_head next;
>>>> +	struct page *page;
>>>> +	grant_handle_t handle;
>>>> +};
>>>> +
>>>> +static DEFINE_SPINLOCK(xenbus_valloc_lock);
>>>> +static LIST_HEAD(xenbus_valloc_pages);
>>>
>>> Could we use this for what the PV version of xenbus_unmap_ring_vfree
>>> does? (where it uses the vmlist_lock to look for the appropiate vaddr).
>>>
>>> Could the vmlist_lock be removed and instead we can use this structure
>>> for both PV and HVM?
>>
>> Yes, the next version will do this.
>>
>> [...]
>>>> +
>>>> +/**
>>>> + * xenbus_map_ring_valloc
>>>> + * @dev: xenbus device
>>>> + * @gnt_ref: grant reference
>>>> + * @vaddr: pointer to address to be filled out by mapping
>>>> + *
>>>> + * Based on Rusty Russell's skeleton driver's map_page.
>>>> + * Map a page of memory into this domain from another domain's grant table.
>>>> + * xenbus_map_ring_valloc allocates a page of virtual address space, maps the
>>>> + * page to that address, and sets *vaddr to that address.
>>>> + * Returns 0 on success, and GNTST_* (see xen/include/interface/grant_table.h)
>>>> + * or -ENOMEM on error. If an error is returned, device will switch to
>>>> + * XenbusStateClosing and the error message will be saved in XenStore.
>>>> + */
>>>> +int xenbus_map_ring_valloc(struct xenbus_device *dev, int gnt_ref, void **vaddr)
>>>> +{
>>>> +	if (xen_pv_domain())
>>>> +		return xenbus_map_ring_valloc_pv(dev, gnt_ref, vaddr);
>>>> +	else
>>>> +		return xenbus_map_ring_valloc_hvm(dev, gnt_ref, vaddr);
>>>
>>> This could be done in a similar way which Annie's granttable v2 patches are done.
>>>
>>> Meaning define something like:
>>>
>>> struct xenbus_ring_ops {
>>> 	int (*map)(struct xenbus_device *dev, int gnt, void *vaddr);
>>> 	...
>>>
>>> And then define two variants of it (PV and HVM):
>>>
>>> struct xenbus_ring_ops pv_ring_ops = {
>>> 	.map = xenbus_map_ring_valloc_pv;
>>> 	..
>>> }
>>>
>>> have a static to which either one will be assigned:
>>>
>>> static struct xenbus_ring_ops *ring_ops;
>>>
>>> And in the init function:
>>>
>>> void __init xenbus_ring_ops_init(void)
>>> {
>>> 	if (xen_hvm_domain)
>>> 		ring_ops = hvm_ring_ops;
>>> 	else
>>> 	...
>>>
>>> And then xenbus_init() calls the xenbus_rings_ops_init().
>>>
>>> Then these 'xenbus_map_ring_valloc' end up just using the
>>> ring_ops->map call.
>>
>> Is the reason for doing this just to avoid overhead? The overhead from
>> an indirect function call is much higher than from an integer comparison
>> (which is what xen_pv_domain resolves to). In this case, the _pv and _hvm
>> functions are both inlined into the dispatch function.
>
> Do we care about that? How often are these calls made?

Not all that often - domain creation and destruction or device plug/unplug.
So performance doesn't really matter. Is there a reason to prefer an _ops
structure for this instead of direct calls?