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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <3e8a0bb1-4c69-93d2-71f9-81bb8466cb14@nvidia.com>
Date:   Tue, 16 Aug 2022 13:12:08 +0000
From:   Chaitanya Kulkarni <chaitanyak@...dia.com>
To:     "Fabio M. De Francesco" <fmdefrancesco@...il.com>
CC:     "linux-nvme@...ts.infradead.org" <linux-nvme@...ts.infradead.org>,
        Christoph Hellwig <hch@....de>,
        Chaitanya Kulkarni <chaitanyak@...dia.com>,
        Sagi Grimberg <sagi@...mberg.me>,
        Ira Weiny <ira.weiny@...el.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [RFC PATCH] nvmet-tcp: Don't kmap() pages which can't come from
 HIGHMEM

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