qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Auger Eric
Subject: Re: [PATCH v13 03/10] virtio-iommu: Implement attach/detach command
Date: Mon, 3 Feb 2020 18:46:36 +0100
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.4.0

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,
> 




reply via email to

[Prev in Thread] Current Thread [Next in Thread]