| Message ID | 20220211150042.11972-1-luca.fancellu@arm.com (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
| Series | [RFC] arm/vgic-v3: provide custom callbacks for pend_lpi_tree radix tree | expand |
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. Cheers,
> On 11 Feb 2022, at 15:26, Julien Grall <julien@xen.org> 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. Cheers, Luca > Cheers, > > -- > Julien Grall
On 11/02/2022 15:45, Luca Fancellu wrote: > > >> On 11 Feb 2022, at 15:26, Julien Grall <julien@xen.org> 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. Cheers,
> On 11 Feb 2022, at 16:12, Julien Grall <julien@xen.org> wrote: > > > > On 11/02/2022 15:45, Luca Fancellu wrote: >>> On 11 Feb 2022, at 15:26, Julien Grall <julien@xen.org> 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
diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c index 65bb7991a69b..970747a72012 100644 --- a/xen/arch/arm/vgic-v3.c +++ b/xen/arch/arm/vgic-v3.c @@ -1650,6 +1650,18 @@ static inline unsigned int vgic_v3_max_rdist_count(struct domain *d) GUEST_GICV3_RDIST_REGIONS; } +static struct radix_tree_node *vgic_v3_radix_tree_node_alloc(void *arg) +{ + struct radix_tree_node *node = xmalloc(struct radix_tree_node); + + return node ? node : NULL; +} + +static void vgic_v3_radix_tree_node_free(struct radix_tree_node *elem, void *arg) +{ + xfree(elem); +} + static int vgic_v3_domain_init(struct domain *d) { struct vgic_rdist_region *rdist_regions; @@ -1668,6 +1680,14 @@ static int vgic_v3_domain_init(struct domain *d) rwlock_init(&d->arch.vgic.pend_lpi_tree_lock); radix_tree_init(&d->arch.vgic.pend_lpi_tree); + /* + * pend_lpi_tree is protected by rwlock, so don't use lockless RCU default + * management for it and provide callbacks to alloc/free elements. + */ + radix_tree_set_alloc_callbacks(&d->arch.vgic.pend_lpi_tree, + vgic_v3_radix_tree_node_alloc, + vgic_v3_radix_tree_node_free, NULL); + /* * Domain 0 gets the hardware address. * Guests get the virtual platform layout.
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. This commit provides custom callbacks to alloc/free items of the radix tree and the free function doesn't use the RCU mechanism. Signed-off-by: Luca Fancellu <luca.fancellu@arm.com> --- xen/arch/arm/vgic-v3.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)