Re: [RFC PATCH] nvmet-tcp: Don't kmap() pages which can't come from HIGHMEM

[Chaitanya Kulkarni]

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@xxxxxxxxx>
> Signed-off-by: Fabio M. De Francesco <fmdefrancesco@xxxxxxxxx>

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