> -----Original Message----- > From: [email protected] > [mailto:[email protected]] On Behalf Of Sebastian Andrzej > Siewior > Sent: Thursday, July 9, 2020 7:17 PM > To: Song Bao Hua (Barry Song) <[email protected]> > Cc: [email protected]; [email protected]; > [email protected]; [email protected]; [email protected]; > [email protected]; Linuxarm <[email protected]>; Luis Claudio > R . Goncalves <[email protected]>; Mahipal Challa > <[email protected]>; Seth Jennings <[email protected]>; > Dan Streetman <[email protected]>; Vitaly Wool > <[email protected]>; Wangzhou (B) <[email protected]>; > Colin Ian King <[email protected]> > 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
