Messages in this thread | | | From | "Fabio M. De Francesco" <> | Subject | Re: [RFC PATCH] nvmet-tcp: Don't kmap() pages which can't come from HIGHMEM | Date | Tue, 16 Aug 2022 20:16:54 +0200 |
| |
On martedì 16 agosto 2022 15:12:08 CEST Chaitanya Kulkarni wrote: > Fabio, > > On 8/16/22 02:18, Fabio M. De Francesco wrote: > > > kmap() is being deprecated in favor of kmap_local_page(). > > > > There are two main problems with kmap(): (1) It comes with an overhead as > > mapping space is restricted and protected by a global lock for > > synchronization and (2) it also requires global TLB invalidation when the > > kmap’s pool wraps and it might block when the mapping space is fully > > utilized until a slot becomes available. > > > > so I believe this should give us better performance under heavy > workload ? >
Yes, correct. Can you please take a look at the highmem official documentation (highmem.rst)? I reworked and extended it with two series of patches. Everything about the deprecation of kmap() is explained there and in a patch from Ira: "checkpatch: Add kmap and kmap_atomic to the deprecated list" which you reviewed at https://lore.kernel.org/all/91f708ed-f456-dc83-281e-fc18a0b4b981@nvidia.com/
> > With kmap_local_page() the mappings are per thread, CPU local, can take > > page faults, and can be called from any context (including interrupts). > > It is faster than kmap() in kernels with HIGHMEM enabled. Furthermore, > > the tasks can be preempted and, when they are scheduled to run again, the > > kernel virtual addresses are restored and are still valid. > > > > However, there is a huge constraint which might block some conversions > > to kmap_local_page(): the kernel virtual address cannot be handed across > > different threads. Ira made me notice that the kmap() and kunmap() in this > > driver happen in two different workqueues. Therefore, kunmap_local() will > > try to unmap an invalid address. > > > > Let me explain why I'm sending an RFC. When I hit the above mentioned > > issues I tried to refactor the code in ways where mapping and unmapping > > happen in a single thread (to not break the rules of threads locality). > > > > However, while reading this code again I think I noticed an important > > prerequisite which may lead to a simpler solution... If I'm not wrong, it > > looks like the pages are allocated in nvmet_tcp_map_data(), using the > > GFP_KERNEL flag. > > > > This would assure that those pages _cannot_ come from HIGHMEM. If I'm not > > missing something (again!), a plain page_address() could replace the kmap() > > of sg_page(sg); furthermore, we shouldn't need the un-mappings any longer. > > > > Unfortunately, I don't know this protocol and I'm not so experienced with > > kernel development to be able to understand this code properly. > > > > Therefore, I have two questions: am I right about thinking that the pages > > mapped in nvmet_tcp_map_pdu_iovec() are allocated with GFP_KERNEL? If so, > > can anyone with more knowledge than mine please say if my changes make any > > sense? > > > > Suggested-by: Ira Weiny <ira.weiny@intel.com> > > Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com> > > > Thanks a lot for detailed explanation.
You are welcome!
> Quick question what kind of performance benefits you have seen with > this change ? we need to document the performance numbers since commit > log mentions here that kmap_local_page() is faster than kmap().
OK, but kmap_local_page() was discarded because not applicable here without heavy refactoring.
> In case you are not aware please have a look at the blktests to create > a simple loopback setup with nvme-loop transport.
I have nothing against learning how blktests works and running this tool. I'll do as you requested.
However, please read the implementation of kmap():
#ifdef CONFIG_HIGHMEM
static inline void *kmap(struct page *page) { void *addr;
might_sleep(); if (!PageHighMem(page)) addr = page_address(page); else addr = kmap_high(page); kmap_flush_tlb((unsigned long)addr); return addr; }
If page is not from HIGHMEM it is a simple page_address(), like it is in my RFC patch.
#else /* CONFIG_HIGHMEM */
static inline void *kmap(struct page *page) { might_sleep(); return page_address(page); }
Again, a plain page_address(). Furthermore, with a simple page_address() we avoid the calls to kunmap().
I think it implicitly say all we need to know about why we should prefer page_address() whenever we are _sure_ that pages cannot come from HIGHMEM.
Thanks for your comments and questions,
Fabio
> -ck >
| |