Re: [PATCH V1] vfio: add attach_group_by_node to control behavior of attaching group to domain

[Zhenguo Yao]

Understood, thanks！

Alex Williamson <alex.williamson@xxxxxxxxxx> 于2023年11月18日周六 06:27写道：
>
> On Wed, 15 Nov 2023 10:02:09 +0800
> yaozhenguo <yaozhenguo1@xxxxxxxxx> wrote:
>
> > From: Zhenguo Yao <yaozhenguo1@xxxxxxxxx>
> >
> > Groups will attach to one iommu_domain if ops and enforce_cache_coherency
> > are equal. And all the iommu hardware share one pagetable by default.
> > There are performance issue in some scenarios. For example:
> > Host hardware topopy:
> >
> > node0 + PCIe RP0 ---+ GPU A100
> >       |         |---+ GPU A100
> >       |               |---+ NIC Mellanox CX6
> >       |               |---+ NIC Mellanox CX6
> >       + PCIe RP1 ---+ GPU A100
> >                 |---+ GPU A100
> >                       |---+ NIC Mellanox CX6
> >                 |---+ NIC Mellanox CX6
> > node1 + PCIe RP0 ---+ GPU A100
> >       |         |---+ GPU A100
> >       |               |---+ NIC Mellanox CX6
> >       |               |---+ NIC Mellanox CX6
> >       + PCIe RP1 ---+ GPU A100
> >                 |---+ GPU A100
> >                       |---+ NIC Mellanox CX6
> >                 |---+ NIC Mellanox CX6
> >
> > We passthrough all NICs and GPU to VM, and emulate host hardware topopy.
> > Mellanox CX6 ATS feature is enabled, GPU direct RDMA enabled.
> > We test NCCL allreduce in VM at different cases.
> >
> > Case1: allreduce test use 4nic and 4GPU in numa0.
> > Case2：allreduce test use 4nic and 4GPU in numa1.
> > case3: allreduce test use 8nic and 8GPU.
> >
> > the result are below:
> >
> > |        | algbw (GB/S) |
> > | ------ | -------------|
> > | case1  | 24           |
> > | case2  | 32           |
> > | case3  | 45           |
> >
> > We checked that IOMMU pagetable is allocated in numa1 when VM boot up.
> > So, if IOTLB miss happan, IOMMU hardware in numa0 will access remote
> > pagetable in numa1. This will drop performance. After apply this patch and
> > attach_group_by_node is 1. Group in same node will attach to one domain.
> > IOMMU will access there local pagetable. Performance is improved:
> >
> > |        | algbw (GB/S) |
> > | ------ | -------------|
> > | case1  | 32           |
> > | case2  | 32           |
> > | case3  | 63           |
> >
> > Signed-off-by: Zhenguo Yao <yaozhenguo1@xxxxxxxxx>
> > Co-developed-by: Wenchao Yao <yaowenchao@xxxxxx>
> > Signed-off-by: Wenchao Yao <yaowenchao@xxxxxx>
> > Co-developed-by: ZiHan Zhou <zhouzihan30@xxxxxx>
> > Signed-off-by: ZiHan Zhou <zhouzihan30@xxxxxx>
> > ---
> >  drivers/iommu/intel/iommu.c     |  8 +++++++-
> >  drivers/vfio/vfio_iommu_type1.c | 33 +++++++++++++++++++++------------
> >  include/linux/iommu.h           |  1 +
> >  3 files changed, 29 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> > index 3531b95..2c6d8f0 100644
> > --- a/drivers/iommu/intel/iommu.c
> > +++ b/drivers/iommu/intel/iommu.c
> > @@ -569,8 +569,10 @@ void domain_update_iommu_cap(struct dmar_domain *domain)
> >        * If RHSA is missing, we should default to the device numa domain
> >        * as fall back.
> >        */
> > -     if (domain->nid == NUMA_NO_NODE)
> > +     if (domain->nid == NUMA_NO_NODE) {
> >               domain->nid = domain_update_device_node(domain);
> > +             domain->domain.nid = domain->nid;
> > +     }
> >
> >       /*
> >        * First-level translation restricts the input-address to a
> > @@ -1767,6 +1769,7 @@ static struct dmar_domain *alloc_domain(unsigned int type)
> >               return NULL;
> >
> >       domain->nid = NUMA_NO_NODE;
> > +     domain->domain.nid = NUMA_NO_NODE;
> >       if (first_level_by_default(type))
> >               domain->use_first_level = true;
> >       domain->has_iotlb_device = false;
> > @@ -1808,6 +1811,8 @@ int domain_attach_iommu(struct dmar_domain *domain, struct intel_iommu *iommu)
> >       info->refcnt    = 1;
> >       info->did       = num;
> >       info->iommu     = iommu;
> > +     domain->nid     = iommu->node;
> > +     domain->domain.nid     = iommu->node;
> >       curr = xa_cmpxchg(&domain->iommu_array, iommu->seq_id,
> >                         NULL, info, GFP_ATOMIC);
> >       if (curr) {
> > @@ -1837,6 +1842,7 @@ void domain_detach_iommu(struct dmar_domain *domain, struct intel_iommu *iommu)
> >               clear_bit(info->did, iommu->domain_ids);
> >               xa_erase(&domain->iommu_array, iommu->seq_id);
> >               domain->nid = NUMA_NO_NODE;
> > +             domain->domain.nid = NUMA_NO_NODE;
> >               domain_update_iommu_cap(domain);
> >               kfree(info);
> >       }
> > diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
> > index eacd6ec..6a5641e 100644
> > --- a/drivers/vfio/vfio_iommu_type1.c
> > +++ b/drivers/vfio/vfio_iommu_type1.c
> > @@ -59,6 +59,11 @@
> >  module_param_named(dma_entry_limit, dma_entry_limit, uint, 0644);
> >  MODULE_PARM_DESC(dma_entry_limit,
> >                "Maximum number of user DMA mappings per container (65535).");
> > +static uint attach_group_by_node;
> > +module_param_named(attach_group_by_node,
> > +             attach_group_by_node, uint, 0644);
> > +MODULE_PARM_DESC(attach_group_by_node,
> > +              "Attach group to domain when it's in same node");
> >
> >  struct vfio_iommu {
> >       struct list_head        domain_list;
> > @@ -2287,19 +2292,23 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
> >               if (d->domain->ops == domain->domain->ops &&
> >                   d->enforce_cache_coherency ==
> >                           domain->enforce_cache_coherency) {
> > -                     iommu_detach_group(domain->domain, group->iommu_group);
> > -                     if (!iommu_attach_group(d->domain,
> > -                                             group->iommu_group)) {
> > -                             list_add(&group->next, &d->group_list);
> > -                             iommu_domain_free(domain->domain);
> > -                             kfree(domain);
> > -                             goto done;
> > -                     }
> > +                     if ((attach_group_by_node == 1 &&
> > +                             d->domain->nid == domain->domain->nid) ||
> > +                             attach_group_by_node == 0) {
> > +                             iommu_detach_group(domain->domain, group->iommu_group);
> > +                             if (!iommu_attach_group(d->domain,
> > +                                                     group->iommu_group)) {
> > +                                     list_add(&group->next, &d->group_list);
> > +                                     iommu_domain_free(domain->domain);
> > +                                     kfree(domain);
> > +                                     goto done;
> > +                             }
> >
> > -                     ret = iommu_attach_group(domain->domain,
> > -                                              group->iommu_group);
> > -                     if (ret)
> > -                             goto out_domain;
> > +                             ret = iommu_attach_group(domain->domain,
> > +                                             group->iommu_group);
> > +                             if (ret)
> > +                                     goto out_domain;
> > +                     }
> >               }
> >       }
> >
> > diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> > index ec289c1..c1330ed 100644
> > --- a/include/linux/iommu.h
> > +++ b/include/linux/iommu.h
> > @@ -123,6 +123,7 @@ struct iommu_domain {
> >                       int users;
> >               };
> >       };
> > +     int nid;
> >  };
> >
> >  static inline bool iommu_is_dma_domain(struct iommu_domain *domain)
>
> As I understand what's being done here, we're duplicating
> dmar_domain.nid to iommu_domain.nid, then when enabled by this new
> module option, we'll use this node id as part of the match to determine
> whether to create a new domain within the same container context or
> re-use an existing domain, which may have non-favorably locality.
>
> If we're going to implement a node id on the iommu_domain, it should
> replace the existing use of node id in the device specific structure
> and not simply duplicate it.  This should also account for non-VT-d use
> cases as well, for example AMD IOMMU also has a nid field on their
> protection_domain structure.  Alternatively this might be implemented
> through iommu_domain_ops so we could query the node association for a
> domain.
>
> I question whether we need this solution at all though.  AIUI the
> initial domain is allocated in proximity to the initial group.  The
> problem comes when the user asks to add an additional group into the
> same container.  Another valid solution would be that the user
> recognizes that these groups are not within the same locality and
> creates a separate container for this group.  In fact, if we're using
> QEMU here and created a q35 VM with vIOMMU, each device would have a
> separate address space and therefore a separate container and we'd
> already avoid the issue this patch tries to solve.
>
> Separate containers per QEMU AddressSpace are a requirement, but QEMU
> might also implement a policy to not re-use vfio containers between
> virtual nodes such that if each locality were mapped to separate PXBs
> with unique proximities, then simply reflecting the physical locality
> into the VM would be sufficient to avoid this non-optimal domain
> allocation placement.
>
> In any case, the type1 vfio IOMMU backend is in the early stages of
> deprecation, so any choices we make here would also need to be reflected
> in IOMMUFD, both in the compatibility and native interfaces.  Thanks,
>
> Alex
>