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

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

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