linux-kernel - RE: [PATCH v4] mm/zswap: move to use crypto

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <B926444035E5E2439431908E3842AFD2561D4E@DGGEMI525-MBS.china.huawei.com>
Date:   Thu, 9 Jul 2020 07:55:22 +0000
From:   "Song Bao Hua (Barry Song)" <song.bao.hua@...ilicon.com>
To:     Sebastian Andrzej Siewior <bigeasy@...utronix.de>
CC:     "akpm@...ux-foundation.org" <akpm@...ux-foundation.org>,
        "herbert@...dor.apana.org.au" <herbert@...dor.apana.org.au>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "linux-crypto@...r.kernel.org" <linux-crypto@...r.kernel.org>,
        "linux-mm@...ck.org" <linux-mm@...ck.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Linuxarm <linuxarm@...wei.com>,
        "Luis Claudio R . Goncalves" <lgoncalv@...hat.com>,
        Mahipal Challa <mahipalreddy2006@...il.com>,
        Seth Jennings <sjenning@...hat.com>,
        Dan Streetman <ddstreet@...e.org>,
        Vitaly Wool <vitaly.wool@...sulko.com>,
        "Wangzhou (B)" <wangzhou1@...ilicon.com>,
        "Colin Ian King" <colin.king@...onical.com>
Subject: RE: [PATCH v4] mm/zswap: move to use crypto_acomp API for hardware
 acceleration



> -----Original Message-----
> From: linux-crypto-owner@...r.kernel.org
> [mailto:linux-crypto-owner@...r.kernel.org] On Behalf Of Sebastian Andrzej
> Siewior
> Sent: Thursday, July 9, 2020 7:39 PM
> To: Song Bao Hua (Barry Song) <song.bao.hua@...ilicon.com>
> Cc: akpm@...ux-foundation.org; herbert@...dor.apana.org.au;
> davem@...emloft.net; linux-crypto@...r.kernel.org; linux-mm@...ck.org;
> linux-kernel@...r.kernel.org; Linuxarm <linuxarm@...wei.com>; Luis Claudio
> R . Goncalves <lgoncalv@...hat.com>; Mahipal Challa
> <mahipalreddy2006@...il.com>; Seth Jennings <sjenning@...hat.com>;
> Dan Streetman <ddstreet@...e.org>; Vitaly Wool
> <vitaly.wool@...sulko.com>; Wangzhou (B) <wangzhou1@...ilicon.com>;
> Colin Ian King <colin.king@...onical.com>
> 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