Hi Peter,
On 2/3/20 7:19 PM, Peter Xu wrote:
> On Mon, Feb 03, 2020 at 06:46:36PM +0100, Auger Eric wrote:
>> 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.
>
> IMHO that's two different things. g_tree_destroy() should be the same
> as g_tree_unref() here when the tree is empty. It's really a matter
> of easy reading of code:
>
> void
> g_tree_destroy (GTree *tree)
> {
> g_return_if_fail (tree != NULL);
>
> g_tree_remove_all (tree);
> g_tree_unref (tree);
> }
>
> What we really changed here is to allow the ref/unref to be clearly
> paired, i.e., for each EP it'll ref once and unref once. The prvious
> solution has the trick in that the 1st EP don't ref, the latter EPs
> ref, and when the domain quits it doesn't unref to match the first
> ref. It's error prone to me. Then, if we can do it in the paired way
> easily, I don't see why not...
OK. I will respin according to your suggestion.
Thanks
Eric
>
> Thanks,
>
