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

["Fabio M. De Francesco" <fmdefrancesco@gmail.com>]

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
>