From: "Song Bao Hua (Barry Song)" <song.bao.hua@hisilicon.com>
To: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Cc: "akpm@linux-foundation.org" <akpm@linux-foundation.org>,
"herbert@gondor.apana.org.au" <herbert@gondor.apana.org.au>,
"davem@davemloft.net" <davem@davemloft.net>,
"linux-crypto@vger.kernel.org" <linux-crypto@vger.kernel.org>,
"linux-mm@kvack.org" <linux-mm@kvack.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
"Luis Claudio R . Goncalves" <lgoncalv@redhat.com>,
Mahipal Challa <mahipalreddy2006@gmail.com>,
Seth Jennings <sjenning@redhat.com>,
"Dan Streetman" <ddstreet@ieee.org>,
Vitaly Wool <vitaly.wool@konsulko.com>,
"Wangzhou (B)" <wangzhou1@hisilicon.com>,
"fanghao (A)" <fanghao11@huawei.com>,
Colin Ian King <colin.king@canonical.com>
Subject: RE: [PATCH v6] mm/zswap: move to use crypto_acomp API for hardware acceleration
Date: Tue, 29 Sep 2020 10:02:15 +0000 [thread overview]
Message-ID: <5951148aef79459192826f405a6fa5aa@hisilicon.com> (raw)
In-Reply-To: <20200929093113.3cv63szruo3c4inu@linutronix.de>
> -----Original Message-----
> From: Sebastian Andrzej Siewior [mailto:bigeasy@linutronix.de]
> Sent: Tuesday, September 29, 2020 10:31 PM
> To: Song Bao Hua (Barry Song) <song.bao.hua@hisilicon.com>
> Cc: akpm@linux-foundation.org; herbert@gondor.apana.org.au;
> davem@davemloft.net; linux-crypto@vger.kernel.org; linux-mm@kvack.org;
> linux-kernel@vger.kernel.org; Luis Claudio R . Goncalves
> <lgoncalv@redhat.com>; Mahipal Challa <mahipalreddy2006@gmail.com>;
> Seth Jennings <sjenning@redhat.com>; Dan Streetman <ddstreet@ieee.org>;
> Vitaly Wool <vitaly.wool@konsulko.com>; Wangzhou (B)
> <wangzhou1@hisilicon.com>; fanghao (A) <fanghao11@huawei.com>; Colin
> Ian King <colin.king@canonical.com>
> Subject: Re: [PATCH v6] mm/zswap: move to use crypto_acomp API for
> hardware acceleration
>
> On 2020-09-29 05:14:31 [+0000], Song Bao Hua (Barry Song) wrote:
> > After second thought and trying to make this change, I would like to change
> my mind
> > and disagree with this idea. Two reasons:
> > 1. while using this_cpu_ptr() without preemption lock, people usually put all
> things bound
> > with one cpu to one structure, so that once we get the pointer of the whole
> structure, we get
> > all its parts belonging to the same cpu. If we move the dstmem and mutex
> out of the structure
> > containing them, we will have to do:
> > a. get_cpu_ptr() for the acomp_ctx //lock preemption
> > b. this_cpu_ptr() for the dstmem and mutex
> > c. put_cpu_ptr() for the acomp_ctx //unlock preemption
> > d. mutex_lock()
> > sg_init_one()
> > compress/decompress etc.
> > ...
> > mutex_unlock
> >
> > as the get() and put() have a preemption lock/unlock, this will make certain
> this_cpu_ptr()
> > in the step "b" will return the right dstmem and mutex which belong to the
> same cpu with
> > step "a".
> >
> > The steps from "a" to "c" are quite silly and confusing. I believe the existing
> code aligns
> > with the most similar code in kernel better:
> > a. this_cpu_ptr() //get everything for one cpu
> > b. mutex_lock()
> > sg_init_one()
> > compress/decompress etc.
> > ...
> > mutex_unlock
>
> My point was that there will be a warning at run-time and you don't want
> that. There are raw_ accessors if you know what you are doing. But…
I have only seen get_cpu_ptr/var() things will disable preemption. I don't think
we will have a warning as this_cpu_ptr() won't disable preemption.
>
> Earlier you had compression/decompression with disabled preemption and
No. that is right now done in enabled preemption context with this patch. The code before this patch
was doing (de)compression in preemption-disabled context by using get_cpu_ptr and get_cpu_var.
> strict per-CPU memory allocation. Now if you keep this per-CPU memory
> allocation then you gain a possible bottleneck.
> In the previous email you said that there may be a bottleneck in the
> upper layer where you can't utilize all that memory you allocate. So you
> may want to rethink that strategy before that rework.
we are probably not talking about same thing :-)
I was talking about possible generic swap bottleneck. For example, LRU is global,
while swapping, multiple cores might have some locks on this LRU. for example,
if we have 8 inactive pages to swap out, I am not sure if mm can use 8 cores
to swap them out at the same time.
>
> > 2. while allocating mutex, we can put the mutex into local memory by using
> kmalloc_node().
> > If we move to "struct mutex lock" directly, most CPUs in a NUMA server will
> have to access
> > remote memory to read/write the mutex, therefore, this will increase the
> latency dramatically.
>
> If you need something per-CPU then DEFINE_PER_CPU() will give it to you.
Yes. It is true.
> It would be very bad for performance if this allocations were not from
> CPU-local memory, right? So what makes you think this is worse than
> kmalloc_node() based allocations?
Yes. If your read zswap code, it has considered NUMA very carefully by allocating various
memory locally. And in crypto framework, I also added API to allocate local compression.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=7bc13b5b60e94
this zswap patch has used the new node-aware API.
Memory access crossing NUMA node, practically crossing packages, can dramatically increase,
like double, triple or more.
Thanks
Barry
next prev parent reply other threads:[~2020-09-29 10:02 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-08-18 12:31 [PATCH v6] mm/zswap: move to use crypto_acomp API for hardware acceleration Barry Song
2020-09-28 12:44 ` Vitaly Wool
2020-09-28 15:24 ` Sebastian Andrzej Siewior
2020-09-28 21:05 ` Song Bao Hua (Barry Song)
2020-09-29 5:14 ` Song Bao Hua (Barry Song)
2020-09-29 9:31 ` Sebastian Andrzej Siewior
2020-09-29 10:02 ` Song Bao Hua (Barry Song) [this message]
2020-09-29 10:28 ` Sebastian Andrzej Siewior
2020-09-29 11:05 ` Song Bao Hua (Barry Song)
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=5951148aef79459192826f405a6fa5aa@hisilicon.com \
--to=song.bao.hua@hisilicon.com \
--cc=akpm@linux-foundation.org \
--cc=bigeasy@linutronix.de \
--cc=colin.king@canonical.com \
--cc=davem@davemloft.net \
--cc=ddstreet@ieee.org \
--cc=fanghao11@huawei.com \
--cc=herbert@gondor.apana.org.au \
--cc=lgoncalv@redhat.com \
--cc=linux-crypto@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=mahipalreddy2006@gmail.com \
--cc=sjenning@redhat.com \
--cc=vitaly.wool@konsulko.com \
--cc=wangzhou1@hisilicon.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).