* 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 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.