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