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

[Peter Xu]

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

Thanks,

-- 
Peter Xu