Re: [Xen-devel] [PATCH] xen: reuse the same pirq allocated when driver load first time

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

[-- Attachment #1: Type: text/plain, Size: 2976 bytes --]

> Looking at the hypervisor code I couldn't see anything obviously wrong.

I think the culprit is "physdev_unmap_pirq":

   if ( is_hvm_domain(d) )                                                     
    {                                                                           
        spin_lock(&d->event_lock);                                              
        gdprintk(XENLOG_WARNING,"d%d, pirq: %d is %x %s, irq: %d\n",            
            d->domain_id, pirq, domain_pirq_to_emuirq(d, pirq),                 
            domain_pirq_to_emuirq(d, pirq) == IRQ_UNBOUND ? "unbound" : "",        
            domain_pirq_to_irq(d, pirq));                                       
                                                                                
        if ( domain_pirq_to_emuirq(d, pirq) != IRQ_UNBOUND )                    
            ret = unmap_domain_pirq_emuirq(d, pirq);                            
        spin_unlock(&d->event_lock);                                            
        if ( domid == DOMID_SELF || ret )                                       
            goto free_domain;                                             

It always tells me unbound:

(XEN) physdev.c:237:d14 14, pirq: 54 is ffffffff
(XEN) irq.c:1873:d14 14, nr_pirqs: 56
(XEN) physdev.c:237:d14 14, pirq: 53 is ffffffff
(XEN) irq.c:1873:d14 14, nr_pirqs: 56
(XEN) physdev.c:237:d14 14, pirq: 52 is ffffffff
(XEN) irq.c:1873:d14 14, nr_pirqs: 56
(XEN) physdev.c:237:d14 14, pirq: 51 is ffffffff
(XEN) irq.c:1873:d14 14, nr_pirqs: 56
(XEN) physdev.c:237:d14 14, pirq: 50 is ffffffff
(XEN) irq.c:1873:d14 14, nr_pirqs: 56
(a bit older debug code, so the 'unbound' does not show up here).

Which means that the call to unmap_domain_pirq_emuirq does not happen.
The checks in unmap_domain_pirq_emuirq also look to be depend
on the code being IRQ_UNBOUND.

In other words, all of that code looks to only clear things when
they are !IRQ_UNBOUND.

But the other logic (IRQ_UNBOUND) looks to be missing a removal
in the radix tree:

  if ( emuirq != IRQ_PT )                                                     
        radix_tree_delete(&d->arch.hvm_domain.emuirq_pirq, emuirq);             
                                                                        
And I think that is what is causing the leak - the radix tree
needs to be pruned? Or perhaps the allocate_pirq should check
the radix tree for IRQ_UNBOUND ones and re-use them?

>  I do note that Xen doesn't free the pirq until it has been unbound by
> the guest.  Xen will warn if the guest unmaps a pirq that is still bound
> ("domD: forcing unbind of pirq P").  Is this what is happening?  If so,

No. It does not b/c of this check in  physdev_unmap_pirq:

if ( domid == DOMID_SELF || ret )
   goto free_domain

which jumps over the call to unmap_domain_pirq.

> that would suggest a bug in the guest rather than the hypervisor.

No. But then I am not even using it. See attached module.

> 
> David

[-- Attachment #2: alloc_and_unmap.c --]
[-- Type: text/plain, Size: 1232 bytes --]

#include <linux/module.h>
#include <linux/kthread.h>
#include <linux/pagemap.h>
#include <linux/init.h>
#include <xen/xen.h>
#include <xen/page.h>
#include <asm/xen/hypervisor.h>
#include <xen/features.h>
#include <xen/events.h>

MODULE_AUTHOR("Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>");
MODULE_DESCRIPTION("alloc_and_unmap");
MODULE_LICENSE("GPL");
MODULE_VERSION("0.1");

static int do_it(void)
{
	int rc;
	struct physdev_get_free_pirq op_get_free_pirq;
	struct physdev_unmap_pirq unmap_irq;
	int pirq;

	op_get_free_pirq.type = MAP_PIRQ_TYPE_MSI;
 	rc = HYPERVISOR_physdev_op(PHYSDEVOP_get_free_pirq, &op_get_free_pirq);
	if (rc) {
		printk(KERN_WARNING "%s:%d rc:%d\n", __func__, __LINE__, rc);
		return rc;
	}
	pirq = op_get_free_pirq.pirq;
	unmap_irq.pirq = pirq;
	unmap_irq.domid = DOMID_SELF;
	rc = HYPERVISOR_physdev_op(PHYSDEVOP_unmap_pirq, &unmap_irq);
	if (rc) {
		printk(KERN_WARNING "unmap irq failed %d\n", rc);
		return rc;
	}
	printk("PIRQ: %d\n", pirq);
	return 0;
}
static int __init alloc_and_unmap_init(void)
{
	int i;

	for (i = 0; i < 10; i++)
		if (do_it())
			break;
	return 0;
}
static void __exit alloc_and_unmap_exit(void)
{
}
module_init(alloc_and_unmap_init);
module_exit(alloc_and_unmap_exit);