* Re: [CRYPTO]: Only reschedule if !in_atomic() [not found] <200505232300.j4NN07lE012726@hera.kernel.org> @ 2005-05-23 23:28 ` Andrew Morton 2005-05-23 23:58 ` Benjamin Herrenschmidt 2005-05-24 2:21 ` Herbert Xu 2005-05-24 6:40 ` Arjan van de Ven 1 sibling, 2 replies; 11+ messages in thread From: Andrew Morton @ 2005-05-23 23:28 UTC (permalink / raw) To: Linux Kernel Mailing List; +Cc: linux-crypto Linux Kernel Mailing List <linux-kernel@vger.kernel.org> wrote: > > The netlink gfp_any() problem made me double-check the uses of in_softirq() > in crypto/*. It seems to me that we should be checking in_atomic() instead > of in_softirq() in crypto_yield. Otherwise people calling the crypto ops > with spin locks held or preemption disabled will get burnt, right? > Sort-of, but the code is still wrong. > > crypto/internal.h | 2 +- > 1 files changed, 1 insertion(+), 1 deletion(-) > > Index: crypto/internal.h > =================================================================== > --- dade029a8df8b249d14282d8f8023a0de0f6c1e7/crypto/internal.h (mode:100644 sha1:e68e43886d3cc23439f30210e88b517911bf395e) > +++ c48106158bce4c7af328c486b7f33ad2133459ee/crypto/internal.h (mode:100644 sha1:964b9a60ca24413f07b1fe8410f7ac3198642135) > @@ -38,7 +38,7 @@ static inline void crypto_kunmap(void *v > > static inline void crypto_yield(struct crypto_tfm *tfm) > { > - if (!in_softirq()) > + if (!in_atomic()) > cond_resched(); > } This code can cause deadlocks on CONFIG_SMP && !CONFIG_PREEMPT kernels. Please see http://lkml.org/lkml/2005/3/10/358 You (the programmer) *have* to know what context you're running in before doing a voluntary yield. There is simply no way to work this out at runtime. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [CRYPTO]: Only reschedule if !in_atomic() 2005-05-23 23:28 ` [CRYPTO]: Only reschedule if !in_atomic() Andrew Morton @ 2005-05-23 23:58 ` Benjamin Herrenschmidt 2005-05-24 0:24 ` Andrew Morton 2005-05-24 2:21 ` Herbert Xu 1 sibling, 1 reply; 11+ messages in thread From: Benjamin Herrenschmidt @ 2005-05-23 23:58 UTC (permalink / raw) To: Andrew Morton; +Cc: Linux Kernel Mailing List, linux-crypto On Mon, 2005-05-23 at 16:28 -0700, Andrew Morton wrote: > This code can cause deadlocks on CONFIG_SMP && !CONFIG_PREEMPT kernels. > > Please see http://lkml.org/lkml/2005/3/10/358 > > You (the programmer) *have* to know what context you're running in before > doing a voluntary yield. There is simply no way to work this out at > runtime. Hrm... Linus just merged it though... Ben. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [CRYPTO]: Only reschedule if !in_atomic() 2005-05-23 23:58 ` Benjamin Herrenschmidt @ 2005-05-24 0:24 ` Andrew Morton 0 siblings, 0 replies; 11+ messages in thread From: Andrew Morton @ 2005-05-24 0:24 UTC (permalink / raw) To: Benjamin Herrenschmidt; +Cc: linux-kernel, linux-crypto Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote: > > On Mon, 2005-05-23 at 16:28 -0700, Andrew Morton wrote: > > > This code can cause deadlocks on CONFIG_SMP && !CONFIG_PREEMPT kernels. > > > > Please see http://lkml.org/lkml/2005/3/10/358 > > > > You (the programmer) *have* to know what context you're running in before > > doing a voluntary yield. There is simply no way to work this out at > > runtime. > > Hrm... Linus just merged it though... > The old version was: if (!in_softirq()) cond_resched(); Which I guess is OK if the programmer knows that this code is only ever called a) from softirq context or b) from process context with no locks/smp_processor_id/etc held. The new version is: if (!in_atomic()) cond_resched(); which happens to still be correct as long as a) and b) still hold, which I assume they do. Both versions are deadlocky if b) is violated. So. It sucks before and it sucks after, but we might not be deadlocky. Problem is, !CONFIG_PREEMPT also disable the beancounting which might_sleep() depends upon, so it's harder to tell whether all callers are correct. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [CRYPTO]: Only reschedule if !in_atomic() 2005-05-23 23:28 ` [CRYPTO]: Only reschedule if !in_atomic() Andrew Morton 2005-05-23 23:58 ` Benjamin Herrenschmidt @ 2005-05-24 2:21 ` Herbert Xu 2005-05-24 2:31 ` Andrew Morton [not found] ` <20050523.193612.08320356.davem@davemloft.net> 1 sibling, 2 replies; 11+ messages in thread From: Herbert Xu @ 2005-05-24 2:21 UTC (permalink / raw) To: Andrew Morton Cc: Linux Kernel Mailing List, linux-crypto, David S. Miller, James Morris On Mon, May 23, 2005 at 11:28:06PM +0000, Andrew Morton wrote: > > This code can cause deadlocks on CONFIG_SMP && !CONFIG_PREEMPT kernels. > > Please see http://lkml.org/lkml/2005/3/10/358 > > You (the programmer) *have* to know what context you're running in before > doing a voluntary yield. There is simply no way to work this out at > runtime. You're right. Perhaps we should code this into the crypto API instead? For instance, we can have a tfm flag that says whether we can sleep or not. Dave & James, What do you think? Cheers, -- Visit Openswan at http://www.openswan.org/ Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [CRYPTO]: Only reschedule if !in_atomic() 2005-05-24 2:21 ` Herbert Xu @ 2005-05-24 2:31 ` Andrew Morton 2005-05-24 2:43 ` Herbert Xu 2005-05-24 13:32 ` Bill Davidsen [not found] ` <20050523.193612.08320356.davem@davemloft.net> 1 sibling, 2 replies; 11+ messages in thread From: Andrew Morton @ 2005-05-24 2:31 UTC (permalink / raw) To: Herbert Xu; +Cc: linux-kernel, linux-crypto, davem, jmorris Herbert Xu <herbert@gondor.apana.org.au> wrote: > > Perhaps we should code this into the crypto API instead? For instance, > we can have a tfm flag that says whether we can sleep or not. Are you sure it's actually needed? Have significant scheduling latencies actually been observed? Bear in mind that anyone who cares a lot about latency will be running CONFIG_PREEMPT kernels, in which case the whole thing is redundant anyway. I generally take the position that if we're going to put a scheduling point into a non-premept kernel then it'd better be for a pretty bad latency point - more than 10 milliseconds, say. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [CRYPTO]: Only reschedule if !in_atomic() 2005-05-24 2:31 ` Andrew Morton @ 2005-05-24 2:43 ` Herbert Xu 2005-05-24 3:20 ` James Morris 2005-05-24 13:32 ` Bill Davidsen 1 sibling, 1 reply; 11+ messages in thread From: Herbert Xu @ 2005-05-24 2:43 UTC (permalink / raw) To: Andrew Morton; +Cc: linux-kernel, linux-crypto, davem, jmorris On Mon, May 23, 2005 at 07:31:16PM -0700, Andrew Morton wrote: > > Are you sure it's actually needed? Have significant scheduling latencies > actually been observed? I certainly don't have any problems with removing the yield altogether. > Bear in mind that anyone who cares a lot about latency will be running > CONFIG_PREEMPT kernels, in which case the whole thing is redundant anyway. > I generally take the position that if we're going to put a scheduling point > into a non-premept kernel then it'd better be for a pretty bad latency > point - more than 10 milliseconds, say. The crypt() function can easily take more than 10 milliseconds with a large enough buffer. James & Dave, do you have any opinions on this? Cheers, -- Visit Openswan at http://www.openswan.org/ Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [CRYPTO]: Only reschedule if !in_atomic() 2005-05-24 2:43 ` Herbert Xu @ 2005-05-24 3:20 ` James Morris 2005-05-24 3:50 ` Herbert Xu 0 siblings, 1 reply; 11+ messages in thread From: James Morris @ 2005-05-24 3:20 UTC (permalink / raw) To: Herbert Xu; +Cc: Andrew Morton, linux-kernel, linux-crypto, davem On Tue, 24 May 2005, Herbert Xu wrote: > On Mon, May 23, 2005 at 07:31:16PM -0700, Andrew Morton wrote: > > > > Are you sure it's actually needed? Have significant scheduling latencies > > actually been observed? > > I certainly don't have any problems with removing the yield altogether. > > > Bear in mind that anyone who cares a lot about latency will be running > > CONFIG_PREEMPT kernels, in which case the whole thing is redundant anyway. > > I generally take the position that if we're going to put a scheduling point > > into a non-premept kernel then it'd better be for a pretty bad latency > > point - more than 10 milliseconds, say. > > The crypt() function can easily take more than 10 milliseconds with > a large enough buffer. > > James & Dave, do you have any opinions on this? a) remove the scheudling point and see if anyone complains b) if so, add a flag - James -- James Morris <jmorris@redhat.com> ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [CRYPTO]: Only reschedule if !in_atomic() 2005-05-24 3:20 ` James Morris @ 2005-05-24 3:50 ` Herbert Xu 0 siblings, 0 replies; 11+ messages in thread From: Herbert Xu @ 2005-05-24 3:50 UTC (permalink / raw) To: James Morris; +Cc: Andrew Morton, linux-kernel, linux-crypto, davem On Mon, May 23, 2005 at 11:20:26PM -0400, James Morris wrote: > > a) remove the scheudling point and see if anyone complains > b) if so, add a flag OK. I think we should go with the flag though since it could also be used for memory allocation. I've just added some code which allocates a scratch space for unaligned input to the VIA Padlock (IPv4 ESP traffic is normally unaligned due to the 20-byte IP header). It could use this flag to determine whether it should do GFP_KERNEL or GFP_ATOMIC. Actually, has anyone considered using a 4-byte IP option padding? It's legal per RFC-791 but it'd be interesting to know how well it works in the field. Cheers, -- Visit Openswan at http://www.openswan.org/ Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [CRYPTO]: Only reschedule if !in_atomic() 2005-05-24 2:31 ` Andrew Morton 2005-05-24 2:43 ` Herbert Xu @ 2005-05-24 13:32 ` Bill Davidsen 1 sibling, 0 replies; 11+ messages in thread From: Bill Davidsen @ 2005-05-24 13:32 UTC (permalink / raw) To: Andrew Morton; +Cc: linux-kernel, linux-crypto, davem, jmorris Andrew Morton wrote: > Herbert Xu <herbert@gondor.apana.org.au> wrote: > >>Perhaps we should code this into the crypto API instead? For instance, >> we can have a tfm flag that says whether we can sleep or not. > > > Are you sure it's actually needed? Have significant scheduling latencies > actually been observed? > > Bear in mind that anyone who cares a lot about latency will be running > CONFIG_PREEMPT kernels, in which case the whole thing is redundant anyway. > I generally take the position that if we're going to put a scheduling point > into a non-premept kernel then it'd better be for a pretty bad latency > point - more than 10 milliseconds, say. > People do run crypto on old slow machines, and also laptops configured to use as little power as possible. I wouldn't be surprised if latencies got in the >10ms range pretty regularly on some systems which are pretty mainstream. Just my read on it, if a flag will prevent deadlock without relying on callers doing the right thing, that's probably a desirable change WRT future stability. ^ permalink raw reply [flat|nested] 11+ messages in thread
[parent not found: <20050523.193612.08320356.davem@davemloft.net>]
* Re: [CRYPTO]: Only reschedule if !in_atomic() [not found] ` <20050523.193612.08320356.davem@davemloft.net> @ 2005-05-24 3:47 ` Herbert Xu 0 siblings, 0 replies; 11+ messages in thread From: Herbert Xu @ 2005-05-24 3:47 UTC (permalink / raw) To: David S. Miller; +Cc: akpm, linux-kernel, linux-crypto, jmorris On Mon, May 23, 2005 at 07:36:12PM -0700, David S. Miller wrote: > > See how ugly this stuff gets once you start letting some people call > this stuff with locks and some not? But we already do that anyway. For example, IPsec calls the crypto functions with a spin lock on the xfrm_state. As it is, you're allowed to call crypto functions while holding spin locks if and only if you're in softirq context. Incidentally, that is something I intend on changing. There is no reason why we can't do away with that spin lock on the fast path for IPsec. > Crypto operations, especially the software operations, are extremely > expensive compute bound tasks. It is very desirable, as a result, for > them to be allowed to relinquish the cpu from time to time. Agreed. > That being said, I guess a flag isn't so bad. The other thing we could do with a flag is to use it to set GFP flags for memory allocation. Cheers, -- Visit Openswan at http://www.openswan.org/ Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [CRYPTO]: Only reschedule if !in_atomic() [not found] <200505232300.j4NN07lE012726@hera.kernel.org> 2005-05-23 23:28 ` [CRYPTO]: Only reschedule if !in_atomic() Andrew Morton @ 2005-05-24 6:40 ` Arjan van de Ven 1 sibling, 0 replies; 11+ messages in thread From: Arjan van de Ven @ 2005-05-24 6:40 UTC (permalink / raw) To: Linux Kernel Mailing List; +Cc: davem > - if (!in_softirq()) > + if (!in_atomic()) > cond_resched(); this looks wrong. in_atomic() isn't doing what I think you think it does; for one it doesn't get set inside spinlock regions (unless preempt is enabled) ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2005-05-24 13:34 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <200505232300.j4NN07lE012726@hera.kernel.org>
2005-05-23 23:28 ` [CRYPTO]: Only reschedule if !in_atomic() Andrew Morton
2005-05-23 23:58 ` Benjamin Herrenschmidt
2005-05-24 0:24 ` Andrew Morton
2005-05-24 2:21 ` Herbert Xu
2005-05-24 2:31 ` Andrew Morton
2005-05-24 2:43 ` Herbert Xu
2005-05-24 3:20 ` James Morris
2005-05-24 3:50 ` Herbert Xu
2005-05-24 13:32 ` Bill Davidsen
[not found] ` <20050523.193612.08320356.davem@davemloft.net>
2005-05-24 3:47 ` Herbert Xu
2005-05-24 6:40 ` Arjan van de Ven
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox