> -----Original Message----- > From: linux-crypto-ow...@vger.kernel.org > [mailto:linux-crypto-ow...@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....@hisilicon.com> > Cc: a...@linux-foundation.org; herb...@gondor.apana.org.au; > da...@davemloft.net; linux-cry...@vger.kernel.org; linux...@kvack.org; > linux-kernel@vger.kernel.org; Linuxarm <linux...@huawei.com>; Luis Claudio > R . Goncalves <lgonc...@redhat.com>; Mahipal Challa > <mahipalreddy2...@gmail.com>; Seth Jennings <sjenn...@redhat.com>; > Dan Streetman <ddstr...@ieee.org>; Vitaly Wool > <vitaly.w...@konsulko.com>; Wangzhou (B) <wangzh...@hisilicon.com>; > Colin Ian King <colin.k...@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
