| Message ID | 20231226200205.562565-11-pasha.tatashin@soleen.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | IOMMU memory observability | expand |
On Tue, 26 Dec 2023, Pasha Tatashin wrote: > In order to be able to limit the amount of memory that is allocated > by IOMMU subsystem, the memory must be accounted. > > Account IOMMU as part of the secondary pagetables as it was discussed > at LPC. > > The value of SecPageTables now contains mmeory allocation by IOMMU > and KVM. > > Signed-off-by: Pasha Tatashin <pasha.tatashin@soleen.com> Acked-by: David Rientjes <rientjes@google.com>
> > SecPageTables > > - Memory consumed by secondary page tables, this currently > > - currently includes KVM mmu allocations on x86 and arm64. > > + Memory consumed by secondary page tables, this currently includes > > + KVM mmu and IOMMU allocations on x86 and arm64. Hi Will, > While I can see the value in this for IOMMU mappings managed by VFIO, > doesn't this end up conflating that with the normal case of DMA domains? > For systems that e.g. rely on an IOMMU for functional host DMA, it seems > wrong to subject that to accounting constraints. The accounting constraints are only applicable when GFP_KERNEL_ACCOUNT is passed to the iommu mapping functions. We do that from the vfio, iommufd, and vhost. Without this flag, the memory useage is reported in /proc/meminfo as part of SecPageTables field, but not constrained in cgroup. Pasha
On Tue, Feb 13, 2024 at 10:44:53AM -0500, Pasha Tatashin wrote: > > > SecPageTables > > > - Memory consumed by secondary page tables, this currently > > > - currently includes KVM mmu allocations on x86 and arm64. > > > + Memory consumed by secondary page tables, this currently includes > > > + KVM mmu and IOMMU allocations on x86 and arm64. > > Hi Will, > > > While I can see the value in this for IOMMU mappings managed by VFIO, > > doesn't this end up conflating that with the normal case of DMA domains? > > For systems that e.g. rely on an IOMMU for functional host DMA, it seems > > wrong to subject that to accounting constraints. > > The accounting constraints are only applicable when GFP_KERNEL_ACCOUNT > is passed to the iommu mapping functions. We do that from the vfio, > iommufd, and vhost. Without this flag, the memory useage is reported > in /proc/meminfo as part of SecPageTables field, but not constrained > in cgroup. Thanks, Pasha, that explanation makes sense. I still find it bizarre to include IOMMU allocations from the DMA API in SecPageTables though, and I worry that it will confuse people who are using that metric as a way to get a feeling for how much memory is being used by KVM's secondary page-tables. As an extreme example, having a non-zero SecPageTables count without KVM even compiled in is pretty bizarre. Will
On Fri, Feb 16, 2024 at 12:58 PM Will Deacon <will@kernel.org> wrote: > > On Tue, Feb 13, 2024 at 10:44:53AM -0500, Pasha Tatashin wrote: > > > > SecPageTables > > > > - Memory consumed by secondary page tables, this currently > > > > - currently includes KVM mmu allocations on x86 and arm64. > > > > + Memory consumed by secondary page tables, this currently includes > > > > + KVM mmu and IOMMU allocations on x86 and arm64. > > > > Hi Will, > > > > > While I can see the value in this for IOMMU mappings managed by VFIO, > > > doesn't this end up conflating that with the normal case of DMA domains? > > > For systems that e.g. rely on an IOMMU for functional host DMA, it seems > > > wrong to subject that to accounting constraints. > > > > The accounting constraints are only applicable when GFP_KERNEL_ACCOUNT > > is passed to the iommu mapping functions. We do that from the vfio, > > iommufd, and vhost. Without this flag, the memory useage is reported > > in /proc/meminfo as part of SecPageTables field, but not constrained > > in cgroup. > > Thanks, Pasha, that explanation makes sense. I still find it bizarre to > include IOMMU allocations from the DMA API in SecPageTables though, and > I worry that it will confuse people who are using that metric as a way > to get a feeling for how much memory is being used by KVM's secondary > page-tables. As an extreme example, having a non-zero SecPageTables count > without KVM even compiled in is pretty bizarre. I agree; I also prefer a new field in /proc/meminfo named 'IOMMUPageTables'. This is what I proposed at LPC, but I was asked to reuse the existing 'SecPageTables' field instead. The rationale was that 'secondary' implies not only KVM page tables, but any other non-regular page tables. I would appreciate the opinion of IOMMU maintainers on this: is it preferable to bundle the information with 'SecPageTables' or maintain a separate field? Pasha
On Fri, Feb 16, 2024 at 02:48:00PM -0500, Pasha Tatashin wrote: > On Fri, Feb 16, 2024 at 12:58 PM Will Deacon <will@kernel.org> wrote: > > > > On Tue, Feb 13, 2024 at 10:44:53AM -0500, Pasha Tatashin wrote: > > > > > SecPageTables > > > > > - Memory consumed by secondary page tables, this currently > > > > > - currently includes KVM mmu allocations on x86 and arm64. > > > > > + Memory consumed by secondary page tables, this currently includes > > > > > + KVM mmu and IOMMU allocations on x86 and arm64. > > > > > > Hi Will, > > > > > > > While I can see the value in this for IOMMU mappings managed by VFIO, > > > > doesn't this end up conflating that with the normal case of DMA domains? > > > > For systems that e.g. rely on an IOMMU for functional host DMA, it seems > > > > wrong to subject that to accounting constraints. > > > > > > The accounting constraints are only applicable when GFP_KERNEL_ACCOUNT > > > is passed to the iommu mapping functions. We do that from the vfio, > > > iommufd, and vhost. Without this flag, the memory useage is reported > > > in /proc/meminfo as part of SecPageTables field, but not constrained > > > in cgroup. > > > > Thanks, Pasha, that explanation makes sense. I still find it bizarre to > > include IOMMU allocations from the DMA API in SecPageTables though, and > > I worry that it will confuse people who are using that metric as a way > > to get a feeling for how much memory is being used by KVM's secondary > > page-tables. As an extreme example, having a non-zero SecPageTables count > > without KVM even compiled in is pretty bizarre. > > I agree; I also prefer a new field in /proc/meminfo named > 'IOMMUPageTables'. This is what I proposed at LPC, but I was asked to > reuse the existing 'SecPageTables' field instead. The rationale was > that 'secondary' implies not only KVM page tables, but any other > non-regular page tables. > > I would appreciate the opinion of IOMMU maintainers on this: is it > preferable to bundle the information with 'SecPageTables' or maintain > a separate field? I personally find it confusing to add all IOMMU page-table allocations to SecPageTables, considering that userspace could be using that today with a reasonable expectation that it's concerned only with virtual machine overhead. However, if the opposite conclusion was reached at LPC, then I really don't want to re-open the discussion and derail your patchset. Will
On Fri, Feb 16, 2024 at 02:48:00PM -0500, Pasha Tatashin wrote: > On Fri, Feb 16, 2024 at 12:58 PM Will Deacon <will@kernel.org> wrote: > > > > On Tue, Feb 13, 2024 at 10:44:53AM -0500, Pasha Tatashin wrote: > > > > > SecPageTables > > > > > - Memory consumed by secondary page tables, this currently > > > > > - currently includes KVM mmu allocations on x86 and arm64. > > > > > + Memory consumed by secondary page tables, this currently includes > > > > > + KVM mmu and IOMMU allocations on x86 and arm64. > > > > > > Hi Will, > > > > > > > While I can see the value in this for IOMMU mappings managed by VFIO, > > > > doesn't this end up conflating that with the normal case of DMA domains? > > > > For systems that e.g. rely on an IOMMU for functional host DMA, it seems > > > > wrong to subject that to accounting constraints. > > > > > > The accounting constraints are only applicable when GFP_KERNEL_ACCOUNT > > > is passed to the iommu mapping functions. We do that from the vfio, > > > iommufd, and vhost. Without this flag, the memory useage is reported > > > in /proc/meminfo as part of SecPageTables field, but not constrained > > > in cgroup. > > > > Thanks, Pasha, that explanation makes sense. I still find it bizarre to > > include IOMMU allocations from the DMA API in SecPageTables though, and > > I worry that it will confuse people who are using that metric as a way > > to get a feeling for how much memory is being used by KVM's secondary > > page-tables. As an extreme example, having a non-zero SecPageTables count > > without KVM even compiled in is pretty bizarre. > > I agree; I also prefer a new field in /proc/meminfo named > 'IOMMUPageTables'. This is what I proposed at LPC, but I was asked to > reuse the existing 'SecPageTables' field instead. The rationale was > that 'secondary' implies not only KVM page tables, but any other > non-regular page tables. Right, SeanC mentioned that the purpose of SecPageTables was to capture all non-mm page table radix allocations. > I would appreciate the opinion of IOMMU maintainers on this: is it > preferable to bundle the information with 'SecPageTables' or maintain > a separate field? I think you should keep them together. I don't think we should be introducing new counters, in general. Detailed memory profile should come from some kind of more dynamic and universal scheme. Hopefully that other giant thread about profiling will reach some conclusion. Jason
> > > > > While I can see the value in this for IOMMU mappings managed by VFIO, > > > > > doesn't this end up conflating that with the normal case of DMA domains? > > > > > For systems that e.g. rely on an IOMMU for functional host DMA, it seems > > > > > wrong to subject that to accounting constraints. > > > > > > > > The accounting constraints are only applicable when GFP_KERNEL_ACCOUNT > > > > is passed to the iommu mapping functions. We do that from the vfio, > > > > iommufd, and vhost. Without this flag, the memory useage is reported > > > > in /proc/meminfo as part of SecPageTables field, but not constrained > > > > in cgroup. > > > > > > Thanks, Pasha, that explanation makes sense. I still find it bizarre to > > > include IOMMU allocations from the DMA API in SecPageTables though, and > > > I worry that it will confuse people who are using that metric as a way > > > to get a feeling for how much memory is being used by KVM's secondary > > > page-tables. As an extreme example, having a non-zero SecPageTables count > > > without KVM even compiled in is pretty bizarre. > > > > I agree; I also prefer a new field in /proc/meminfo named > > 'IOMMUPageTables'. This is what I proposed at LPC, but I was asked to > > reuse the existing 'SecPageTables' field instead. The rationale was > > that 'secondary' implies not only KVM page tables, but any other > > non-regular page tables. > > Right, SeanC mentioned that the purpose of SecPageTables was to > capture all non-mm page table radix allocations. > > > I would appreciate the opinion of IOMMU maintainers on this: is it > > preferable to bundle the information with 'SecPageTables' or maintain > > a separate field? > > I think you should keep them together. I don't think we should be > introducing new counters, in general. Thanks Jason, I will keep it as-is. I will send a new version soon with your comments addressed. > Detailed memory profile should come from some kind of more dynamic and > universal scheme. Hopefully that other giant thread about profiling > will reach some conclusion. +1! Memory profiling is going to be a very useful addition to the kernel. Pasha
diff --git a/Documentation/admin-guide/cgroup-v2.rst b/Documentation/admin-guide/cgroup-v2.rst index 3f85254f3cef..e004e05a7cde 100644 --- a/Documentation/admin-guide/cgroup-v2.rst +++ b/Documentation/admin-guide/cgroup-v2.rst @@ -1418,7 +1418,7 @@ PAGE_SIZE multiple when read back. sec_pagetables Amount of memory allocated for secondary page tables, this currently includes KVM mmu allocations on x86 - and arm64. + and arm64 and IOMMU page tables. percpu (npn) Amount of memory used for storing per-cpu kernel diff --git a/Documentation/filesystems/proc.rst b/Documentation/filesystems/proc.rst index 49ef12df631b..86f137a9b66b 100644 --- a/Documentation/filesystems/proc.rst +++ b/Documentation/filesystems/proc.rst @@ -1110,8 +1110,8 @@ KernelStack PageTables Memory consumed by userspace page tables SecPageTables - Memory consumed by secondary page tables, this currently - currently includes KVM mmu allocations on x86 and arm64. + Memory consumed by secondary page tables, this currently includes + KVM mmu and IOMMU allocations on x86 and arm64. NFS_Unstable Always zero. Previous counted pages which had been written to the server, but has not been committed to stable storage. diff --git a/drivers/iommu/iommu-pages.h b/drivers/iommu/iommu-pages.h index 4e70cdf7acac..b4289d577e2c 100644 --- a/drivers/iommu/iommu-pages.h +++ b/drivers/iommu/iommu-pages.h @@ -27,6 +27,7 @@ static inline void __iommu_alloc_account(struct page *page, int order) const long pgcnt = 1l << order; mod_node_page_state(page_pgdat(page), NR_IOMMU_PAGES, pgcnt); + mod_lruvec_page_state(page, NR_SECONDARY_PAGETABLE, pgcnt); } /** @@ -39,6 +40,7 @@ static inline void __iommu_free_account(struct page *page, int order) const long pgcnt = 1l << order; mod_node_page_state(page_pgdat(page), NR_IOMMU_PAGES, -pgcnt); + mod_lruvec_page_state(page, NR_SECONDARY_PAGETABLE, -pgcnt); } /** diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h index f0b54c752e22..da68f9977206 100644 --- a/include/linux/mmzone.h +++ b/include/linux/mmzone.h @@ -199,7 +199,7 @@ enum node_stat_item { NR_KERNEL_SCS_KB, /* measured in KiB */ #endif NR_PAGETABLE, /* used for pagetables */ - NR_SECONDARY_PAGETABLE, /* secondary pagetables, e.g. KVM pagetables */ + NR_SECONDARY_PAGETABLE, /* secondary pagetables, KVM & IOMMU */ #ifdef CONFIG_IOMMU_SUPPORT NR_IOMMU_PAGES, /* # of pages allocated by IOMMU */ #endif
In order to be able to limit the amount of memory that is allocated by IOMMU subsystem, the memory must be accounted. Account IOMMU as part of the secondary pagetables as it was discussed at LPC. The value of SecPageTables now contains mmeory allocation by IOMMU and KVM. Signed-off-by: Pasha Tatashin <pasha.tatashin@soleen.com> --- Documentation/admin-guide/cgroup-v2.rst | 2 +- Documentation/filesystems/proc.rst | 4 ++-- drivers/iommu/iommu-pages.h | 2 ++ include/linux/mmzone.h | 2 +- 4 files changed, 6 insertions(+), 4 deletions(-)