> -----Original Message----- > From: linux-crypto-owner@xxxxxxxxxxxxxxx > [mailto:linux-crypto-owner@xxxxxxxxxxxxxxx] On Behalf Of Sebastian Andrzej > Siewior > Sent: Thursday, July 9, 2020 7:39 PM > To: Song Bao Hua (Barry Song) <song.bao.hua@xxxxxxxxxxxxx> > Cc: akpm@xxxxxxxxxxxxxxxxxxxx; herbert@xxxxxxxxxxxxxxxxxxx; > davem@xxxxxxxxxxxxx; linux-crypto@xxxxxxxxxxxxxxx; linux-mm@xxxxxxxxx; > linux-kernel@xxxxxxxxxxxxxxx; Linuxarm <linuxarm@xxxxxxxxxx>; Luis Claudio > R . Goncalves <lgoncalv@xxxxxxxxxx>; Mahipal Challa > <mahipalreddy2006@xxxxxxxxx>; Seth Jennings <sjenning@xxxxxxxxxx>; > Dan Streetman <ddstreet@xxxxxxxx>; Vitaly Wool > <vitaly.wool@xxxxxxxxxxxx>; Wangzhou (B) <wangzhou1@xxxxxxxxxxxxx>; > Colin Ian King <colin.king@xxxxxxxxxxxxx> > Subject: Re: [PATCH v4] mm/zswap: move to use crypto_acomp API for > hardware acceleration > > On 2020-07-09 01:32:38 [+0000], Song Bao Hua (Barry Song) wrote: > > > This looks using the same synchronous mechanism around an > asynchronous > > > interface. It works as a PoC. > > > > > > As far as I remember the crypto async interface, the incoming skbs were fed > to > > > the async interface and returned to the caller so the NIC could continue > > > allocate new RX skbs and move on. Only if the queue of requests was > getting > > > to long the code started to throttle. Eventually the async crypto code > > > completed the decryption operation in a different context and fed the > > > decrypted packet(s) into the stack. > > > > > > From a quick view, you would have to return -EINPROGRESS here and have > at > > > the caller side something like that: > > > > > > iff --git a/mm/page_io.c b/mm/page_io.c > > > index e8726f3e3820b..9d1baa46ec3ed 100644 > > > --- a/mm/page_io.c > > > +++ b/mm/page_io.c > > > @@ -252,12 +252,15 @@ int swap_writepage(struct page *page, struct > > > writeback_control *wbc) > > > unlock_page(page); > > > goto out; > > > } > > > - if (frontswap_store(page) == 0) { > > > + ret = frontswap_store(page); > > > + if (ret == 0) { > > > set_page_writeback(page); > > > unlock_page(page); > > > end_page_writeback(page); > > > goto out; > > > } > > > + if (ret = -EINPROGRESS) > > > + goto out; > > > ret = __swap_writepage(page, wbc, end_swap_bio_write); > > > out: > > > return ret; > > > > > Unfortunately, this is not true and things are not that simple. > > > > We can't simply depend on -EINPROGRESS and go out. > > We have to wait for the result of compression to decide if we should > > do __swap_writepage(). As one page might be compressed into two > > pages, in this case, we will still need to do _swap_writepage(). > > As I replied in the latest email, all of the async improvement to frontswap > > needs very careful thinking and benchmark. It can only happen after > > we build the base in this patch, fixing the broken connection between > > zswap and those new zip drivers. > > At the time the compression finishes you see what happens and based on > the design you can either complete it immediately (the 0/error case from > above) or forward the result to the caller and let him decide. Hello Sebastian, thanks for your reply and careful review. Right now, frontswap is pretty much one thing which happens before __swap_writepage(). The whole design is full of the assumption that frontswap is sync. So if frontswap consumes a page without any error, this page won't go to __swap_writepage() which is async. On the other hand, if frontswap's store has any error, that means this page needs to swap to disk. int swap_writepage(struct page *page, struct writeback_control *wbc) { int ret = 0; if (try_to_free_swap(page)) { unlock_page(page); goto out; } if (frontswap_store(page) == 0) { set_page_writeback(page); unlock_page(page); end_page_writeback(page); goto out; } ret = __swap_writepage(page, wbc, end_swap_bio_write); out: return ret; } I don't think we can simply "forward the result to the caller and let him decide". Would you like to present some pseudo code? Thanks Barry
