| Message ID | 20231128204938.1453583-9-pasha.tatashin@soleen.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | IOMMU memory observability | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
On 2023-11-28 8:49 pm, Pasha Tatashin wrote: > Convert iommu/fsl_pamu.c to use the new page allocation functions > provided in iommu-pages.h. Again, this is not a pagetable. This thing doesn't even *have* pagetables. Similar to patches #1 and #2 where you're lumping in configuration tables which belong to the IOMMU driver itself, as opposed to pagetables which effectively belong to an IOMMU domain's user. But then there are still drivers where you're *not* accounting similar configuration structures, so I really struggle to see how this metric is useful when it's so completely inconsistent in what it's counting :/ Thanks, Robin. > Signed-off-by: Pasha Tatashin <pasha.tatashin@soleen.com> > --- > drivers/iommu/fsl_pamu.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/iommu/fsl_pamu.c b/drivers/iommu/fsl_pamu.c > index f37d3b044131..7bfb49940f0c 100644 > --- a/drivers/iommu/fsl_pamu.c > +++ b/drivers/iommu/fsl_pamu.c > @@ -16,6 +16,7 @@ > #include <linux/platform_device.h> > > #include <asm/mpc85xx.h> > +#include "iommu-pages.h" > > /* define indexes for each operation mapping scenario */ > #define OMI_QMAN 0x00 > @@ -828,7 +829,7 @@ static int fsl_pamu_probe(struct platform_device *pdev) > (PAGE_SIZE << get_order(OMT_SIZE)); > order = get_order(mem_size); > > - p = alloc_pages(GFP_KERNEL | __GFP_ZERO, order); > + p = __iommu_alloc_pages(GFP_KERNEL, order); > if (!p) { > dev_err(dev, "unable to allocate PAACT/SPAACT/OMT block\n"); > ret = -ENOMEM; > @@ -916,7 +917,7 @@ static int fsl_pamu_probe(struct platform_device *pdev) > iounmap(guts_regs); > > if (ppaact) > - free_pages((unsigned long)ppaact, order); > + iommu_free_pages(ppaact, order); > > ppaact = NULL; >
On Tue, Nov 28, 2023 at 5:53 PM Robin Murphy <robin.murphy@arm.com> wrote: > > On 2023-11-28 8:49 pm, Pasha Tatashin wrote: > > Convert iommu/fsl_pamu.c to use the new page allocation functions > > provided in iommu-pages.h. > > Again, this is not a pagetable. This thing doesn't even *have* pagetables. > > Similar to patches #1 and #2 where you're lumping in configuration > tables which belong to the IOMMU driver itself, as opposed to pagetables > which effectively belong to an IOMMU domain's user. But then there are > still drivers where you're *not* accounting similar configuration > structures, so I really struggle to see how this metric is useful when > it's so completely inconsistent in what it's counting :/ The whole IOMMU subsystem allocates a significant amount of kernel locked memory that we want to at least observe. The new field in vmstat does just that: it reports ALL buddy allocator memory that IOMMU allocates. However, for accounting purposes, I agree, we need to do better, and separate at least iommu pagetables from the rest. We can separate the metric into two: iommu pagetable only iommu everything or into three: iommu pagetable only iommu dma iommu everything What do you think? Pasha > > Thanks, > Robin. > > > Signed-off-by: Pasha Tatashin <pasha.tatashin@soleen.com> > > --- > > drivers/iommu/fsl_pamu.c | 5 +++-- > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/iommu/fsl_pamu.c b/drivers/iommu/fsl_pamu.c > > index f37d3b044131..7bfb49940f0c 100644 > > --- a/drivers/iommu/fsl_pamu.c > > +++ b/drivers/iommu/fsl_pamu.c > > @@ -16,6 +16,7 @@ > > #include <linux/platform_device.h> > > > > #include <asm/mpc85xx.h> > > +#include "iommu-pages.h" > > > > /* define indexes for each operation mapping scenario */ > > #define OMI_QMAN 0x00 > > @@ -828,7 +829,7 @@ static int fsl_pamu_probe(struct platform_device *pdev) > > (PAGE_SIZE << get_order(OMT_SIZE)); > > order = get_order(mem_size); > > > > - p = alloc_pages(GFP_KERNEL | __GFP_ZERO, order); > > + p = __iommu_alloc_pages(GFP_KERNEL, order); > > if (!p) { > > dev_err(dev, "unable to allocate PAACT/SPAACT/OMT block\n"); > > ret = -ENOMEM; > > @@ -916,7 +917,7 @@ static int fsl_pamu_probe(struct platform_device *pdev) > > iounmap(guts_regs); > > > > if (ppaact) > > - free_pages((unsigned long)ppaact, order); > > + iommu_free_pages(ppaact, order); > > > > ppaact = NULL; > >
On Tue, Nov 28, 2023 at 06:00:13PM -0500, Pasha Tatashin wrote: > On Tue, Nov 28, 2023 at 5:53 PM Robin Murphy <robin.murphy@arm.com> wrote: > > > > On 2023-11-28 8:49 pm, Pasha Tatashin wrote: > > > Convert iommu/fsl_pamu.c to use the new page allocation functions > > > provided in iommu-pages.h. > > > > Again, this is not a pagetable. This thing doesn't even *have* pagetables. > > > > Similar to patches #1 and #2 where you're lumping in configuration > > tables which belong to the IOMMU driver itself, as opposed to pagetables > > which effectively belong to an IOMMU domain's user. But then there are > > still drivers where you're *not* accounting similar configuration > > structures, so I really struggle to see how this metric is useful when > > it's so completely inconsistent in what it's counting :/ > > The whole IOMMU subsystem allocates a significant amount of kernel > locked memory that we want to at least observe. The new field in > vmstat does just that: it reports ALL buddy allocator memory that > IOMMU allocates. However, for accounting purposes, I agree, we need to > do better, and separate at least iommu pagetables from the rest. > > We can separate the metric into two: > iommu pagetable only > iommu everything > > or into three: > iommu pagetable only > iommu dma > iommu everything > > What do you think? I think I said this at LPC - if you want to have fine grained accounting of memory by owner you need to go talk to the cgroup people and come up with something generic. Adding ever open coded finer category breakdowns just for iommu doesn't make alot of sense. You can make some argument that the pagetable memory should be counted because kvm counts it's shadow memory, but I wouldn't go into further detail than that with hand coded counters.. Jason
On 28/11/2023 11:50 pm, Jason Gunthorpe wrote: > On Tue, Nov 28, 2023 at 06:00:13PM -0500, Pasha Tatashin wrote: >> On Tue, Nov 28, 2023 at 5:53 PM Robin Murphy <robin.murphy@arm.com> wrote: >>> >>> On 2023-11-28 8:49 pm, Pasha Tatashin wrote: >>>> Convert iommu/fsl_pamu.c to use the new page allocation functions >>>> provided in iommu-pages.h. >>> >>> Again, this is not a pagetable. This thing doesn't even *have* pagetables. >>> >>> Similar to patches #1 and #2 where you're lumping in configuration >>> tables which belong to the IOMMU driver itself, as opposed to pagetables >>> which effectively belong to an IOMMU domain's user. But then there are >>> still drivers where you're *not* accounting similar configuration >>> structures, so I really struggle to see how this metric is useful when >>> it's so completely inconsistent in what it's counting :/ >> >> The whole IOMMU subsystem allocates a significant amount of kernel >> locked memory that we want to at least observe. The new field in >> vmstat does just that: it reports ALL buddy allocator memory that >> IOMMU allocates. However, for accounting purposes, I agree, we need to >> do better, and separate at least iommu pagetables from the rest. >> >> We can separate the metric into two: >> iommu pagetable only >> iommu everything >> >> or into three: >> iommu pagetable only >> iommu dma >> iommu everything >> >> What do you think? > > I think I said this at LPC - if you want to have fine grained > accounting of memory by owner you need to go talk to the cgroup people > and come up with something generic. Adding ever open coded finer > category breakdowns just for iommu doesn't make alot of sense. > > You can make some argument that the pagetable memory should be counted > because kvm counts it's shadow memory, but I wouldn't go into further > detail than that with hand coded counters.. Right, pagetable memory is interesting since it's something that any random kernel user can indirectly allocate via iommu_domain_alloc() and iommu_map(), and some of those users may even be doing so on behalf of userspace. I have no objection to accounting and potentially applying limits to *that*. Beyond that, though, there is nothing special about "the IOMMU subsystem". The amount of memory an IOMMU driver needs to allocate for itself in order to function is not of interest beyond curiosity, it just is what it is; limiting it would only break the IOMMU, and if a user thinks it's "too much", the only actionable thing that might help is to physically remove devices from the system. Similar for DMA buffers; it might be intriguing to account those, but it's not really an actionable metric - in the overwhelming majority of cases you can't simply tell a driver to allocate less than what it needs. And that is of course assuming if we were to account *all* DMA buffers, since whether they happen to have an IOMMU translation or not is irrelevant (we'd have already accounted the pagetables as pagetables if so). I bet "the networking subsystem" also consumes significant memory on the same kind of big systems where IOMMU pagetables would be of any concern. I believe some of the some of the "serious" NICs can easily run up hundreds of megabytes if not gigabytes worth of queues, SKB pools, etc. - would you propose accounting those too? Thanks, Robin.
> >> We can separate the metric into two: > >> iommu pagetable only > >> iommu everything > >> > >> or into three: > >> iommu pagetable only > >> iommu dma > >> iommu everything > >> > >> What do you think? > > > > I think I said this at LPC - if you want to have fine grained > > accounting of memory by owner you need to go talk to the cgroup people > > and come up with something generic. Adding ever open coded finer > > category breakdowns just for iommu doesn't make alot of sense. > > > > You can make some argument that the pagetable memory should be counted > > because kvm counts it's shadow memory, but I wouldn't go into further > > detail than that with hand coded counters.. > > Right, pagetable memory is interesting since it's something that any > random kernel user can indirectly allocate via iommu_domain_alloc() and > iommu_map(), and some of those users may even be doing so on behalf of > userspace. I have no objection to accounting and potentially applying > limits to *that*. Yes, in the next version, I will separate pagetable only from the rest, for the limits. > Beyond that, though, there is nothing special about "the IOMMU > subsystem". The amount of memory an IOMMU driver needs to allocate for > itself in order to function is not of interest beyond curiosity, it just > is what it is; limiting it would only break the IOMMU, and if a user Agree about the amount of memory IOMMU allocates for itself, but that should be small, if it is not, we have to at least show where the memory is used. > thinks it's "too much", the only actionable thing that might help is to > physically remove devices from the system. Similar for DMA buffers; it > might be intriguing to account those, but it's not really an actionable > metric - in the overwhelming majority of cases you can't simply tell a > driver to allocate less than what it needs. And that is of course > assuming if we were to account *all* DMA buffers, since whether they > happen to have an IOMMU translation or not is irrelevant (we'd have > already accounted the pagetables as pagetables if so). DMA mappings should be observable (do not have to be limited). At the very least, it can help with explaining the kernel memory overhead anomalies on production systems. > I bet "the networking subsystem" also consumes significant memory on the It does, and GPU drivers also may consume a significant amount of memory. > same kind of big systems where IOMMU pagetables would be of any concern. > I believe some of the some of the "serious" NICs can easily run up > hundreds of megabytes if not gigabytes worth of queues, SKB pools, etc. > - would you propose accounting those too? Yes. Any kind of kernel memory that is proportional to the workload should be accountable. Someone is using those resources compared to the idling system, and that someone should be charged. Pasha
On Wed, Nov 29, 2023 at 02:45:03PM -0500, Pasha Tatashin wrote: > > same kind of big systems where IOMMU pagetables would be of any concern. > > I believe some of the some of the "serious" NICs can easily run up > > hundreds of megabytes if not gigabytes worth of queues, SKB pools, etc. > > - would you propose accounting those too? > > Yes. Any kind of kernel memory that is proportional to the workload > should be accountable. Someone is using those resources compared to > the idling system, and that someone should be charged. There is a difference between charged and accounted You should be running around adding GFP_KERNEL_ACCOUNT, yes. I already did a bunch of that work. Split that out from this series and send it to the right maintainers. Adding a counter for allocations and showing in procfs is a very different question. IMHO that should not be done in micro, the threshold to add a new counter should be high. There is definately room for a generic debugging feature to break down GFP_KERNEL_ACCOUNT by owernship somehow. Maybe it can already be done with BPF. IDK Jason
On Wed, Nov 29, 2023 at 3:03 PM Jason Gunthorpe <jgg@ziepe.ca> wrote: > > On Wed, Nov 29, 2023 at 02:45:03PM -0500, Pasha Tatashin wrote: > > > > same kind of big systems where IOMMU pagetables would be of any concern. > > > I believe some of the some of the "serious" NICs can easily run up > > > hundreds of megabytes if not gigabytes worth of queues, SKB pools, etc. > > > - would you propose accounting those too? > > > > Yes. Any kind of kernel memory that is proportional to the workload > > should be accountable. Someone is using those resources compared to > > the idling system, and that someone should be charged. > > There is a difference between charged and accounted > > You should be running around adding GFP_KERNEL_ACCOUNT, yes. I already > did a bunch of that work. Split that out from this series and send it > to the right maintainers. I will do that. > > Adding a counter for allocations and showing in procfs is a very > different question. IMHO that should not be done in micro, the > threshold to add a new counter should be high. I agree, /proc/meminfo, should not include everything, however overall network consumption that includes memory allocated by network driver would be useful to have, may be it should be exported by device drivers and added to the protocol memory. We already have network protocol memory consumption in procfs: # awk '{printf "%-10s %s\n", $1, $4}' /proc/net/protocols | grep -v '\-1' protocol memory UDPv6 22673 TCPv6 16961 > There is definately room for a generic debugging feature to break down > GFP_KERNEL_ACCOUNT by owernship somehow. Maybe it can already be done > with BPF. IDK
diff --git a/drivers/iommu/fsl_pamu.c b/drivers/iommu/fsl_pamu.c index f37d3b044131..7bfb49940f0c 100644 --- a/drivers/iommu/fsl_pamu.c +++ b/drivers/iommu/fsl_pamu.c @@ -16,6 +16,7 @@ #include <linux/platform_device.h> #include <asm/mpc85xx.h> +#include "iommu-pages.h" /* define indexes for each operation mapping scenario */ #define OMI_QMAN 0x00 @@ -828,7 +829,7 @@ static int fsl_pamu_probe(struct platform_device *pdev) (PAGE_SIZE << get_order(OMT_SIZE)); order = get_order(mem_size); - p = alloc_pages(GFP_KERNEL | __GFP_ZERO, order); + p = __iommu_alloc_pages(GFP_KERNEL, order); if (!p) { dev_err(dev, "unable to allocate PAACT/SPAACT/OMT block\n"); ret = -ENOMEM; @@ -916,7 +917,7 @@ static int fsl_pamu_probe(struct platform_device *pdev) iounmap(guts_regs); if (ppaact) - free_pages((unsigned long)ppaact, order); + iommu_free_pages(ppaact, order); ppaact = NULL;
Convert iommu/fsl_pamu.c to use the new page allocation functions provided in iommu-pages.h. Signed-off-by: Pasha Tatashin <pasha.tatashin@soleen.com> --- drivers/iommu/fsl_pamu.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)