[PATCH] Remove unnecessary calls to kmap{,_local_page}() when acquiring pages using GFP_DMA32.
The GFP_DMA32 uses the DMA32 zone to satisfy the allocation requests. Therefore, pages allocated with GFP_DMA32 cannot come from Highmem. Avoid using calls to kmap() / kunmap() as the kmap() is being deprecated [1]. Avoid using calls to kmap_local_page() / kunmap_local() as the code does not depends on the implicit disable of migration of local mappings and is, in fact, an unnecessary overhead for the main code [2]. Hence, use a plain page_address() directly in the psb_mmu_alloc_pd function. [1]: https://lore.kernel.org/all/20220813220034.806698-1-ira.we...@intel.com/ [2]: https://lwn.net/Articles/836503/ Suggested-by: Ira Weiny Signed-off-by: Sumitra Sharma --- drivers/gpu/drm/gma500/mmu.c | 11 +++ 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/drivers/gpu/drm/gma500/mmu.c b/drivers/gpu/drm/gma500/mmu.c index a70b01ccdf70..1a44dd062fd1 100644 --- a/drivers/gpu/drm/gma500/mmu.c +++ b/drivers/gpu/drm/gma500/mmu.c @@ -184,20 +184,15 @@ struct psb_mmu_pd *psb_mmu_alloc_pd(struct psb_mmu_driver *driver, pd->invalid_pte = 0; } - v = kmap_local_page(pd->dummy_pt); + v = page_address(pd->dummy_pt); for (i = 0; i < (PAGE_SIZE / sizeof(uint32_t)); ++i) v[i] = pd->invalid_pte; - kunmap_local(v); - - v = kmap_local_page(pd->p); + v = page_address(pd->p); for (i = 0; i < (PAGE_SIZE / sizeof(uint32_t)); ++i) v[i] = pd->invalid_pde; - kunmap_local(v); - - clear_page(kmap(pd->dummy_page)); - kunmap(pd->dummy_page); + clear_page(page_address(pd->dummy_page)); pd->tables = vmalloc_user(sizeof(struct psb_mmu_pt *) * 1024); if (!pd->tables) -- 2.25.1
Re: [PATCH] drm/gma500: Replace kmap{,_atomic}() with page_address()
On Wed, Jun 21, 2023 at 12:31:40AM +0200, Fabio M. De Francesco wrote:
> On martedì 20 giugno 2023 20:01:48 CEST Sumitra Sharma wrote:
> > Remove unnecessary calls to kmap{,_atomic}() when acquiring
> > pages using GFP_DMA32.
> >
> > The GFP_DMA32 uses the DMA32 zone to satisfy the allocation
> > requests. Therefore, pages allocated with GFP_DMA32 cannot
> > come from Highmem.
> >
> > Avoid using calls to kmap_local_page() / kunmap_local() and
> > kmap() / kunmap() in the psb_mmu_alloc_pd function. Instead,
> > utilize page_address().
> >
> > Remove the usage of kmap_atomic() / kunmap_atomic() in the
> > psb_mmu_alloc_pt function. Use page_address() instead.
> >
> > Substitute kmap_atomic(pt->p) / kunmap_atomic(pt->v) calls
> > in the psb_mmu_pt_alloc_map_lock() and psb_mmu_pt_unmap_unlock()
> > functions with page_address(pt->p). This is possible as
> > pt = psb_mmu_alloc_pt(pd) allocates a page using
> > pt->p = alloc_page(GFP_DMA32).
>
> Sumitra,
>
> I'm sorry because this patch cannot acked with this commit message.
>
> This commit message is missing two _really_ important information. Therefore,
> it is not acked. Please check again what I write below and either add the
> missing information or change the code accordingly...
>
> You should assure everybody that the code between the old kmap_atomic() /
> kunmap_atomic() doesn't depend either on implicit pagefault_disable() or
> preempt_disable() calls or both.
>
> Please read again the section of the Highmem's documentation regarding
> kmap_atomic() at https://docs.kernel.org/mm/highmem.html
>
> In particular take care to read and understand what "[] the code between
> calls
> to kmap_atomic() and kunmap_atomic() may implicitly depend on the side
> effects
> of atomic mappings, i.e. disabling page faults or preemption, or both. In
> that
> case, explicit calls to pagefault_disable() or preempt_disable() or both must
> be made in conjunction with the use of kmap_local_page().".
>
> Please study carefully also the following patch from Zhao, suggested by Ira
> and reviewed by Ira and I: "[PATCH v2 3/9] drm/i915: Use kmap_local_page() in
> gem/i915_gem_shmem.c". It's not yet reached upstream so you need to find it
> in
> lore.kernel.org at
> https://lore.kernel.org/lkml/20230329073220.3982460-4-zhao1@linux.intel.com/
>
> Please note that, in turn, that patch also contains a link to a patch from
> Ira
> who too had to disable faults (https://lore.kernel.org/all/
> 20220813220034.806698-1-ira.we...@intel.com)
>
> I haven't yet looked at your code. If any sections do depend on those
> implicit
> disables, you should act accordingly and add one or both of the above-
> mentioned calls, even in cases where you get rid of local mappings.
>
> Instead if the sections don't depend on the mentioned side effects, you
> should
> write something like what I wrote in "[PATCH] NFS: Convert kmap_atomic() to
> kmap_local_folio()" that you can find at https://lore.kernel.org/lkml/
> 20230503172411.3356-1-fmdefrance...@gmail.com/ or, by by using "git show
> 4b71e2416ec4".
>
> Thanks for working on this,
>
> Fabio
>
Hi Fabio,
Thank you for notifying the mistakes I made in this patch. I am directly
working on the changes.
Regards
Sumitra
> >
> > Suggested-by: Ira Weiny
> > Signed-off-by: Sumitra Sharma
> > ---
> > drivers/gpu/drm/gma500/mmu.c | 17 +
> > 1 file changed, 5 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/gma500/mmu.c b/drivers/gpu/drm/gma500/mmu.c
> > index a70b01ccdf70..59aa5661e56a 100644
> > --- a/drivers/gpu/drm/gma500/mmu.c
> > +++ b/drivers/gpu/drm/gma500/mmu.c
> > @@ -184,20 +184,15 @@ struct psb_mmu_pd *psb_mmu_alloc_pd(struct
> > psb_mmu_driver *driver, pd->invalid_pte = 0;
> > }
> >
> > - v = kmap_local_page(pd->dummy_pt);
> > + v = page_address(pd->dummy_pt);
> > for (i = 0; i < (PAGE_SIZE / sizeof(uint32_t)); ++i)
> > v[i] = pd->invalid_pte;
> >
> > - kunmap_local(v);
> > -
> > - v = kmap_local_page(pd->p);
> > + v = page_address(pd->p);
> > for (i = 0; i < (PAGE_SIZE / sizeof(uint32_t)); ++i)
> > v[i] = pd->invalid_pde;
> >
> > - kunmap_local(v);
> > -
> > - clear_page(kmap(pd->dummy_page));
> > - kunmap(pd->dummy_page);
> > + clear_page(page_address(pd->dummy_page));
> >
> > pd->tables = vmalloc_user(sizeof(struct psb_mmu_pt *)
Re: [PATCH v2] drm/i915: Replace kmap() with kmap_local_page()
On Tue, Jun 20, 2023 at 06:23:38AM -0700, Ira Weiny wrote: > Sumitra Sharma wrote: > > On Sun, Jun 18, 2023 at 11:11:08AM -0700, Ira Weiny wrote: > > > Sumitra Sharma wrote: > > > > kmap() has been deprecated in favor of the kmap_local_page() > > > > due to high cost, restricted mapping space, the overhead of a > > > > global lock for synchronization, and making the process sleep > > > > in the absence of free slots. > > > > > > > > kmap_local_page() is faster than kmap() and offers thread-local > > > > and CPU-local mappings, take pagefaults in a local kmap region > > > > and preserves preemption by saving the mappings of outgoing tasks > > > > and restoring those of the incoming one during a context switch. > > > > > > > > The mapping is kept thread local in the function > > > > “i915_vma_coredump_create” in i915_gpu_error.c > > > > > > > > Therefore, replace kmap() with kmap_local_page(). > > > > > > > > Suggested-by: Ira Weiny > > > > > > > > > > NIT: No need for the line break between Suggested-by and your signed off > > > line. > > > > > > > Hi Ira, > > > > What does NIT stand for? > > Shorthand for 'nitpicking'. > > "giving too much attention to details that are not important, especially > as a way of criticizing: " > > - https://dictionary.cambridge.org/dictionary/english/nitpicking > > Via email this is a way for authors of an email to indicate something is > technically wrong but while nicely acknowledging that it is not very > significant and could be seen as overly critical. > > For this particular comment I'm showing something to pay attention to next > time but that was not a big deal this time around. > Hi Ira, Thank for your explanation on NIT. > > > > Thank you. I will take care about the line breaks. > > > > > > Signed-off-by: Sumitra Sharma > > > > --- > > > > > > > > Changes in v2: > > > > - Replace kmap() with kmap_local_page(). > > > > > > Generally it is customary to attribute a change like this to those who > > > suggested it in a V1 review. > > > > > > For example: > > > > > > - Tvrtko/Thomas: Use kmap_local_page() instead of page_address() > > > > > > Also I don't see Thomas on the new email list. Since he took the time to > > > review V1 he might want to check this version out. I've added him to the > > > 'To:' list. > > > > > > Also a link to V1 is nice. B4 formats it like this: > > > > > > - Link to v1: > > > https://lore.kernel.org/all/20230614123556.ga381...@sumitra.com/ > > > > > > All that said the code looks good to me. So with the above changes. > > > > > > Reviewed-by: Ira Weiny > > > > > > > I have noted down the points mentioned above. Thank you again. > > > > I am not supposed to create another version of this patch for > > adding the above mentions, as you and Thomas both gave this patch > > a reviewed-by tag. Right? > > > > Based on this response[*] from Tvrtko I think this version can move > through without a v3. Okay! Thanks & regards Sumitra > > Thanks! > Ira > > [*] > https://lore.kernel.org/all/bcb0a1d2-cd4d-a56f-1ee6-7ccfdd2f7...@linux.intel.com/ > > > Thanks all! I'll just re-send the patch for our CI, since it didn't get > picked up automatically (stuck in moderation perhaps), with all r-b tags > added and extra line space removed and merge it if results will be green. > > Regards, > > Tvrtko > > > > > > > Thanks & regards > > Sumitra > > > > PS: I am new to the open source vocabulary terms. > > > > > > - Change commit subject and message. > > > > > > > > drivers/gpu/drm/i915/i915_gpu_error.c | 4 ++-- > > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c > > > > b/drivers/gpu/drm/i915/i915_gpu_error.c > > > > index f020c0086fbc..bc41500eedf5 100644 > > > > --- a/drivers/gpu/drm/i915/i915_gpu_error.c > > > > +++ b/drivers/gpu/drm/i915/i915_gpu_error.c > > > > @@ -1164,9 +1164,9 @@ i915_vma_coredump_create(const struct intel_gt > > > > *gt, > > > > > > > > drm_clflush_pages(, 1); > > > > > > > > - s = kmap(page); > > > > + s = kmap_local_page(page); > > > > ret = compress_page(compress, s, dst, false); > > > > - kunmap(page); > > > > + kunmap_local(s); > > > > > > > > drm_clflush_pages(, 1); > > > > > > > > -- > > > > 2.25.1 > > > > > > > > > > > >
[PATCH] drm/gma500: Replace kmap{,_atomic}() with page_address()
Remove unnecessary calls to kmap{,_atomic}() when acquiring
pages using GFP_DMA32.
The GFP_DMA32 uses the DMA32 zone to satisfy the allocation
requests. Therefore, pages allocated with GFP_DMA32 cannot
come from Highmem.
Avoid using calls to kmap_local_page() / kunmap_local() and
kmap() / kunmap() in the psb_mmu_alloc_pd function. Instead,
utilize page_address().
Remove the usage of kmap_atomic() / kunmap_atomic() in the
psb_mmu_alloc_pt function. Use page_address() instead.
Substitute kmap_atomic(pt->p) / kunmap_atomic(pt->v) calls
in the psb_mmu_pt_alloc_map_lock() and psb_mmu_pt_unmap_unlock()
functions with page_address(pt->p). This is possible as
pt = psb_mmu_alloc_pt(pd) allocates a page using
pt->p = alloc_page(GFP_DMA32).
Suggested-by: Ira Weiny
Signed-off-by: Sumitra Sharma
---
drivers/gpu/drm/gma500/mmu.c | 17 +
1 file changed, 5 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/gma500/mmu.c b/drivers/gpu/drm/gma500/mmu.c
index a70b01ccdf70..59aa5661e56a 100644
--- a/drivers/gpu/drm/gma500/mmu.c
+++ b/drivers/gpu/drm/gma500/mmu.c
@@ -184,20 +184,15 @@ struct psb_mmu_pd *psb_mmu_alloc_pd(struct psb_mmu_driver
*driver,
pd->invalid_pte = 0;
}
- v = kmap_local_page(pd->dummy_pt);
+ v = page_address(pd->dummy_pt);
for (i = 0; i < (PAGE_SIZE / sizeof(uint32_t)); ++i)
v[i] = pd->invalid_pte;
- kunmap_local(v);
-
- v = kmap_local_page(pd->p);
+ v = page_address(pd->p);
for (i = 0; i < (PAGE_SIZE / sizeof(uint32_t)); ++i)
v[i] = pd->invalid_pde;
- kunmap_local(v);
-
- clear_page(kmap(pd->dummy_page));
- kunmap(pd->dummy_page);
+ clear_page(page_address(pd->dummy_page));
pd->tables = vmalloc_user(sizeof(struct psb_mmu_pt *) * 1024);
if (!pd->tables)
@@ -279,7 +274,7 @@ static struct psb_mmu_pt *psb_mmu_alloc_pt(struct
psb_mmu_pd *pd)
spin_lock(lock);
- v = kmap_atomic(pt->p);
+ v = page_address(pt->p);
clf = (uint8_t *) v;
ptes = (uint32_t *) v;
for (i = 0; i < (PAGE_SIZE / sizeof(uint32_t)); ++i)
@@ -293,7 +288,6 @@ static struct psb_mmu_pt *psb_mmu_alloc_pt(struct
psb_mmu_pd *pd)
}
mb();
}
- kunmap_atomic(v);
spin_unlock(lock);
pt->count = 0;
@@ -339,7 +333,7 @@ static struct psb_mmu_pt *psb_mmu_pt_alloc_map_lock(struct
psb_mmu_pd *pd,
atomic_set(>driver->needs_tlbflush, 1);
}
}
- pt->v = kmap_atomic(pt->p);
+ pt->v = page_address(pt->p);
return pt;
}
@@ -365,7 +359,6 @@ static void psb_mmu_pt_unmap_unlock(struct psb_mmu_pt *pt)
struct psb_mmu_pd *pd = pt->pd;
uint32_t *v;
- kunmap_atomic(pt->v);
if (pt->count == 0) {
v = kmap_atomic(pd->p);
v[pt->index] = pd->invalid_pde;
--
2.25.1
Re: [PATCH v2] drm/i915: Replace kmap() with kmap_local_page()
On Sun, Jun 18, 2023 at 11:11:08AM -0700, Ira Weiny wrote: > Sumitra Sharma wrote: > > kmap() has been deprecated in favor of the kmap_local_page() > > due to high cost, restricted mapping space, the overhead of a > > global lock for synchronization, and making the process sleep > > in the absence of free slots. > > > > kmap_local_page() is faster than kmap() and offers thread-local > > and CPU-local mappings, take pagefaults in a local kmap region > > and preserves preemption by saving the mappings of outgoing tasks > > and restoring those of the incoming one during a context switch. > > > > The mapping is kept thread local in the function > > “i915_vma_coredump_create” in i915_gpu_error.c > > > > Therefore, replace kmap() with kmap_local_page(). > > > > Suggested-by: Ira Weiny > > > > NIT: No need for the line break between Suggested-by and your signed off line. > Hi Ira, What does NIT stand for? Thank you. I will take care about the line breaks. > > Signed-off-by: Sumitra Sharma > > --- > > > > Changes in v2: > > - Replace kmap() with kmap_local_page(). > > Generally it is customary to attribute a change like this to those who > suggested it in a V1 review. > > For example: > > - Tvrtko/Thomas: Use kmap_local_page() instead of page_address() > > Also I don't see Thomas on the new email list. Since he took the time to > review V1 he might want to check this version out. I've added him to the > 'To:' list. > > Also a link to V1 is nice. B4 formats it like this: > > - Link to v1: https://lore.kernel.org/all/20230614123556.ga381...@sumitra.com/ > > All that said the code looks good to me. So with the above changes. > > Reviewed-by: Ira Weiny > I have noted down the points mentioned above. Thank you again. I am not supposed to create another version of this patch for adding the above mentions, as you and Thomas both gave this patch a reviewed-by tag. Right? Thanks & regards Sumitra PS: I am new to the open source vocabulary terms. > > - Change commit subject and message. > > > > drivers/gpu/drm/i915/i915_gpu_error.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c > > b/drivers/gpu/drm/i915/i915_gpu_error.c > > index f020c0086fbc..bc41500eedf5 100644 > > --- a/drivers/gpu/drm/i915/i915_gpu_error.c > > +++ b/drivers/gpu/drm/i915/i915_gpu_error.c > > @@ -1164,9 +1164,9 @@ i915_vma_coredump_create(const struct intel_gt *gt, > > > > drm_clflush_pages(, 1); > > > > - s = kmap(page); > > + s = kmap_local_page(page); > > ret = compress_page(compress, s, dst, false); > > - kunmap(page); > > + kunmap_local(s); > > > > drm_clflush_pages(, 1); > > > > -- > > 2.25.1 > > > >
Re: [Intel-gfx] [PATCH] drm/i915: Call page_address() on page acquired with GFP_KERNEL flag
On Wed, Jun 14, 2023 at 05:30:25PM +0200, Thomas Hellström (Intel) wrote: > > On 6/14/23 15:22, Tvrtko Ursulin wrote: > > > > On 14/06/2023 13:35, Sumitra Sharma wrote: > > > Pages allocated with GFP_KERNEL cannot come from Highmem. > > > That is why there is no need to call kmap() on them. > > > > Are you sure it is GFP_KERNEL backed and not tmpfs? I am not sure myself > > so let me copy Matt and Thomas if they happen to know off hand. > Hello, Yes it is true that the pages have not been acquired using the GFP_KERNEL. I confused the allocation of the struct 'i915_vma_resource' tracking the pages with the allocation of the pages themselves. This was noted by my mentor Ira Weiny . > It looks to me these are shmem pages or TTM pages. Both could be highmem. So > I think kmap is the correct choice here. > However, the kmap() will not be the correct choice here and kmap_local_page() must be used instead. I have created a v2 patch for the same https://lore.kernel.org/lkml/20230617180420.ga410...@sumitra.com/ Thank you for helping me. Regards Sumitra > /Thomas > > > > > > > > Regards, > > > > Tvrtko > > > > > Therefore, don't call kmap() on the page coming from > > > vma_res->bi.pages using for_each_sgt_page() in > > > i915_vma_coredump_create(). > > > > > > Use a plain page_address() to get the kernel address instead. > > > > > > Signed-off-by: Sumitra Sharma > > > --- > > > drivers/gpu/drm/i915/i915_gpu_error.c | 3 +-- > > > 1 file changed, 1 insertion(+), 2 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c > > > b/drivers/gpu/drm/i915/i915_gpu_error.c > > > index f020c0086fbc..6f51cb4fc55c 100644 > > > --- a/drivers/gpu/drm/i915/i915_gpu_error.c > > > +++ b/drivers/gpu/drm/i915/i915_gpu_error.c > > > @@ -1164,9 +1164,8 @@ i915_vma_coredump_create(const struct intel_gt > > > *gt, > > > drm_clflush_pages(, 1); > > > - s = kmap(page); > > > + s = page_address(page); > > > ret = compress_page(compress, s, dst, false); > > > - kunmap(page); > > > drm_clflush_pages(, 1);
[PATCH v2] drm/i915: Replace kmap() with kmap_local_page()
kmap() has been deprecated in favor of the kmap_local_page() due to high cost, restricted mapping space, the overhead of a global lock for synchronization, and making the process sleep in the absence of free slots. kmap_local_page() is faster than kmap() and offers thread-local and CPU-local mappings, take pagefaults in a local kmap region and preserves preemption by saving the mappings of outgoing tasks and restoring those of the incoming one during a context switch. The mapping is kept thread local in the function “i915_vma_coredump_create” in i915_gpu_error.c Therefore, replace kmap() with kmap_local_page(). Suggested-by: Ira Weiny Signed-off-by: Sumitra Sharma --- Changes in v2: - Replace kmap() with kmap_local_page(). - Change commit subject and message. drivers/gpu/drm/i915/i915_gpu_error.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c index f020c0086fbc..bc41500eedf5 100644 --- a/drivers/gpu/drm/i915/i915_gpu_error.c +++ b/drivers/gpu/drm/i915/i915_gpu_error.c @@ -1164,9 +1164,9 @@ i915_vma_coredump_create(const struct intel_gt *gt, drm_clflush_pages(, 1); - s = kmap(page); + s = kmap_local_page(page); ret = compress_page(compress, s, dst, false); - kunmap(page); + kunmap_local(s); drm_clflush_pages(, 1); -- 2.25.1
[PATCH] drm/i915: Call page_address() on page acquired with GFP_KERNEL flag
Pages allocated with GFP_KERNEL cannot come from Highmem. That is why there is no need to call kmap() on them. Therefore, don't call kmap() on the page coming from vma_res->bi.pages using for_each_sgt_page() in i915_vma_coredump_create(). Use a plain page_address() to get the kernel address instead. Signed-off-by: Sumitra Sharma --- drivers/gpu/drm/i915/i915_gpu_error.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c index f020c0086fbc..6f51cb4fc55c 100644 --- a/drivers/gpu/drm/i915/i915_gpu_error.c +++ b/drivers/gpu/drm/i915/i915_gpu_error.c @@ -1164,9 +1164,8 @@ i915_vma_coredump_create(const struct intel_gt *gt, drm_clflush_pages(, 1); - s = kmap(page); + s = page_address(page); ret = compress_page(compress, s, dst, false); - kunmap(page); drm_clflush_pages(, 1); -- 2.25.1
