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

[Julien Grall <julien@xxxxxxx>]

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.

Cheers,

--
Julien Grall