Re: [PATCH v13 03/10] virtio-iommu: Implement attach/detach command

[Auger Eric <eric.auger@redhat.com>]

Hi Peter,

On 2/3/20 4:19 PM, Peter Xu wrote:
> On Mon, Feb 03, 2020 at 03:59:00PM +0100, Auger Eric wrote:
> 
> [...]
> 
>>>> +static void virtio_iommu_detach_endpoint_from_domain(VirtIOIOMMUEndpoint *ep)
>>>> +{
>>>> +    QLIST_REMOVE(ep, next);
>>>> +    g_tree_unref(ep->domain->mappings);
>>>
>>> Here domain->mapping is unreferenced for each endpoint, while at [1]
>>> below you only reference the domain->mappings if it's the first
>>> endpoint.  Is that problematic?
>> in [1] I take a ref to the domain->mappings if it is *not* the 1st
>> endpoint. This aims at deleting the mappings gtree when the last EP is
>> detached from the domain.
>>
>> This fixes the issue reported by Jean in:
>> https://patchwork.kernel.org/patch/11258267/#23046313
> 
> Ah OK. :)
> 
> However this is tricky.  How about do explicit g_tree_destroy() in
> virtio_iommu_detach() when it's the last endpoint?  I see that you
> have:
> 
>     /*
>      * when the last EP is detached, simply remove the domain for
>      * the domain list and destroy it. Note its mappings were already
>      * freed by the ref count mechanism. Next operation involving
>      * the same domain id will re-create one domain object.
>      */
>     if (QLIST_EMPTY(&domain->endpoint_list)) {
>         g_tree_remove(s->domains, GUINT_TO_POINTER(domain->id));
>     }
> 
> Then it becomes:
> 
>     if (QLIST_EMPTY(&domain->endpoint_list)) {
>         g_tree_destroy(domain->mappings);
>         g_tree_remove(s->domains, GUINT_TO_POINTER(domain->id));
>     }
> 
> And also remove the trick in attach() so you take the domain ref
> unconditionally.  Would that work?
Yes I think so. On the other hand this ref counting mechanism is also
made for that purpose of destroying objects without being forced to
explicitly call the destroy function.

Thanks

Eric
> 
> Thanks,
>