Re: [RFC PATCH] nvmet-tcp: Don't kmap() pages which can't come from HIGHMEM
From: Fabio M. De Francesco
Date: Tue Aug 16 2022 - 14:17:03 EST
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@xxxxxxxxxx/
> > 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@xxxxxxxxx>
> > Signed-off-by: Fabio M. De Francesco <fmdefrancesco@xxxxxxxxx>
>
>
> 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
>