Messages in this thread | | | From | Chaitanya Kulkarni <> | Subject | Re: [RFC PATCH] nvmet-tcp: Don't kmap() pages which can't come from HIGHMEM | Date | Tue, 16 Aug 2022 13:12:08 +0000 |
| |
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 ?
> 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 unmappings 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.
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_loca_page() is faster than kmap().
In case you are not aware please have a look at the blktests to create a simple loopback setpu with nvme-loop transport.
-ck
| |