Re: [PATCH v9 00/20] simplify crypto wait for async op
On Tue, Oct 17, 2017 at 5:06 PM, Russell King - ARM Linux wrote: > On Sun, Oct 15, 2017 at 10:19:45AM +0100, Gilad Ben-Yossef wrote: >> Many users of kernel async. crypto services have a pattern of >> starting an async. crypto op and than using a completion >> to wait for it to end. >> >> This patch set simplifies this common use case in two ways: >> >> First, by separating the return codes of the case where a >> request is queued to a backlog due to the provider being >> busy (-EBUSY) from the case the request has failed due >> to the provider being busy and backlogging is not enabled >> (-EAGAIN). >> >> Next, this change is than built on to create a generic API >> to wait for a async. crypto operation to complete. >> >> The end result is a smaller code base and an API that is >> easier to use and more difficult to get wrong. >> >> The patch set was boot tested on x86_64 and arm64 which >> at the very least tests the crypto users via testmgr and >> tcrypt but I do note that I do not have access to some >> of the HW whose drivers are modified nor do I claim I was >> able to test all of the corner cases. >> >> The patch set is based upon linux-next release tagged >> next-20171013. > > Has there been any performance impact analysis of these changes? I > ended up with patches for one of the crypto drivers which converted > its interrupt handling to threaded interrupts being reverted because > it caused a performance degredation. > > Moving code to latest APIs to simplify it is not always beneficial. I agree with the sentiment but I believe this one is justified. This patch set basically does 3 things: 1. Replace one immediate value (-EBUSY) by another (-EAGAIN). Mostly it's just s/EBUSY/EAGAIN/g. In very few places this resulted very trivial code changes. I don't foresee this having any effect on performance. 2. Removal of some conditions and/or conditional jumps that were used to discern between two different cases which are now now easily tested for by the different return value. If at all, this will be an increase in performance, although I don't expect it to be noticeable. 3. Replacing a whole bunch of open coded code and data structures which were pretty much cut and pasted from the Documentation and therefore identical, with a single copy thereof. Every place that I found that deviated slightly from the identical pattern, it turned out to be a bug of some sorts and patches for those were sent and accepted already. So, we might be losing a few inline optimization opportunities but we're gaining better cache utilization. Again, I don't expect any of this to have a noticeable effect to either direction. I did run the changed code as best I could and did not notice any performance changes and none of the testers and maintainers that ACKed mentioned any. Having said that, it's a big change that touches many places, sub-systems and drivers. I do not claim to have thoroughly tested for performance all the changes in person. In some cases, I don't even have access to the specialized hardware. I did get a reasonable amount of review and testers I believe - would always love to see more :-) Many thanks, Gilad -- Gilad Ben-Yossef Chief Coffee Drinker "If you take a class in large-scale robotics, can you end up in a situation where the homework eats your dog?" -- Jean-Baptiste Queru -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v9 00/20] simplify crypto wait for async op
On Sun, Oct 15, 2017 at 10:19:45AM +0100, Gilad Ben-Yossef wrote: > Many users of kernel async. crypto services have a pattern of > starting an async. crypto op and than using a completion > to wait for it to end. > > This patch set simplifies this common use case in two ways: > > First, by separating the return codes of the case where a > request is queued to a backlog due to the provider being > busy (-EBUSY) from the case the request has failed due > to the provider being busy and backlogging is not enabled > (-EAGAIN). > > Next, this change is than built on to create a generic API > to wait for a async. crypto operation to complete. > > The end result is a smaller code base and an API that is > easier to use and more difficult to get wrong. > > The patch set was boot tested on x86_64 and arm64 which > at the very least tests the crypto users via testmgr and > tcrypt but I do note that I do not have access to some > of the HW whose drivers are modified nor do I claim I was > able to test all of the corner cases. > > The patch set is based upon linux-next release tagged > next-20171013. Has there been any performance impact analysis of these changes? I ended up with patches for one of the crypto drivers which converted its interrupt handling to threaded interrupts being reverted because it caused a performance degredation. Moving code to latest APIs to simplify it is not always beneficial. -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up According to speedtest.net: 8.21Mbps down 510kbps up -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v9 00/20] simplify crypto wait for async op
On Tue, Oct 17, 2017 at 02:55:21PM +0300, Gilad Ben-Yossef wrote: > > Would you mind if we used ENOSPC instead of E2BIG? > > "No space left on device" seems more appropriate than > "Argument list too long". It's fine by me. Thanks, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v9 00/20] simplify crypto wait for async op
On Sun, Oct 15, 2017 at 6:38 PM, Herbert Xu wrote: > > On Sun, Oct 15, 2017 at 10:19:45AM +0100, Gilad Ben-Yossef wrote: > > > > Changes from v8: > > - Remove the translation of EAGAIN return code to the > > previous return code of EBUSY for the user space > > interface of algif as no one seems to rely on it as > > requested by Herbert Xu. > > Sorry, but I forgot to mention that EAGAIN is not a good value > to use because it's used by the network system calls for other > meanings (interrupted by a signal). > > So if we stop doing the translation then we also need to pick > a different value, perhaps E2BIG or something similar that have > no current use within the crypto API or network API. Yes, I see what you mean. With a netlink based interface this can be confusing to debug. Would you mind if we used ENOSPC instead of E2BIG? "No space left on device" seems more appropriate than "Argument list too long". Thanks, Gilad -- Gilad Ben-Yossef Chief Coffee Drinker "If you take a class in large-scale robotics, can you end up in a situation where the homework eats your dog?" -- Jean-Baptiste Queru -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v9 00/20] simplify crypto wait for async op
On Sun, Oct 15, 2017 at 10:19:45AM +0100, Gilad Ben-Yossef wrote: > > Changes from v8: > - Remove the translation of EAGAIN return code to the > previous return code of EBUSY for the user space > interface of algif as no one seems to rely on it as > requested by Herbert Xu. Sorry, but I forgot to mention that EAGAIN is not a good value to use because it's used by the network system calls for other meanings (interrupted by a signal). So if we stop doing the translation then we also need to pick a different value, perhaps E2BIG or something similar that have no current use within the crypto API or network API. Thanks, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v9 00/20] simplify crypto wait for async op
Many users of kernel async. crypto services have a pattern of starting an async. crypto op and than using a completion to wait for it to end. This patch set simplifies this common use case in two ways: First, by separating the return codes of the case where a request is queued to a backlog due to the provider being busy (-EBUSY) from the case the request has failed due to the provider being busy and backlogging is not enabled (-EAGAIN). Next, this change is than built on to create a generic API to wait for a async. crypto operation to complete. The end result is a smaller code base and an API that is easier to use and more difficult to get wrong. The patch set was boot tested on x86_64 and arm64 which at the very least tests the crypto users via testmgr and tcrypt but I do note that I do not have access to some of the HW whose drivers are modified nor do I claim I was able to test all of the corner cases. The patch set is based upon linux-next release tagged next-20171013. Changes from v8: - Remove the translation of EAGAIN return code to the previous return code of EBUSY for the user space interface of algif as no one seems to rely on it as requested by Herbert Xu. Changes from v7: - Turn -EBUSY to -EAGAIN also in crypto using net code which I missed before, as has been pointed out by Harsh Jain. Changes from v6: - Fix brown paper bag compile error on marvell/cesa code. Changes from v5: - Remove redundant new line as spotted by Jonathan Cameron. - Reworded dm-verity change commit message to better clarify potential issue averted by change as pointed out by Mikulas Patocka. Changes from v4: - Rebase on top of latest algif changes from Stephan Mueller. - Fix typo in ccp patch title. Changes from v3: - Instead of changing the return code to indicate backlog queueing, change the return code to indicate transient busy state, as suggested by Herbert Xu. Changes from v2: - Patch title changed from "introduce crypto wait for async op" to better reflect the current state. - Rebase on top of latest linux-next. - Add a new return code of -EIOCBQUEUED for backlog queueing, as suggested by Herbert Xu. - Transform more users to the new API. - Update the drbg change to account for new init as indicated by Stephan Muller. Changes from v1: - Address review comments from Eric Biggers. - Separated out bug fixes of existing code and rebase on top of that patch set. - Rename 'ecr' to 'wait' in fscrypto code. - Split patch introducing the new API from the change moving over the algif code which it originated from to the new API. - Inline crypto_wait_req(). - Some code indentation fixes. Gilad Ben-Yossef (20): crypto: change transient busy return code to -EAGAIN crypto: ccp: use -EAGAIN for transient busy indication net: use -EAGAIN for transient busy indication crypto: remove redundant backlog checks on EBUSY crypto: marvell/cesa: remove redundant backlog checks on EBUSY crypto: introduce crypto wait for async op crypto: move algif to generic async completion crypto: move pub key to generic async completion crypto: move drbg to generic async completion crypto: move gcm to generic async completion crypto: move testmgr to generic async completion fscrypt: move to generic async completion dm: move dm-verity to generic async completion cifs: move to generic async completion ima: move to generic async completion crypto: tcrypt: move to generic async completion crypto: talitos: move to generic async completion crypto: qce: move to generic async completion crypto: mediatek: move to generic async completion crypto: adapt api sample to use async. op wait Documentation/crypto/api-samples.rst | 52 ++--- crypto/af_alg.c | 27 - crypto/ahash.c | 12 +-- crypto/algapi.c | 6 +- crypto/algif_aead.c | 8 +- crypto/algif_hash.c | 30 +++--- crypto/algif_skcipher.c | 9 +- crypto/api.c | 13 +++ crypto/asymmetric_keys/public_key.c | 28 + crypto/cryptd.c | 4 +- crypto/cts.c | 6 +- crypto/drbg.c| 36 ++- crypto/gcm.c | 32 ++ crypto/lrw.c | 8 +- crypto/rsa-pkcs1pad.c| 16 +-- crypto/tcrypt.c | 84 +-- crypto/testmgr.c | 204 --- crypto/xts.c | 8 +- drivers/crypto/ccp/ccp-crypto-main.c | 8 +- drivers/crypto/ccp/ccp-dev.c | 7 +- drivers/crypto/marvell/cesa.c| 3 +- drivers/crypto/marvell/cesa.h| 2 +- drivers/crypto/mediatek/mtk-aes.c| 31 +- drivers/crypto/qce/sha.c | 30 +- drivers/crypto/talitos.c | 38 +-- drivers/md/dm-verity-target.c| 81 -
