The recently added test_mb_ahash_speed() has clearly serious coding
style issues. Try to fix some of them:
1. Don't mix pr_err() and printk();
2. Don't wrap strings;
3. Properly align goto statement in if() block;
4. Align wrapped arguments on new line;
5. Don't wrap functions on first argument;
Signed-off-by: Krzysztof Kozlowski <[email protected]>
---
crypto/tcrypt.c | 33 ++++++++++++++-------------------
1 file changed, 14 insertions(+), 19 deletions(-)
diff --git a/crypto/tcrypt.c b/crypto/tcrypt.c
index 6ef78157a0ab..3788a607921e 100644
--- a/crypto/tcrypt.c
+++ b/crypto/tcrypt.c
@@ -608,12 +608,12 @@ static void test_mb_ahash_speed(const char *algo, unsigned int sec,
req[i] = ahash_request_alloc(tfm, GFP_KERNEL);
if (!req[i]) {
- printk(KERN_ERR "alg: hash: Failed to allocate "
- "request for %s\n", algo);
+ pr_err("alg: hash: Failed to allocate request for %s\n",
+ algo);
goto out_noreq;
}
ahash_request_set_callback(req[i], CRYPTO_TFM_REQ_MAY_BACKLOG,
- tcrypt_complete, &tresult[i]);
+ tcrypt_complete, &tresult[i]);
hash_buff = xbuf[i][0];
memcpy(hash_buff, ptext, 4096);
@@ -621,15 +621,14 @@ static void test_mb_ahash_speed(const char *algo, unsigned int sec,
j = 0;
- printk(KERN_INFO "\ntesting speed of %s (%s)\n", algo,
- get_driver_name(crypto_ahash, tfm));
+ pr_err("\ntesting speed of %s (%s)\n", algo,
+ get_driver_name(crypto_ahash, tfm));
for (i = 0; speed[i].blen != 0; i++) {
if (speed[i].blen > TVMEMSIZE * PAGE_SIZE) {
- printk(KERN_ERR
- "template (%u) too big for tvmem (%lu)\n",
- speed[i].blen, TVMEMSIZE * PAGE_SIZE);
- goto out;
+ pr_err("template (%u) too big for tvmem (%lu)\n",
+ speed[i].blen, TVMEMSIZE * PAGE_SIZE);
+ goto out;
}
if (speed[i].klen)
@@ -637,13 +636,12 @@ static void test_mb_ahash_speed(const char *algo, unsigned int sec,
for (k = 0; k < 8; ++k) {
sg_init_one(&sg[k][0], (void *) xbuf[k][0],
- speed[i].blen);
+ speed[i].blen);
ahash_request_set_crypt(req[k], sg[k],
result[k], speed[i].blen);
}
- printk(KERN_INFO "test%3u "
- "(%5u byte blocks,%5u bytes per update,%4u updates): ",
+ pr_err("test%3u (%5u byte blocks,%5u bytes per update,%4u updates): ",
i, speed[i].blen, speed[i].plen,
speed[i].blen / speed[i].plen);
@@ -653,9 +651,8 @@ static void test_mb_ahash_speed(const char *algo, unsigned int sec,
if (ret == -EBUSY || ret == -EINPROGRESS)
continue;
if (ret) {
- printk(KERN_ERR
- "alg (%s) something wrong, ret = %d ...\n",
- algo, ret);
+ pr_err("alg (%s) something wrong, ret = %d ...\n",
+ algo, ret);
goto out;
}
}
@@ -664,11 +661,9 @@ static void test_mb_ahash_speed(const char *algo, unsigned int sec,
for (k = 0; k < 8; ++k) {
struct tcrypt_result *tr = &tresult[k];
- ret = wait_for_completion_interruptible
- (&tr->completion);
+ ret = wait_for_completion_interruptible(&tr->completion);
if (ret)
- printk(KERN_ERR
- "alg(%s): hash: digest failed\n", algo);
+ pr_err("alg(%s): hash: digest failed\n", algo);
end[k] = get_cycles();
}
--
1.9.1
gcc 4.7.3 for ARM on Ubuntu couldn't link tcrypt module because of
division of s64:
ERROR: "__aeabi_ldivmod" [crypto/tcrypt.ko] undefined!
Fixes: 087bcd225c56 ("crypto: tcrypt - Add speed tests for SHA multibuffer algorithms")
Signed-off-by: Krzysztof Kozlowski <[email protected]>
---
crypto/tcrypt.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/crypto/tcrypt.c b/crypto/tcrypt.c
index 3788a607921e..1ff373352511 100644
--- a/crypto/tcrypt.c
+++ b/crypto/tcrypt.c
@@ -668,8 +668,8 @@ static void test_mb_ahash_speed(const char *algo, unsigned int sec,
}
printk("\nBlock: %lld cycles (%lld cycles/byte), %d bytes\n",
- (s64) (end[7]-start[0])/1,
- (s64) (end[7]-start[0])/(8*speed[i].blen),
+ (s64) (end[7]-start[0]),
+ (s64) div64_s64(end[7]-start[0], 8*speed[i].blen),
8*speed[i].blen);
}
ret = 0;
--
1.9.1
On Tue, Jun 28, 2016 at 09:23:06AM +0200, Krzysztof Kozlowski wrote:
> The recently added test_mb_ahash_speed() has clearly serious coding
> style issues. Try to fix some of them:
> 1. Don't mix pr_err() and printk();
> 2. Don't wrap strings;
> 3. Properly align goto statement in if() block;
> 4. Align wrapped arguments on new line;
> 5. Don't wrap functions on first argument;
>
> Signed-off-by: Krzysztof Kozlowski <[email protected]>
Patch applied. Thanks.
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
On Tue, Jun 28, 2016 at 09:23:07AM +0200, Krzysztof Kozlowski wrote:
> gcc 4.7.3 for ARM on Ubuntu couldn't link tcrypt module because of
> division of s64:
> ERROR: "__aeabi_ldivmod" [crypto/tcrypt.ko] undefined!
>
> Fixes: 087bcd225c56 ("crypto: tcrypt - Add speed tests for SHA multibuffer algorithms")
> Signed-off-by: Krzysztof Kozlowski <[email protected]>
I'd prefer to be consistent here and just use unsigned long like
every other speed test in tcrypt. This code has some serious
issues, such as not waiting for completion in case of an error.
Let me try to fix them up.
Thanks!
---8<---
Subject: crypto: tcrypt - Use unsigned long for mb ahash cycle counter
For the timescales we are working against there is no need to
go beyond unsigned long.
Signed-off-by: Herbert Xu <[email protected]>
diff --git a/crypto/tcrypt.c b/crypto/tcrypt.c
index 2f84583..1537a1c 100644
--- a/crypto/tcrypt.c
+++ b/crypto/tcrypt.c
@@ -415,7 +415,7 @@ char result[8][64];
struct ahash_request *req[8];
struct tcrypt_result tresult[8];
char *xbuf[8][XBUFSIZE];
-cycles_t start[8], end[8], mid;
+unsigned long start[8], end[8], mid;
static void test_mb_ahash_speed(const char *algo, unsigned int sec,
struct hash_speed *speed)
@@ -424,6 +424,7 @@ static void test_mb_ahash_speed(const char *algo, unsigned int sec,
void *hash_buff;
int ret = -ENOMEM;
struct crypto_ahash *tfm;
+ unsigned long cycles;
tfm = crypto_alloc_ahash(algo, 0, 0);
if (IS_ERR(tfm)) {
@@ -498,10 +499,9 @@ static void test_mb_ahash_speed(const char *algo, unsigned int sec,
end[k] = get_cycles();
}
- printk("\nBlock: %lld cycles (%lld cycles/byte), %d bytes\n",
- (s64) (end[7]-start[0])/1,
- (s64) (end[7]-start[0])/(8*speed[i].blen),
- 8*speed[i].blen);
+ cycles = end[7] - start[0];
+ printk("\nBlock: %6lu cycles (%4lu cycles/byte)\n",
+ cycles, cycles / (8 * speed[i].blen));
}
ret = 0;
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
On 06/28/2016 10:41 AM, Herbert Xu wrote:
> On Tue, Jun 28, 2016 at 09:23:07AM +0200, Krzysztof Kozlowski wrote:
>> gcc 4.7.3 for ARM on Ubuntu couldn't link tcrypt module because of
>> division of s64:
>> ERROR: "__aeabi_ldivmod" [crypto/tcrypt.ko] undefined!
>>
>> Fixes: 087bcd225c56 ("crypto: tcrypt - Add speed tests for SHA multibuffer algorithms")
>> Signed-off-by: Krzysztof Kozlowski <[email protected]>
>
> I'd prefer to be consistent here and just use unsigned long like
> every other speed test in tcrypt. This code has some serious
> issues, such as not waiting for completion in case of an error.
>
> Let me try to fix them up.
>
> Thanks!
>
> ---8<---
> Subject: crypto: tcrypt - Use unsigned long for mb ahash cycle counter
>
> For the timescales we are working against there is no need to
> go beyond unsigned long.
>
> Signed-off-by: Herbert Xu <[email protected]>
>
Works fine for me:
Reported-by: Krzysztof Kozlowski <[email protected]>
Reviewed-by: Krzysztof Kozlowski <[email protected]>
Tested-by: Krzysztof Kozlowski <[email protected]>
Best regards,
Krzysztof
This patch resolves a number of issues with the mb speed test
function:
* The tfm is never freed.
* Memory is allocated even when we're not using mb.
* When an error occurs we don't wait for completion for other requests.
* When an error occurs during allocation we may leak memory.
* The test function ignores plen but still runs for plen != blen.
* The backlog flag is incorrectly used (may crash).
This patch tries to resolve all these issues as well as making
the code consistent with the existing hash speed testing function.
Signed-off-by: Herbert Xu <[email protected]>
diff --git a/crypto/tcrypt.c b/crypto/tcrypt.c
index 1537a1c..8208026 100644
--- a/crypto/tcrypt.c
+++ b/crypto/tcrypt.c
@@ -409,54 +409,61 @@ static inline int do_one_ahash_op(struct ahash_request *req, int ret)
return ret;
}
-char ptext[4096];
-struct scatterlist sg[8][8];
-char result[8][64];
-struct ahash_request *req[8];
-struct tcrypt_result tresult[8];
-char *xbuf[8][XBUFSIZE];
-unsigned long start[8], end[8], mid;
+struct test_mb_ahash_data {
+ struct scatterlist sg[TVMEMSIZE];
+ char result[64];
+ struct ahash_request *req;
+ struct tcrypt_result tresult;
+ char *xbuf[XBUFSIZE];
+};
static void test_mb_ahash_speed(const char *algo, unsigned int sec,
- struct hash_speed *speed)
+ struct hash_speed *speed)
{
- unsigned int i, j, k;
- void *hash_buff;
- int ret = -ENOMEM;
+ struct test_mb_ahash_data *data;
struct crypto_ahash *tfm;
+ unsigned long start, end;
unsigned long cycles;
+ unsigned int i, j, k;
+ int ret;
+
+ data = kzalloc(sizeof(*data) * 8, GFP_KERNEL);
+ if (!data)
+ return;
tfm = crypto_alloc_ahash(algo, 0, 0);
if (IS_ERR(tfm)) {
pr_err("failed to load transform for %s: %ld\n",
algo, PTR_ERR(tfm));
- return;
+ goto free_data;
}
+
for (i = 0; i < 8; ++i) {
- if (testmgr_alloc_buf(xbuf[i]))
- goto out_nobuf;
+ if (testmgr_alloc_buf(data[i].xbuf))
+ goto out;
- init_completion(&tresult[i].completion);
+ init_completion(&data[i].tresult.completion);
- req[i] = ahash_request_alloc(tfm, GFP_KERNEL);
- if (!req[i]) {
+ data[i].req = ahash_request_alloc(tfm, GFP_KERNEL);
+ if (!data[i].req) {
pr_err("alg: hash: Failed to allocate request for %s\n",
algo);
- goto out_noreq;
+ goto out;
}
- ahash_request_set_callback(req[i], CRYPTO_TFM_REQ_MAY_BACKLOG,
- tcrypt_complete, &tresult[i]);
- hash_buff = xbuf[i][0];
- memcpy(hash_buff, ptext, 4096);
+ ahash_request_set_callback(data[i].req, 0,
+ tcrypt_complete, &data[i].tresult);
+ test_hash_sg_init(data[i].sg);
}
- j = 0;
-
- pr_err("\ntesting speed of %s (%s)\n", algo,
- get_driver_name(crypto_ahash, tfm));
+ pr_info("\ntesting speed of multibuffer %s (%s)\n", algo,
+ get_driver_name(crypto_ahash, tfm));
for (i = 0; speed[i].blen != 0; i++) {
+ /* For some reason this only tests digests. */
+ if (speed[i].blen != speed[i].plen)
+ continue;
+
if (speed[i].blen > TVMEMSIZE * PAGE_SIZE) {
pr_err("template (%u) too big for tvmem (%lu)\n",
speed[i].blen, TVMEMSIZE * PAGE_SIZE);
@@ -466,53 +473,58 @@ static void test_mb_ahash_speed(const char *algo, unsigned int sec,
if (speed[i].klen)
crypto_ahash_setkey(tfm, tvmem[0], speed[i].klen);
- for (k = 0; k < 8; ++k) {
- sg_init_one(&sg[k][0], (void *) xbuf[k][0],
- speed[i].blen);
- ahash_request_set_crypt(req[k], sg[k],
- result[k], speed[i].blen);
- }
+ for (k = 0; k < 8; k++)
+ ahash_request_set_crypt(data[k].req, data[k].sg,
+ data[k].result, speed[i].blen);
- pr_err("test%3u (%5u byte blocks,%5u bytes per update,%4u updates): ",
+ pr_info("test%3u "
+ "(%5u byte blocks,%5u bytes per update,%4u updates): ",
i, speed[i].blen, speed[i].plen,
speed[i].blen / speed[i].plen);
- for (k = 0; k < 8; ++k) {
- start[k] = get_cycles();
- ret = crypto_ahash_digest(req[k]);
- if (ret == -EBUSY || ret == -EINPROGRESS)
+ start = get_cycles();
+
+ for (k = 0; k < 8; k++) {
+ ret = crypto_ahash_digest(data[k].req);
+ if (ret == -EINPROGRESS)
continue;
- if (ret) {
- pr_err("alg (%s) something wrong, ret = %d ...\n",
- algo, ret);
- goto out;
- }
+
+ if (ret)
+ break;
+
+ complete(&data[k].tresult.completion);
}
- mid = get_cycles();
- for (k = 0; k < 8; ++k) {
- struct tcrypt_result *tr = &tresult[k];
+ for (j = 0; j < k; j++) {
+ struct tcrypt_result *tr = &data[k].tresult;
- ret = wait_for_completion_interruptible(&tr->completion);
- if (ret)
- pr_err("alg(%s): hash: digest failed\n", algo);
- end[k] = get_cycles();
+ wait_for_completion(&tr->completion);
+ if (tr->err)
+ ret = tr->err;
}
- cycles = end[7] - start[0];
- printk("\nBlock: %6lu cycles (%4lu cycles/byte)\n",
- cycles, cycles / (8 * speed[i].blen));
+ end = get_cycles();
+ cycles = end - start;
+ pr_cont("%6lu cycles/operation, %4lu cycles/byte\n",
+ cycles, cycles / (8 * speed[i].blen));
+
+ if (ret) {
+ pr_err("At least one hashing failed ret=%d\n", ret);
+ break;
+ }
}
- ret = 0;
out:
for (k = 0; k < 8; ++k)
- ahash_request_free(req[k]);
-out_noreq:
+ ahash_request_free(data[k].req);
+
for (k = 0; k < 8; ++k)
- testmgr_free_buf(xbuf[k]);
-out_nobuf:
- return;
+ testmgr_free_buf(data[k].xbuf);
+
+ crypto_free_ahash(tfm);
+
+free_data:
+ kfree(data);
}
static int test_ahash_jiffies_digest(struct ahash_request *req, int blen,
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
On Tue, Jun 28, 2016 at 11:55 AM, Herbert Xu
<[email protected]> wrote:
> This patch resolves a number of issues with the mb speed test
> function:
>
> * The tfm is never freed.
> * Memory is allocated even when we're not using mb.
> * When an error occurs we don't wait for completion for other requests.
> * When an error occurs during allocation we may leak memory.
> * The test function ignores plen but still runs for plen != blen.
> * The backlog flag is incorrectly used (may crash).
>
> This patch tries to resolve all these issues as well as making
> the code consistent with the existing hash speed testing function.
>
> Signed-off-by: Herbert Xu <[email protected]>
>
Oops:
[root@odroidxu3 ~]# modprobe tcrypt mode=422
[ 23.466899]
[ 23.466899] testing speed of multibuffer sha1 (sha1-neon)
[ 23.472517] test 0 ( 16 byte blocks, 16 bytes per update, 1 updates):
[ 23.480312] BUG: spinlock bad magic on CPU#5, modprobe/285
[ 23.484918] lock: 0xecab06a8, .magic: 00000000, .owner: <none>/-1,
.owner_cpu: 0
[ 23.492374] CPU: 5 PID: 285 Comm: modprobe Not tainted
4.7.0-rc5-next-20160628-00003-g28618d2d8c0b #862
[ 23.501724] Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
[ 23.507828] [<c010d738>] (unwind_backtrace) from [<c010a4b0>]
(show_stack+0x10/0x14)
[ 23.515526] [<c010a4b0>] (show_stack) from [<c031f06c>]
(dump_stack+0x74/0x94)
[ 23.522711] [<c031f06c>] (dump_stack) from [<c0156f78>]
(do_raw_spin_lock+0x160/0x1a8)
[ 23.530605] [<c0156f78>] (do_raw_spin_lock) from [<c06bd34c>]
(wait_for_common+0x20/0x144)
[ 23.538853] [<c06bd34c>] (wait_for_common) from [<bf157898>]
(test_mb_ahash_speed.constprop.2+0x20c/0x354 [tcrypt])
[ 23.549196] [<bf157898>] (test_mb_ahash_speed.constprop.2 [tcrypt])
from [<bf159284>] (do_test+0x12c4/0x32ec [tcrypt])
[ 23.559848] [<bf159284>] (do_test [tcrypt]) from [<bf15f048>]
(tcrypt_mod_init+0x48/0xa4 [tcrypt])
[ 23.568776] [<bf15f048>] (tcrypt_mod_init [tcrypt]) from
[<c010178c>] (do_one_initcall+0x3c/0x16c)
[ 23.577703] [<c010178c>] (do_one_initcall) from [<c0193370>]
(do_init_module+0x5c/0x1ac)
[ 23.585764] [<c0193370>] (do_init_module) from [<c018659c>]
(load_module+0x1a30/0x1d08)
[ 23.593731] [<c018659c>] (load_module) from [<c0186a3c>]
(SyS_finit_module+0x8c/0x98)
[ 23.601530] [<c0186a3c>] (SyS_finit_module) from [<c0107900>]
(ret_fast_syscall+0x0/0x3c)
[ 23.609677] Unable to handle kernel NULL pointer dereference at
virtual address 00000000
[ 23.617728] pgd = ec888000
[ 23.620406] [00000000] *pgd=6ddc7835, *pte=00000000, *ppte=00000000
[ 23.626646] Internal error: Oops: 817 [#1] PREEMPT SMP ARM
[ 23.632106] Modules linked in: sha1_generic sha1_arm_neon sha1_arm
tcrypt(+) s5p_jpeg s5p_mfc v4l2_mem2mem videobuf2_dma_contig
v4l2_common videobuf2_memops videobuf2_v4l2 videobuf2_core videodev
media
[ 23.649956] CPU: 5 PID: 285 Comm: modprobe Not tainted
4.7.0-rc5-next-20160628-00003-g28618d2d8c0b #862
[ 23.659323] Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
[ 23.665383] task: eddcf800 ti: ec974000 task.ti: ec974000
[ 23.670757] PC is at wait_for_common+0x6c/0x144
[ 23.675262] LR is at unwind_frame+0x68/0x564
[ 23.679507] pc : [<c06bd398>] lr : [<c010d23c>] psr: 60070093
[ 23.679507] sp : ec975d30 ip : eddcf800 fp : 00000000
[ 23.690953] r10: 00000000 r9 : 00000001 r8 : 00000000
[ 23.696144] r7 : 00000002 r6 : ecab06a4 r5 : ecab06a8 r4 : 7fffffff
[ 23.702644] r3 : 00000000 r2 : ec975d40 r1 : ecab06b8 r0 : 00000001
[ 23.709141] Flags: nZCv IRQs off FIQs on Mode SVC_32 ISA ARM
Segment none
[ 23.716335] Control: 10c5387d Table: 6c88806a DAC: 00000051
[ 23.722050] Process modprobe (pid: 285, stack limit = 0xec974210)
[ 23.728116] Stack: (0xec975d30 to 0xec976000)
[ 23.732450] 5d20: bf15c0c0
00000001 eddcf800 c013aa58
[ 23.740601] 5d40: ecab06b8 00000000 00000000 00000008 bf15c0b4
ecab0620 bf15c0c0 ecab06a4
[ 23.748746] 5d60: 00000000 bf157898 00000001 ee3f6110 ecab0000
2196bd9e ec8af480 ec8af4c0
[ 23.756891] 5d80: 00000001 2196bd9e 000000c3 bf15c640 bf15c640
edee7940 bf15c4c0 bf15f000
[ 23.765037] 5da0: 00000124 00000000 c0183918 bf159284 bf15c640
edee7940 00000010 bf15c640
[ 23.773182] 5dc0: edee7940 bf15c4c0 bf15f000 00000124 00000000
bf15f048 bf15c4c0 00000001
[ 23.781328] 5de0: edee7940 c010178c ffffff04 ecac8000 00000000
8040003e ee801b80 8040003f
[ 23.789473] 5e00: c0b13154 c0a67b60 ef5e5000 ee801f00 c0183918
8040003e 00000080 ef5e5000
[ 23.797618] 5e20: ee801f00 ee801f00 c0a67b60 c0b025ac c0183918
c01d37dc bf15c4c0 00000001
[ 23.805764] 5e40: edee7940 bf15c4c0 00000001 00000124 00000003
c0193370 ec975f4c 00000001
[ 23.813909] 5e60: edee7880 ec975f4c 00000001 edee7880 bf15c4c0
c018659c bf15c4cc 00007fff
[ 23.822055] 5e80: bf15c4c0 c0184168 ed6f0300 00000000 c018398c
bf15c508 bf15c664 00000000
[ 23.830201] 5ea0: bf15c4cc bf15c628 00000000 0000b320 00000000
ec975f3c ec975f40 c01e17fc
[ 23.838346] 5ec0: 0000b320 00000000 00000000 00000000 00000000
00000000 00000000 00000000
[ 23.846491] 5ee0: 00000000 00000000 00000000 00000000 00000000
00000000 00000000 00000000
[ 23.854637] 5f00: 7fffffff 7fffffff 00000000 00000000 0003ff70
c0107ac4 ec974000 00000000
[ 23.862782] 5f20: 0003fe38 c0186a3c 7fffffff 00000000 00000003
0000b320 ed6f0300 f2781000
[ 23.870928] 5f40: 0000b320 00000000 bedfb924 f2781000 0000b320
f2787a88 f278792b f2788b00
[ 23.879074] 5f60: 00006664 00006c84 00000000 00000000 00000000
000012bc 00000021 00000022
[ 23.887219] 5f80: 00000019 00000000 00000015 00000000 00000000
0003fe08 00000008 0003e240
[ 23.895364] 5fa0: 0000017b c0107900 0003fe08 00000008 00000003
0003ff70 00000000 00000000
[ 23.903510] 5fc0: 0003fe08 00000008 0003e240 0000017b 0003ff70
00000008 0003ff70 0003fe38
[ 23.911655] 5fe0: bedfb928 bedfb918 00020b08 b6e206e0 60070010
00000003 00000000 00000000
[ 23.919806] [<c06bd398>] (wait_for_common) from [<bf157898>]
(test_mb_ahash_speed.constprop.2+0x20c/0x354 [tcrypt])
[ 23.930206] [<bf157898>] (test_mb_ahash_speed.constprop.2 [tcrypt])
from [<bf159284>] (do_test+0x12c4/0x32ec [tcrypt])
[ 23.940865] [<bf159284>] (do_test [tcrypt]) from [<bf15f048>]
(tcrypt_mod_init+0x48/0xa4 [tcrypt])
[ 23.949787] [<bf15f048>] (tcrypt_mod_init [tcrypt]) from
[<c010178c>] (do_one_initcall+0x3c/0x16c)
[ 23.958710] [<c010178c>] (do_one_initcall) from [<c0193370>]
(do_init_module+0x5c/0x1ac)
[ 23.966769] [<c0193370>] (do_init_module) from [<c018659c>]
(load_module+0x1a30/0x1d08)
[ 23.974741] [<c018659c>] (load_module) from [<c0186a3c>]
(SyS_finit_module+0x8c/0x98)
[ 23.982539] [<c0186a3c>] (SyS_finit_module) from [<c0107900>]
(ret_fast_syscall+0x0/0x3c)
[ 23.990685] Code: e28d2010 e98d1001 e5862018 e58d1010 (e5832000)
[ 23.996749] ---[ end trace ba7506845ca39b55 ]---
[ 24.001338] note: modprobe[285] exited with preempt_count 1
Segmentation fault
Best regards,
Krzysztof
On Tue, Jun 28, 2016 at 12:15:43PM +0200, Krzysztof Kozlowski wrote:
> Oops:
Thanks, there was a typo where it said k instead of j in the second
loop.
---8<---
This patch resolves a number of issues with the mb speed test
function:
* The tfm is never freed.
* Memory is allocated even when we're not using mb.
* When an error occurs we don't wait for completion for other requests.
* When an error occurs during allocation we may leak memory.
* The test function ignores plen but still runs for plen != blen.
* The backlog flag is incorrectly used (may crash).
This patch tries to resolve all these issues as well as making
the code consistent with the existing hash speed testing function.
Signed-off-by: Herbert Xu <[email protected]>
diff --git a/crypto/tcrypt.c b/crypto/tcrypt.c
index 1537a1c..68064fc 100644
--- a/crypto/tcrypt.c
+++ b/crypto/tcrypt.c
@@ -409,54 +409,61 @@ static inline int do_one_ahash_op(struct ahash_request *req, int ret)
return ret;
}
-char ptext[4096];
-struct scatterlist sg[8][8];
-char result[8][64];
-struct ahash_request *req[8];
-struct tcrypt_result tresult[8];
-char *xbuf[8][XBUFSIZE];
-unsigned long start[8], end[8], mid;
+struct test_mb_ahash_data {
+ struct scatterlist sg[TVMEMSIZE];
+ char result[64];
+ struct ahash_request *req;
+ struct tcrypt_result tresult;
+ char *xbuf[XBUFSIZE];
+};
static void test_mb_ahash_speed(const char *algo, unsigned int sec,
- struct hash_speed *speed)
+ struct hash_speed *speed)
{
- unsigned int i, j, k;
- void *hash_buff;
- int ret = -ENOMEM;
+ struct test_mb_ahash_data *data;
struct crypto_ahash *tfm;
+ unsigned long start, end;
unsigned long cycles;
+ unsigned int i, j, k;
+ int ret;
+
+ data = kzalloc(sizeof(*data) * 8, GFP_KERNEL);
+ if (!data)
+ return;
tfm = crypto_alloc_ahash(algo, 0, 0);
if (IS_ERR(tfm)) {
pr_err("failed to load transform for %s: %ld\n",
algo, PTR_ERR(tfm));
- return;
+ goto free_data;
}
+
for (i = 0; i < 8; ++i) {
- if (testmgr_alloc_buf(xbuf[i]))
- goto out_nobuf;
+ if (testmgr_alloc_buf(data[i].xbuf))
+ goto out;
- init_completion(&tresult[i].completion);
+ init_completion(&data[i].tresult.completion);
- req[i] = ahash_request_alloc(tfm, GFP_KERNEL);
- if (!req[i]) {
+ data[i].req = ahash_request_alloc(tfm, GFP_KERNEL);
+ if (!data[i].req) {
pr_err("alg: hash: Failed to allocate request for %s\n",
algo);
- goto out_noreq;
+ goto out;
}
- ahash_request_set_callback(req[i], CRYPTO_TFM_REQ_MAY_BACKLOG,
- tcrypt_complete, &tresult[i]);
- hash_buff = xbuf[i][0];
- memcpy(hash_buff, ptext, 4096);
+ ahash_request_set_callback(data[i].req, 0,
+ tcrypt_complete, &data[i].tresult);
+ test_hash_sg_init(data[i].sg);
}
- j = 0;
-
- pr_err("\ntesting speed of %s (%s)\n", algo,
- get_driver_name(crypto_ahash, tfm));
+ pr_info("\ntesting speed of multibuffer %s (%s)\n", algo,
+ get_driver_name(crypto_ahash, tfm));
for (i = 0; speed[i].blen != 0; i++) {
+ /* For some reason this only tests digests. */
+ if (speed[i].blen != speed[i].plen)
+ continue;
+
if (speed[i].blen > TVMEMSIZE * PAGE_SIZE) {
pr_err("template (%u) too big for tvmem (%lu)\n",
speed[i].blen, TVMEMSIZE * PAGE_SIZE);
@@ -466,53 +473,59 @@ static void test_mb_ahash_speed(const char *algo, unsigned int sec,
if (speed[i].klen)
crypto_ahash_setkey(tfm, tvmem[0], speed[i].klen);
- for (k = 0; k < 8; ++k) {
- sg_init_one(&sg[k][0], (void *) xbuf[k][0],
- speed[i].blen);
- ahash_request_set_crypt(req[k], sg[k],
- result[k], speed[i].blen);
- }
+ for (k = 0; k < 8; k++)
+ ahash_request_set_crypt(data[k].req, data[k].sg,
+ data[k].result, speed[i].blen);
- pr_err("test%3u (%5u byte blocks,%5u bytes per update,%4u updates): ",
+ pr_info("test%3u "
+ "(%5u byte blocks,%5u bytes per update,%4u updates): ",
i, speed[i].blen, speed[i].plen,
speed[i].blen / speed[i].plen);
- for (k = 0; k < 8; ++k) {
- start[k] = get_cycles();
- ret = crypto_ahash_digest(req[k]);
- if (ret == -EBUSY || ret == -EINPROGRESS)
+ start = get_cycles();
+
+ for (k = 0; k < 8; k++) {
+ ret = crypto_ahash_digest(data[k].req);
+ if (ret == -EINPROGRESS)
continue;
- if (ret) {
- pr_err("alg (%s) something wrong, ret = %d ...\n",
- algo, ret);
- goto out;
- }
+
+ if (ret)
+ break;
+
+ complete(&data[k].tresult.completion);
+ data[k].tresult.err = 0;
}
- mid = get_cycles();
- for (k = 0; k < 8; ++k) {
- struct tcrypt_result *tr = &tresult[k];
+ for (j = 0; j < k; j++) {
+ struct tcrypt_result *tr = &data[j].tresult;
- ret = wait_for_completion_interruptible(&tr->completion);
- if (ret)
- pr_err("alg(%s): hash: digest failed\n", algo);
- end[k] = get_cycles();
+ wait_for_completion(&tr->completion);
+ if (tr->err)
+ ret = tr->err;
}
- cycles = end[7] - start[0];
- printk("\nBlock: %6lu cycles (%4lu cycles/byte)\n",
- cycles, cycles / (8 * speed[i].blen));
+ end = get_cycles();
+ cycles = end - start;
+ pr_cont("%6lu cycles/operation, %4lu cycles/byte\n",
+ cycles, cycles / (8 * speed[i].blen));
+
+ if (ret) {
+ pr_err("At least one hashing failed ret=%d\n", ret);
+ break;
+ }
}
- ret = 0;
out:
for (k = 0; k < 8; ++k)
- ahash_request_free(req[k]);
-out_noreq:
+ ahash_request_free(data[k].req);
+
for (k = 0; k < 8; ++k)
- testmgr_free_buf(xbuf[k]);
-out_nobuf:
- return;
+ testmgr_free_buf(data[k].xbuf);
+
+ crypto_free_ahash(tfm);
+
+free_data:
+ kfree(data);
}
static int test_ahash_jiffies_digest(struct ahash_request *req, int blen,
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
On 06/28/2016 02:33 PM, Herbert Xu wrote:
> On Tue, Jun 28, 2016 at 12:15:43PM +0200, Krzysztof Kozlowski wrote:
>> Oops:
>
> Thanks, there was a typo where it said k instead of j in the second
> loop.
>
> ---8<---
> This patch resolves a number of issues with the mb speed test
> function:
>
> * The tfm is never freed.
> * Memory is allocated even when we're not using mb.
> * When an error occurs we don't wait for completion for other requests.
> * When an error occurs during allocation we may leak memory.
> * The test function ignores plen but still runs for plen != blen.
> * The backlog flag is incorrectly used (may crash).
>
> This patch tries to resolve all these issues as well as making
> the code consistent with the existing hash speed testing function.
>
> Signed-off-by: Herbert Xu <[email protected]>
Seems to work fine except:
1. The updates are always 1.
2. For bigger blocks it reports always 1 or 3 cycles per byte:
[root@odroidxu3 ~]# modprobe tcrypt mode=423
[ 70.129206]
[ 70.129206] testing speed of multibuffer sha256 (sha256-neon)
[ 70.135511] test 0 ( 16 byte blocks, 16 bytes per update, 1 updates): 4597 cycles/operation, 35 cycles/byte
[ 70.145826] test 2 ( 64 byte blocks, 64 bytes per update, 1 updates): 5762 cycles/operation, 11 cycles/byte
[ 70.156469] test 5 ( 256 byte blocks, 256 bytes per update, 1 updates): 10687 cycles/operation, 5 cycles/byte
[ 70.167125] test 8 ( 1024 byte blocks, 1024 bytes per update, 1 updates): 31485 cycles/operation, 3 cycles/byte
[ 70.177919] test 12 ( 2048 byte blocks, 2048 bytes per update, 1 updates): 59281 cycles/operation, 3 cycles/byte
[ 70.189780] test 16 ( 4096 byte blocks, 4096 bytes per update, 1 updates): 118097 cycles/operation, 3 cycles/byte
[ 70.204117] test 21 ( 8192 byte blocks, 8192 bytes per update, 1 updates): 232309 cycles/operation, 3 cycles/byte
[root@odroidxu3 ~]# modprobe tcrypt mode=422
[ 71.988248]
[ 71.988248] testing speed of multibuffer sha1 (sha1-neon)
[ 71.994097] test 0 ( 16 byte blocks, 16 bytes per update, 1 updates): 2506 cycles/operation, 19 cycles/byte
[ 72.004547] test 2 ( 64 byte blocks, 64 bytes per update, 1 updates): 2299 cycles/operation, 4 cycles/byte
[ 72.015152] test 5 ( 256 byte blocks, 256 bytes per update, 1 updates): 3535 cycles/operation, 1 cycles/byte
[ 72.025807] test 8 ( 1024 byte blocks, 1024 bytes per update, 1 updates): 9403 cycles/operation, 1 cycles/byte
[ 72.036401] test 12 ( 2048 byte blocks, 2048 bytes per update, 1 updates): 17142 cycles/operation, 1 cycles/byte
[ 72.047058] test 16 ( 4096 byte blocks, 4096 bytes per update, 1 updates): 33109 cycles/operation, 1 cycles/byte
[ 72.057821] test 21 ( 8192 byte blocks, 8192 bytes per update, 1 updates): 67750 cycles/operation, 1 cycles/byte
modprobe: ERROR: could not insert 'tcrypt': Resource temporarily unavailable
Is it expected?
Best regards,
Krzysztof
On Wed, Jun 29, 2016 at 10:16:10AM +0200, Krzysztof Kozlowski wrote:
>
> Seems to work fine except:
> 1. The updates are always 1.
Yes the test function only does digest so it's always one update.
> 2. For bigger blocks it reports always 1 or 3 cycles per byte:
Yes the average cycles per-byte should reach an asymptotic value.
Thanks,
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
On 06/29/2016 10:19 AM, Herbert Xu wrote:
> On Wed, Jun 29, 2016 at 10:16:10AM +0200, Krzysztof Kozlowski wrote:
>>
>> Seems to work fine except:
>> 1. The updates are always 1.
>
> Yes the test function only does digest so it's always one update.
>
>> 2. For bigger blocks it reports always 1 or 3 cycles per byte:
>
> Yes the average cycles per-byte should reach an asymptotic value.
Then:
Tested-by: Krzysztof Kozlowski <[email protected]>
Best regards,
Krzysztof
I tested the latest cryptodev tree on my haswell machine and this is
what I see:
[ 40.402834] modprobe tcrypt mode=422
[ 40.403105] testing speed of multibuffer sha1 (sha1_mb)
[ 40.403108] test 0 ( 16 byte blocks, 16 bytes per update, 1
updates): 32271 cycles/operation, 252 cycles/byte
[ 40.403118] At least one hashing failed ret=-115
[ 43.218712] modprobe tcrypt mode=423
[ 43.218712] testing speed of multibuffer sha256 (sha256_mb)
[ 43.218715] test 0 ( 16 byte blocks, 16 bytes per update, 1
updates): 106965 cycles/operation, 835 cycles/byte
[ 43.218747] At least one hashing failed ret=-115
[ 45.346657] modprobe tcrypt mode=424
[ 45.346657] testing speed of multibuffer sha512 (sha512_mb)
[ 45.346660] test 0 ( 16 byte blocks, 16 bytes per update, 1
updates): 43179 cycles/operation, 337 cycles/byte
[ 45.346673] At least one hashing failed ret=-115
Don't think this is expected, is it?
This is the patch which might have an issue?
72259deb3a9f2c07d18d71d7c9356754e7d88369
Thanks,
Megha
On Wed, 2016-06-29 at 10:28 +0200, Krzysztof Kozlowski wrote:
> On 06/29/2016 10:19 AM, Herbert Xu wrote:
> > On Wed, Jun 29, 2016 at 10:16:10AM +0200, Krzysztof Kozlowski wrote:
> >>
> >> Seems to work fine except:
> >> 1. The updates are always 1.
> >
> > Yes the test function only does digest so it's always one update.
> >
> >> 2. For bigger blocks it reports always 1 or 3 cycles per byte:
> >
> > Yes the average cycles per-byte should reach an asymptotic value.
>
> Then:
> Tested-by: Krzysztof Kozlowski <[email protected]>
>
> Best regards,
> Krzysztof
>
On Wed, Jun 29, 2016 at 10:45:56AM -0700, Megha Dey wrote:
> I tested the latest cryptodev tree on my haswell machine and this is
> what I see:
> [ 40.402834] modprobe tcrypt mode=422
> [ 40.403105] testing speed of multibuffer sha1 (sha1_mb)
> [ 40.403108] test 0 ( 16 byte blocks, 16 bytes per update, 1
> updates): 32271 cycles/operation, 252 cycles/byte
> [ 40.403118] At least one hashing failed ret=-115
> [ 43.218712] modprobe tcrypt mode=423
> [ 43.218712] testing speed of multibuffer sha256 (sha256_mb)
> [ 43.218715] test 0 ( 16 byte blocks, 16 bytes per update, 1
> updates): 106965 cycles/operation, 835 cycles/byte
> [ 43.218747] At least one hashing failed ret=-115
> [ 45.346657] modprobe tcrypt mode=424
> [ 45.346657] testing speed of multibuffer sha512 (sha512_mb)
> [ 45.346660] test 0 ( 16 byte blocks, 16 bytes per update, 1
> updates): 43179 cycles/operation, 337 cycles/byte
> [ 45.346673] At least one hashing failed ret=-115
>
> Don't think this is expected, is it?
No that's not right. Does this patch fix it?
Thanks!
---8<---
The multibuffer hash speed test is incorrectly bailing because
of an EINPROGRESS return value. This patch fixes it by setting
ret to zero if it is equal to -EINPROGRESS.
Reported-by: Megha Dey <[email protected]>
Signed-off-by: Herbert Xu <[email protected]>
diff --git a/crypto/tcrypt.c b/crypto/tcrypt.c
index 8a91dc3..202cfa1 100644
--- a/crypto/tcrypt.c
+++ b/crypto/tcrypt.c
@@ -655,8 +486,10 @@ static void test_mb_ahash_speed(const char *algo, unsigned int sec,
for (k = 0; k < 8; k++) {
ret = crypto_ahash_digest(data[k].req);
- if (ret == -EINPROGRESS)
+ if (ret == -EINPROGRESS) {
+ ret = 0;
continue;
+ }
if (ret)
break;
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
On Thu, 2016-06-30 at 11:00 +0800, Herbert Xu wrote:
> On Wed, Jun 29, 2016 at 10:45:56AM -0700, Megha Dey wrote:
> > I tested the latest cryptodev tree on my haswell machine and this is
> > what I see:
> > [ 40.402834] modprobe tcrypt mode=422
> > [ 40.403105] testing speed of multibuffer sha1 (sha1_mb)
> > [ 40.403108] test 0 ( 16 byte blocks, 16 bytes per update, 1
> > updates): 32271 cycles/operation, 252 cycles/byte
> > [ 40.403118] At least one hashing failed ret=-115
> > [ 43.218712] modprobe tcrypt mode=423
> > [ 43.218712] testing speed of multibuffer sha256 (sha256_mb)
> > [ 43.218715] test 0 ( 16 byte blocks, 16 bytes per update, 1
> > updates): 106965 cycles/operation, 835 cycles/byte
> > [ 43.218747] At least one hashing failed ret=-115
> > [ 45.346657] modprobe tcrypt mode=424
> > [ 45.346657] testing speed of multibuffer sha512 (sha512_mb)
> > [ 45.346660] test 0 ( 16 byte blocks, 16 bytes per update, 1
> > updates): 43179 cycles/operation, 337 cycles/byte
> > [ 45.346673] At least one hashing failed ret=-115
> >
> > Don't think this is expected, is it?
>
> No that's not right. Does this patch fix it?
>
yes, it fixes it. This is my output:
testing speed of multibuffer sha1 (sha1_mb)
[ 31.342538] test 0 ( 16 byte blocks, 16 bytes per update, 1
updates):
31614 cycles/operation, 246 cycles/byte
[ 31.342549] test 2 ( 64 byte blocks, 64 bytes per update, 1
updates):
10893 cycles/operation, 21 cycles/byte
[ 31.342553] test 5 ( 256 byte blocks, 256 bytes per update, 1
updates):
11043 cycles/operation, 5 cycles/byte
[ 31.342557] test 8 ( 1024 byte blocks, 1024 bytes per update, 1
updates):
17355 cycles/operation, 2 cycles/byte
[ 31.342562] test 12 ( 2048 byte blocks, 2048 bytes per update, 1
updates):
26328 cycles/operation, 1 cycles/byte
[ 31.342570] test 16 ( 4096 byte blocks, 4096 bytes per update, 1
updates):
43833 cycles/operation, 1 cycles/byte
[ 31.342584] test 21 ( 8192 byte blocks, 8192 bytes per update, 1
updates):
81141 cycles/operation, 1 cycles/byte
[ 44.700088]
[ 44.700088] testing speed of multibuffer sha256 (sha256_mb)
[ 44.700091] test 0 ( 16 byte blocks, 16 bytes per update, 1
updates):
116874 cycles/operation, 913 cycles/byte
[ 44.700126] test 2 ( 64 byte blocks, 64 bytes per update, 1
updates):
60894 cycles/operation, 118 cycles/byte
[ 44.700144] test 5 ( 256 byte blocks, 256 bytes per update, 1
updates):
15366 cycles/operation, 7 cycles/byte
[ 44.700149] test 8 ( 1024 byte blocks, 1024 bytes per update, 1
updates):
29130 cycles/operation, 3 cycles/byte
[ 44.700158] test 12 ( 2048 byte blocks, 2048 bytes per update, 1
updates):
48561 cycles/operation, 2 cycles/byte
[ 44.700173] test 16 ( 4096 byte blocks, 4096 bytes per update, 1
updates):
119697 cycles/operation, 3 cycles/byte
[ 44.700207] test 21 ( 8192 byte blocks, 8192 bytes per update, 1
updates):
164163 cycles/operation, 2 cycles/byte
[ 66.309608]
[ 66.309608] testing speed of multibuffer sha512 (sha512_mb)
[ 66.309623] test 0 ( 16 byte blocks, 16 bytes per update, 1
updates):
301050 cycles/operation, 2351 cycles/byte
[ 66.309717] test 2 ( 64 byte blocks, 64 bytes per update, 1
updates):
115416 cycles/operation, 225 cycles/byte
[ 66.309757] test 5 ( 256 byte blocks, 256 bytes per update, 1
updates):
175068 cycles/operation, 85 cycles/byte
[ 66.309814] test 8 ( 1024 byte blocks, 1024 bytes per update, 1
updates):
354834 cycles/operation, 43 cycles/byte
[ 66.309920] test 12 ( 2048 byte blocks, 2048 bytes per update, 1
updates):
599814 cycles/operation, 36 cycles/byte
[ 66.310096] test 16 ( 4096 byte blocks, 4096 bytes per update, 1
updates):
585666 cycles/operation, 17 cycles/byte
[ 66.310263] test 21 ( 8192 byte blocks, 8192 bytes per update, 1
updates):
1057770 cycles/operation, 16 cycles/byte
> Thanks!
>
> ---8<---
> The multibuffer hash speed test is incorrectly bailing because
> of an EINPROGRESS return value. This patch fixes it by setting
> ret to zero if it is equal to -EINPROGRESS.
>
> Reported-by: Megha Dey <[email protected]>
> Signed-off-by: Herbert Xu <[email protected]>
>
> diff --git a/crypto/tcrypt.c b/crypto/tcrypt.c
> index 8a91dc3..202cfa1 100644
> --- a/crypto/tcrypt.c
> +++ b/crypto/tcrypt.c
> @@ -655,8 +486,10 @@ static void test_mb_ahash_speed(const char *algo, unsigned int sec,
>
> for (k = 0; k < 8; k++) {
> ret = crypto_ahash_digest(data[k].req);
> - if (ret == -EINPROGRESS)
> + if (ret == -EINPROGRESS) {
> + ret = 0;
> continue;
> + }
>
> if (ret)
> break;