| Message ID | 6-v1-6e8b3997c46d+89e-iommu_map_gfp_jgg@nvidia.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Let iommufd charge IOPTE allocations to the memory cgroup | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
> From: Jason Gunthorpe <jgg@nvidia.com> > Sent: Saturday, January 7, 2023 12:43 AM > > @@ -2676,7 +2676,7 @@ static int copy_context_table(struct intel_iommu > *iommu, > if (!old_ce) > goto out; > > - new_ce = alloc_pgtable_page(iommu->node); > + new_ce = alloc_pgtable_page(iommu->node, > GFP_KERNEL); GFP_ATOMIC
On Tue, Jan 17, 2023 at 03:35:08AM +0000, Tian, Kevin wrote: > > From: Jason Gunthorpe <jgg@nvidia.com> > > Sent: Saturday, January 7, 2023 12:43 AM > > > > @@ -2676,7 +2676,7 @@ static int copy_context_table(struct intel_iommu > > *iommu, > > if (!old_ce) > > goto out; > > > > - new_ce = alloc_pgtable_page(iommu->node); > > + new_ce = alloc_pgtable_page(iommu->node, > > GFP_KERNEL); > > GFP_ATOMIC Can't be: old_ce = memremap(old_ce_phys, PAGE_SIZE, MEMREMAP_WB); if (!old_ce) goto out; new_ce = alloc_pgtable_page(iommu->node, GFP_KERNEL); if (!new_ce) memremap() is sleeping. And the only caller is: ctxt_tbls = kcalloc(ctxt_table_entries, sizeof(void *), GFP_KERNEL); if (!ctxt_tbls) goto out_unmap; for (bus = 0; bus < 256; bus++) { ret = copy_context_table(iommu, &old_rt[bus], ctxt_tbls, bus, ext); Jason
> From: Jason Gunthorpe <jgg@nvidia.com> > Sent: Tuesday, January 17, 2023 9:30 PM > > On Tue, Jan 17, 2023 at 03:35:08AM +0000, Tian, Kevin wrote: > > > From: Jason Gunthorpe <jgg@nvidia.com> > > > Sent: Saturday, January 7, 2023 12:43 AM > > > > > > @@ -2676,7 +2676,7 @@ static int copy_context_table(struct > intel_iommu > > > *iommu, > > > if (!old_ce) > > > goto out; > > > > > > - new_ce = alloc_pgtable_page(iommu->node); > > > + new_ce = alloc_pgtable_page(iommu->node, > > > GFP_KERNEL); > > > > GFP_ATOMIC > > Can't be: > > old_ce = memremap(old_ce_phys, PAGE_SIZE, > MEMREMAP_WB); > if (!old_ce) > goto out; > > new_ce = alloc_pgtable_page(iommu->node, > GFP_KERNEL); > if (!new_ce) > > memremap() is sleeping. > > And the only caller is: > > ctxt_tbls = kcalloc(ctxt_table_entries, sizeof(void *), GFP_KERNEL); > if (!ctxt_tbls) > goto out_unmap; > > for (bus = 0; bus < 256; bus++) { > ret = copy_context_table(iommu, &old_rt[bus], > ctxt_tbls, bus, ext); > Yes, but the patch description says "Push the GFP_ATOMIC to all callers." implying it's purely a refactoring w/o changing those semantics. I'm fine with doing this change in this patch, but it should worth a clarification in the patch description.
On Wed, Jan 18, 2023 at 01:18:18AM +0000, Tian, Kevin wrote: > > From: Jason Gunthorpe <jgg@nvidia.com> > > Sent: Tuesday, January 17, 2023 9:30 PM > > > > On Tue, Jan 17, 2023 at 03:35:08AM +0000, Tian, Kevin wrote: > > > > From: Jason Gunthorpe <jgg@nvidia.com> > > > > Sent: Saturday, January 7, 2023 12:43 AM > > > > > > > > @@ -2676,7 +2676,7 @@ static int copy_context_table(struct > > intel_iommu > > > > *iommu, > > > > if (!old_ce) > > > > goto out; > > > > > > > > - new_ce = alloc_pgtable_page(iommu->node); > > > > + new_ce = alloc_pgtable_page(iommu->node, > > > > GFP_KERNEL); > > > > > > GFP_ATOMIC > > > > Can't be: > > > > old_ce = memremap(old_ce_phys, PAGE_SIZE, > > MEMREMAP_WB); > > if (!old_ce) > > goto out; > > > > new_ce = alloc_pgtable_page(iommu->node, > > GFP_KERNEL); > > if (!new_ce) > > > > memremap() is sleeping. > > > > And the only caller is: > > > > ctxt_tbls = kcalloc(ctxt_table_entries, sizeof(void *), GFP_KERNEL); > > if (!ctxt_tbls) > > goto out_unmap; > > > > for (bus = 0; bus < 256; bus++) { > > ret = copy_context_table(iommu, &old_rt[bus], > > ctxt_tbls, bus, ext); > > > > Yes, but the patch description says "Push the GFP_ATOMIC to all > callers." implying it's purely a refactoring w/o changing those > semantics. Sure, lets split the patch, it is a good idea Jason
diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c index 59df7e42fd533c..e3807776971563 100644 --- a/drivers/iommu/intel/iommu.c +++ b/drivers/iommu/intel/iommu.c @@ -362,12 +362,12 @@ static int __init intel_iommu_setup(char *str) } __setup("intel_iommu=", intel_iommu_setup); -void *alloc_pgtable_page(int node) +void *alloc_pgtable_page(int node, gfp_t gfp) { struct page *page; void *vaddr = NULL; - page = alloc_pages_node(node, GFP_ATOMIC | __GFP_ZERO, 0); + page = alloc_pages_node(node, gfp | __GFP_ZERO, 0); if (page) vaddr = page_address(page); return vaddr; @@ -612,7 +612,7 @@ struct context_entry *iommu_context_addr(struct intel_iommu *iommu, u8 bus, if (!alloc) return NULL; - context = alloc_pgtable_page(iommu->node); + context = alloc_pgtable_page(iommu->node, GFP_ATOMIC); if (!context) return NULL; @@ -935,7 +935,7 @@ static struct dma_pte *pfn_to_dma_pte(struct dmar_domain *domain, if (!dma_pte_present(pte)) { uint64_t pteval; - tmp_page = alloc_pgtable_page(domain->nid); + tmp_page = alloc_pgtable_page(domain->nid, GFP_ATOMIC); if (!tmp_page) return NULL; @@ -1186,7 +1186,7 @@ static int iommu_alloc_root_entry(struct intel_iommu *iommu) { struct root_entry *root; - root = (struct root_entry *)alloc_pgtable_page(iommu->node); + root = (struct root_entry *)alloc_pgtable_page(iommu->node, GFP_ATOMIC); if (!root) { pr_err("Allocating root entry for %s failed\n", iommu->name); @@ -2676,7 +2676,7 @@ static int copy_context_table(struct intel_iommu *iommu, if (!old_ce) goto out; - new_ce = alloc_pgtable_page(iommu->node); + new_ce = alloc_pgtable_page(iommu->node, GFP_KERNEL); if (!new_ce) goto out_unmap; @@ -4136,7 +4136,7 @@ static int md_domain_init(struct dmar_domain *domain, int guest_width) domain->max_addr = 0; /* always allocate the top pgd */ - domain->pgd = alloc_pgtable_page(domain->nid); + domain->pgd = alloc_pgtable_page(domain->nid, GFP_ATOMIC); if (!domain->pgd) return -ENOMEM; domain_flush_cache(domain, domain->pgd, PAGE_SIZE); diff --git a/drivers/iommu/intel/iommu.h b/drivers/iommu/intel/iommu.h index 06e61e4748567a..ca9a035e0110af 100644 --- a/drivers/iommu/intel/iommu.h +++ b/drivers/iommu/intel/iommu.h @@ -737,7 +737,7 @@ int qi_submit_sync(struct intel_iommu *iommu, struct qi_desc *desc, extern int dmar_ir_support(void); -void *alloc_pgtable_page(int node); +void *alloc_pgtable_page(int node, gfp_t gfp); void free_pgtable_page(void *vaddr); void iommu_flush_write_buffer(struct intel_iommu *iommu); struct intel_iommu *device_to_iommu(struct device *dev, u8 *bus, u8 *devfn); diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c index fb3c7020028d07..c5bf74e9372d62 100644 --- a/drivers/iommu/intel/pasid.c +++ b/drivers/iommu/intel/pasid.c @@ -200,7 +200,7 @@ static struct pasid_entry *intel_pasid_get_entry(struct device *dev, u32 pasid) retry: entries = get_pasid_table_from_pde(&dir[dir_index]); if (!entries) { - entries = alloc_pgtable_page(info->iommu->node); + entries = alloc_pgtable_page(info->iommu->node, GFP_ATOMIC); if (!entries) return NULL;
This is eventually called by iommufd through intel_iommu_map_pages() and it should not be forced to atomic. Push the GFP_ATOMIC to all callers. Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> --- drivers/iommu/intel/iommu.c | 14 +++++++------- drivers/iommu/intel/iommu.h | 2 +- drivers/iommu/intel/pasid.c | 2 +- 3 files changed, 9 insertions(+), 9 deletions(-)