lkml.org 
[lkml]   [2020]   [Jul]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRE: [PATCH v4] mm/zswap: move to use crypto_acomp API for hardware acceleration
Date


> -----Original Message-----
> From: linux-crypto-owner@vger.kernel.org
> [mailto:linux-crypto-owner@vger.kernel.org] On Behalf Of Sebastian Andrzej
> Siewior
> Sent: Thursday, July 9, 2020 7:17 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; Linuxarm <linuxarm@huawei.com>; 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>;
> Colin Ian King <colin.king@canonical.com>
> Subject: Re: [PATCH v4] mm/zswap: move to use crypto_acomp API for
> hardware acceleration
>
> On 2020-07-08 21:45:47 [+0000], Song Bao Hua (Barry Song) wrote:
> > > On 2020-07-08 00:52:10 [+1200], Barry Song wrote:
> > > > @@ -127,9 +129,17 @@
> > > > +struct crypto_acomp_ctx {
> > > > + struct crypto_acomp *acomp;
> > > > + struct acomp_req *req;
> > > > + struct crypto_wait wait;
> > > > + u8 *dstmem;
> > > > + struct mutex mutex;
> > > > +};
> > > …
> > > > @@ -1074,12 +1138,32 @@ static int zswap_frontswap_store(unsigned
> > > type, pgoff_t offset,
> > > > }
> > > >
> > > > /* compress */
> > > > - dst = get_cpu_var(zswap_dstmem);
> > > > - tfm = *get_cpu_ptr(entry->pool->tfm);
> > > > - src = kmap_atomic(page);
> > > > - ret = crypto_comp_compress(tfm, src, PAGE_SIZE, dst, &dlen);
> > > > - kunmap_atomic(src);
> > > > - put_cpu_ptr(entry->pool->tfm);
> > > > + acomp_ctx = *this_cpu_ptr(entry->pool->acomp_ctx);
> > > > +
> > > > + mutex_lock(&acomp_ctx->mutex);
> > > > +
> > > > + src = kmap(page);
> > > > + dst = acomp_ctx->dstmem;
> > >
> > > that mutex is per-CPU, per-context. The dstmem pointer is per-CPU.
> > > So if I read this right, you can get preempted after
> > > crypto_wait_req() and another context in this CPU writes its data to
> > > the same dstmem and then…
> > >
> >
> > This isn't true. Another thread in this cpu will be blocked by the mutex.
> > It is impossible for two threads to write the same dstmem.
> > If thread1 ran on cpu1, it held cpu1's mutex; if another thread wants to run
> on cpu1, it is blocked.
> > If thread1 ran on cpu1 first, it held cpu1's mutex, then it migrated to cpu2
> (with very rare chance)
> > a. if another thread wants to run on cpu1, it is blocked;
>
> How it is blocked? That "struct crypto_acomp_ctx" is
> "this_cpu_ptr(entry->pool->acomp_ctx)" - which is per-CPU of a pool which
> you can have multiple of. But `dstmem' you have only one per-CPU no matter
> have many pools you have.
> So pool1 on CPU1 uses the same `dstmem' as pool2 on CPU1. But pool1 and
> pool2 on CPU1 use a different mutex for protection of this `dstmem'.

Good catch, Sebastian, thanks!
this is a corner case testing has not encountered yet. There is a race if we change the pool type at runtime.
Typically, a group of initial parameters were set, then software wrote/read lots of anon pages to generate
swapping as busy as possible. But never tried to change the compressor/pool type at runtime.

will address this problem in v5 with the cleanup of acomp_ctx pointer in zswap_pool. I mean to
create acomp instants for per-cpu, not for (pools * per-cpu).

Thanks
Barry
\
 
 \ /
  Last update: 2020-07-09 14:15    [W:0.079 / U:0.016 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site