Re: [RFC PATCH] arm/vgic-v3: provide custom callbacks for pend_lpi_tree radix tree

[Luca Fancellu <luca.fancellu@xxxxxxx>]

> On 11 Feb 2022, at 16:12, Julien Grall <julien@xxxxxxx> wrote:
> 
> 
> 
> On 11/02/2022 15:45, Luca Fancellu wrote:
>>> On 11 Feb 2022, at 15:26, Julien Grall <julien@xxxxxxx> wrote:
>>> 
>>> Hi Luca,
>>> 
>>> On 11/02/2022 15:00, Luca Fancellu wrote:
>>>> pend_lpi_tree is a radix tree used to store pending irqs, the tree is
>>>> protected by a lock for read/write operations.
>>>> Currently the radix tree default function to free items uses the
>>>> RCU mechanism, calling call_rcu and deferring the operation.
>>>> However every access to the structure is protected by the lock so we
>>>> can avoid using the default free function that, by using RCU,
>>>> increases memory usage and impacts the predictability of the system.
>>> 
>>> I understand goal but looking at the implementation of 
>>> vgic_v3_lpi_to_pending() (Copied below for convenience). We would release 
>>> the lock as soon as the look-up finish, yet the element is returned.
>>> 
>>> static struct pending_irq *vgic_v3_lpi_to_pending(struct domain *d,
>>>                                                  unsigned int lpi)
>>> {
>>>    struct pending_irq *pirq;
>>> 
>>>    read_lock(&d->arch.vgic.pend_lpi_tree_lock);
>>>    pirq = radix_tree_lookup(&d->arch.vgic.pend_lpi_tree, lpi);
>>>    read_unlock(&d->arch.vgic.pend_lpi_tree_lock);
>>> 
>>>    return pirq;
>>> }
>>> 
>>> So the lock will not protect us against removal. If you want to drop the 
>>> RCU, you will need to ensure the structure pending_irq is suitably 
>>> protected. I haven't check whether there are other locks that may suit us 
>>> here.
>>> 
>> Hi Julien,
>> Yes you are right! I missed that, sorry for the noise.
> 
> Actually,... I think I am wrong :/.
> 
> I thought the lock pend_lpi_tre_lock would protect pending_irq, but it only 
> protects the radix tree element (not the value).
> 
> The use in its_discard_event() seems to confirm that because the
> pending_irq is re-initialized as soon as it gets destroyed.
> 
> I would like a second opinion though.
> 

Hi Julien,

I think you are right, the structure itself is protected but the usage of the 
element not. I guess now it’s safe because RCU
is freeing it when no cpus are using it anymore.

 - radix_tree_lookup
   - vgic_v3_lpi_to_pending (return pointer to item)
     - lpi_to_pending (function pointer to vgic_v3_lpi_to_pending)
       - irq_to_pending (return pointer to item if it is lpi -> is_lpi(irq))

         - vgic_vcpu_inject_lpi
           - gicv3_do_LPI (rcu_lock_domain_by_id on domain)
             - gic_interrupt (do_LPI function pointer)
               - do_trap_irq
               - do_trap_fiq
           - its_handle_int
             - vgic_its_handle_cmds
               - vgic_v3_its_mmio_write
                 - handle_write
                   - try_handle_mmio
                     - do_trap_stage2_abort_guest
                       - do_trap_guest_sync

         - vgic_get_hw_irq_desc 
           - release_guest_irq 
             - arch_do_domctl (XEN_DOMCTL_unbind_pt_irq)
               - do_domctl
             - domain_vgic_free
               - arch_domain_destroy

         - gic_raise_inflight_irq (assert v->arch.vgic.lock)
         - gic_raise_guest_irq (assert v->arch.vgic.lock)
         - gic_update_one_lr (assert v->arch.vgic.lock, irq are disabled)
         - vgic_connect_hw_irq
           - gic_route_irq_to_guest (Assert !is_lpi)
           - gic_remove_irq_from_guest (Assert !is_lpi(virq))
         - vgic_migrate_irq (lock old->arch.vgic.lock)
         - arch_move_irqs (Assert not lpi in loop)
         - vgic_disable_irqs (lock v_target->arch.vgic.lock)
         - vgic_enable_irqs (lock v_target->arch.vgic.lock)
         - vgic_inject_irq (lock v->arch.vgic.lock)
         - vgic_evtchn_irq_pending (assert !is_lpi(v->domain->arch.evtchn_irq))
         - vgic_check_inflight_irqs_pending (lock v_target->arch.vgic.lock)

   - vgic_v3_lpi_get_priority (return value from pointer)
     - lpi_get_priority (function pointer to vgic_v3_lpi_get_priority)

 - radix_tree_delete
   - its_discard_event (lock vcpu->arch.vgic.lock)

>From a quick analysis I see there are path using that pointer who doesn’t 
>share any lock.

I will put on hold this patch.

Cheers,
Luca


> Cheers,
> 
> -- 
> Julien Grall