From: Ard Biesheuvel <ardb@kernel.org> To: Nicolas TOROMANOFF <nicolas.toromanoff@st.com>, Eric Biggers <ebiggers@kernel.org> Cc: Herbert Xu <herbert@gondor.apana.org.au>, "David S . Miller" <davem@davemloft.net>, Maxime Coquelin <mcoquelin.stm32@gmail.com>, Alexandre TORGUE <alexandre.torgue@st.com>, Linux Crypto Mailing List <linux-crypto@vger.kernel.org>, "linux-stm32@st-md-mailman.stormreply.com" <linux-stm32@st-md-mailman.stormreply.com>, Linux ARM <linux-arm-kernel@lists.infradead.org>, Linux Kernel Mailing List <linux-kernel@vger.kernel.org> Subject: Re: [PATCH 5/5] crypto: stm32/crc: protect from concurrent accesses Date: Mon, 25 May 2020 11:07:10 +0200 [thread overview] Message-ID: <CAMj1kXFwt6cs-MJhAeMRF4-yiddm=ezq=qvSjA_sRAX+_Gdqhw@mail.gmail.com> (raw) In-Reply-To: <bd6cac3bd4c74db1a403df58082028fd@SFHDAG6NODE1.st.com> (+ Eric) On Mon, 25 May 2020 at 11:01, Nicolas TOROMANOFF <nicolas.toromanoff@st.com> wrote: > > > -----Original Message----- > > From: Ard Biesheuvel <ardb@kernel.org> > > Sent: Monday, May 25, 2020 9:46 AM > > To: Nicolas TOROMANOFF <nicolas.toromanoff@st.com> > > Subject: Re: [PATCH 5/5] crypto: stm32/crc: protect from concurrent accesses > > > > On Mon, 25 May 2020 at 09:24, Nicolas TOROMANOFF > > <nicolas.toromanoff@st.com> wrote: > > > > > > Hello, > > > > > > > -----Original Message----- > > > > From: Ard Biesheuvel <ardb@kernel.org> > > > > Sent: Friday, May 22, 2020 6:12 PM> > > > > On Tue, 12 May 2020 at 16:13, Nicolas Toromanoff > > > > <nicolas.toromanoff@st.com> wrote: > > > > > > > > > > Protect STM32 CRC device from concurrent accesses. > > > > > > > > > > As we create a spinlocked section that increase with buffer size, > > > > > we provide a module parameter to release the pressure by splitting > > > > > critical section in chunks. > > > > > > > > > > Size of each chunk is defined in burst_size module parameter. > > > > > By default burst_size=0, i.e. don't split incoming buffer. > > > > > > > > > > Signed-off-by: Nicolas Toromanoff <nicolas.toromanoff@st.com> > > > > > > > > Would you mind explaining the usage model here? It looks like you > > > > are sharing a CRC hardware accelerator with a synchronous interface > > > > between different users by using spinlocks? You are aware that this > > > > will tie up the waiting CPUs completely during this time, right? So > > > > it would be much better to use a mutex here. Or perhaps it would > > > > make more sense to fall back to a s/w based CRC routine if the h/w is tied up > > working for another task? > > > > > > I know mutex are more acceptable here, but shash _update() and _init() > > > may be call from any context, and so I cannot take a mutex. > > > And to protect my concurrent HW access I only though about spinlock. > > > Due to possible constraint on CPUs, I add a burst_size option to force > > > slitting long buffer into smaller one, and so decrease time we take the lock. > > > But I didn't though to fallback to software CRC. > > > > > > I'll do a patch on top. > > > In in the burst_update() function I'll use a spin_trylock_irqsave() and use > > software CRC32 if HW is already in use. > > > > > > > Right. I didn't even notice that you were keeping interrupts disabled the whole > > time when using the h/w block. That means that any serious use of this h/w > > block will make IRQ latency go through the roof. > > > > I recommend that you go back to the drawing board on this driver, rather than > > papering over the issues with a spin_trylock(). Perhaps it would be better to > > model it as a ahash (even though the h/w block itself is synchronous) and use a > > kthread to feed in the data. > > I thought when I updated the driver to move to a ahash interface, but the main usage > of crc32 is the ext4 fs, that calls the shash API. > Commit 877b5691f27a ("crypto: shash - remove shash_desc::flags") removed possibility > to sleep in shash callback. (before this commit and with MAY_SLEEP option set, using > a mutex may have been fine). > According to that commit's log, sleeping is never fine for shash(), since it uses kmap_atomic() when iterating over the scatterlist. > By now the solution I see is to use the spin_trylock_irqsave(), fallback to software crc *AND* capping burst_size > to ensure the locked section stay reasonable. > > Does this seems acceptable ? > If the reason for disabling interrupts is to avoid deadlocks, wouldn't the switch to trylock() with a software fallback allow us to keep interrupts enabled?
WARNING: multiple messages have this Message-ID (diff)
From: Ard Biesheuvel <ardb@kernel.org> To: Nicolas TOROMANOFF <nicolas.toromanoff@st.com>, Eric Biggers <ebiggers@kernel.org> Cc: Alexandre TORGUE <alexandre.torgue@st.com>, Linux Kernel Mailing List <linux-kernel@vger.kernel.org>, "David S . Miller" <davem@davemloft.net>, Linux Crypto Mailing List <linux-crypto@vger.kernel.org>, Maxime Coquelin <mcoquelin.stm32@gmail.com>, "linux-stm32@st-md-mailman.stormreply.com" <linux-stm32@st-md-mailman.stormreply.com>, Linux ARM <linux-arm-kernel@lists.infradead.org>, Herbert Xu <herbert@gondor.apana.org.au> Subject: Re: [PATCH 5/5] crypto: stm32/crc: protect from concurrent accesses Date: Mon, 25 May 2020 11:07:10 +0200 [thread overview] Message-ID: <CAMj1kXFwt6cs-MJhAeMRF4-yiddm=ezq=qvSjA_sRAX+_Gdqhw@mail.gmail.com> (raw) In-Reply-To: <bd6cac3bd4c74db1a403df58082028fd@SFHDAG6NODE1.st.com> (+ Eric) On Mon, 25 May 2020 at 11:01, Nicolas TOROMANOFF <nicolas.toromanoff@st.com> wrote: > > > -----Original Message----- > > From: Ard Biesheuvel <ardb@kernel.org> > > Sent: Monday, May 25, 2020 9:46 AM > > To: Nicolas TOROMANOFF <nicolas.toromanoff@st.com> > > Subject: Re: [PATCH 5/5] crypto: stm32/crc: protect from concurrent accesses > > > > On Mon, 25 May 2020 at 09:24, Nicolas TOROMANOFF > > <nicolas.toromanoff@st.com> wrote: > > > > > > Hello, > > > > > > > -----Original Message----- > > > > From: Ard Biesheuvel <ardb@kernel.org> > > > > Sent: Friday, May 22, 2020 6:12 PM> > > > > On Tue, 12 May 2020 at 16:13, Nicolas Toromanoff > > > > <nicolas.toromanoff@st.com> wrote: > > > > > > > > > > Protect STM32 CRC device from concurrent accesses. > > > > > > > > > > As we create a spinlocked section that increase with buffer size, > > > > > we provide a module parameter to release the pressure by splitting > > > > > critical section in chunks. > > > > > > > > > > Size of each chunk is defined in burst_size module parameter. > > > > > By default burst_size=0, i.e. don't split incoming buffer. > > > > > > > > > > Signed-off-by: Nicolas Toromanoff <nicolas.toromanoff@st.com> > > > > > > > > Would you mind explaining the usage model here? It looks like you > > > > are sharing a CRC hardware accelerator with a synchronous interface > > > > between different users by using spinlocks? You are aware that this > > > > will tie up the waiting CPUs completely during this time, right? So > > > > it would be much better to use a mutex here. Or perhaps it would > > > > make more sense to fall back to a s/w based CRC routine if the h/w is tied up > > working for another task? > > > > > > I know mutex are more acceptable here, but shash _update() and _init() > > > may be call from any context, and so I cannot take a mutex. > > > And to protect my concurrent HW access I only though about spinlock. > > > Due to possible constraint on CPUs, I add a burst_size option to force > > > slitting long buffer into smaller one, and so decrease time we take the lock. > > > But I didn't though to fallback to software CRC. > > > > > > I'll do a patch on top. > > > In in the burst_update() function I'll use a spin_trylock_irqsave() and use > > software CRC32 if HW is already in use. > > > > > > > Right. I didn't even notice that you were keeping interrupts disabled the whole > > time when using the h/w block. That means that any serious use of this h/w > > block will make IRQ latency go through the roof. > > > > I recommend that you go back to the drawing board on this driver, rather than > > papering over the issues with a spin_trylock(). Perhaps it would be better to > > model it as a ahash (even though the h/w block itself is synchronous) and use a > > kthread to feed in the data. > > I thought when I updated the driver to move to a ahash interface, but the main usage > of crc32 is the ext4 fs, that calls the shash API. > Commit 877b5691f27a ("crypto: shash - remove shash_desc::flags") removed possibility > to sleep in shash callback. (before this commit and with MAY_SLEEP option set, using > a mutex may have been fine). > According to that commit's log, sleeping is never fine for shash(), since it uses kmap_atomic() when iterating over the scatterlist. > By now the solution I see is to use the spin_trylock_irqsave(), fallback to software crc *AND* capping burst_size > to ensure the locked section stay reasonable. > > Does this seems acceptable ? > If the reason for disabling interrupts is to avoid deadlocks, wouldn't the switch to trylock() with a software fallback allow us to keep interrupts enabled? _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2020-05-25 9:07 UTC|newest] Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-05-12 14:11 [PATCH 0/5] STM32 CRC update Nicolas Toromanoff 2020-05-12 14:11 ` Nicolas Toromanoff 2020-05-12 14:11 ` [PATCH 1/5] crypto: stm32/crc: fix ext4 chksum BUG_ON() Nicolas Toromanoff 2020-05-12 14:11 ` Nicolas Toromanoff 2020-05-12 14:11 ` [PATCH 2/5] crypto: stm32/crc: fix run-time self test issue Nicolas Toromanoff 2020-05-12 14:11 ` Nicolas Toromanoff 2020-05-12 14:11 ` [PATCH 3/5] crypto: stm32/crc: fix multi-instance Nicolas Toromanoff 2020-05-12 14:11 ` Nicolas Toromanoff 2020-05-12 14:11 ` [PATCH 4/5] crypto: stm32/crc: don't sleep in runtime pm Nicolas Toromanoff 2020-05-12 14:11 ` Nicolas Toromanoff 2020-05-12 14:11 ` [PATCH 5/5] crypto: stm32/crc: protect from concurrent accesses Nicolas Toromanoff 2020-05-12 14:11 ` Nicolas Toromanoff 2020-05-22 16:11 ` Ard Biesheuvel 2020-05-22 16:11 ` Ard Biesheuvel 2020-05-25 7:24 ` Nicolas TOROMANOFF 2020-05-25 7:24 ` Nicolas TOROMANOFF 2020-05-25 7:46 ` Ard Biesheuvel 2020-05-25 7:46 ` Ard Biesheuvel 2020-05-25 9:01 ` Nicolas TOROMANOFF 2020-05-25 9:01 ` Nicolas TOROMANOFF 2020-05-25 9:07 ` Ard Biesheuvel [this message] 2020-05-25 9:07 ` Ard Biesheuvel 2020-05-25 11:49 ` Nicolas TOROMANOFF 2020-05-25 11:49 ` Nicolas TOROMANOFF 2020-05-25 12:01 ` Ard Biesheuvel 2020-05-25 12:01 ` Ard Biesheuvel 2020-05-22 14:13 ` [PATCH 0/5] STM32 CRC update Herbert Xu 2020-05-22 14:13 ` Herbert Xu
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to='CAMj1kXFwt6cs-MJhAeMRF4-yiddm=ezq=qvSjA_sRAX+_Gdqhw@mail.gmail.com' \ --to=ardb@kernel.org \ --cc=alexandre.torgue@st.com \ --cc=davem@davemloft.net \ --cc=ebiggers@kernel.org \ --cc=herbert@gondor.apana.org.au \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-crypto@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-stm32@st-md-mailman.stormreply.com \ --cc=mcoquelin.stm32@gmail.com \ --cc=nicolas.toromanoff@st.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.