lkml.org 
[lkml]   [2018]   [Mar]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRE: [dm-devel] [PATCH 2/2] md: dm-verity: allow parallel processing of bio blocks
Date
Hi Milan,
I will run veritysetup test on next version of these patches and contact you about verity-compat-test testsuits.
Thank you,
Yael

-----Original Message-----
From: Milan Broz <gmazyland@gmail.com>
Sent: Tuesday, 27 March 2018 11:05
To: Eric Biggers <ebiggers3@gmail.com>; Yael Chemla <yael.chemla@foss.arm.com>; Mike Snitzer <snitzer@redhat.com>
Cc: Alasdair Kergon <agk@redhat.com>; dm-devel@redhat.com; linux-kernel@vger.kernel.org; ofir.drang@gmail.com; Yael Chemla <yael.chemla@arm.com>; linux-crypto@vger.kernel.org; gilad@benyossef.com
Subject: Re: [dm-devel] [PATCH 2/2] md: dm-verity: allow parallel processing of bio blocks

Mike and others,

did anyone even try to run veritysetup tests?

We have verity-compat-test in our testsuite, is has even basic FEC tests included.

We just added userspace verification of FEC RS codes to compare if kernel behaves the same.

I tried to apply three last dm-verity patches from your tree to Linus mainline.

It does even pass the *first* line of the test script and blocks the kernel forever...
(Running on 32bit Intel VM.)

*NACK* to the last two dm-verity patches.

(The "validate hashes once" is ok, despite I really do not like this approach...)

And comments from Eric are very valid as well, I think all this need to be fixed before it can go to mainline.

Thanks,
Milan

On 03/27/2018 08:55 AM, Eric Biggers wrote:
> [+Cc linux-crypto]
>
> Hi Yael,
>
> On Sun, Mar 25, 2018 at 07:41:30PM +0100, Yael Chemla wrote:
>> Allow parallel processing of bio blocks by moving to async.
>> completion handling. This allows for better resource utilization of
>> both HW and software based hash tfm and therefore better performance
>> in many cases, depending on the specific tfm in use.
>>
>> Tested on ARM32 (zynq board) and ARM64 (Juno board).
>> Time of cat command was measured on a filesystem with various file sizes.
>> 12% performance improvement when HW based hash was used (ccree driver).
>> SW based hash showed less than 1% improvement.
>> CPU utilization when HW based hash was used presented 10% less
>> context switch, 4% less cycles and 7% less instructions. No
>> difference in CPU utilization noticed with SW based hash.
>>
>> Signed-off-by: Yael Chemla <yael.chemla@foss.arm.com>
>
> Okay, I definitely would like to see dm-verity better support hardware
> crypto accelerators, but these patches were painful to read.
>
> There are lots of smaller bugs, but the high-level problem which you
> need to address first is that on every bio you are always allocating
> all the extra memory to hold a hash request and scatterlist for every
> data block. This will not only hurt performance when the hashing is
> done in software (I'm skeptical that your performance numbers are
> representative of that case), but it will also fall apart under memory
> pressure. We are trying to get low-end Android devices to start using
> dm-verity, and such devices often have only 1 GB or even only 512 MB
> of RAM, so memory allocations are at increased risk of failing. In
> fact I'm pretty sure you didn't do any proper stress testing of these
> patches, since the first thing they do for every bio is try to
> allocate a physically contiguous array that is nearly as long as the
> full bio data itself (n_blocks * sizeof(struct dm_verity_req_data) =
> n_blocks * 3264, at least on a 64-bit platform, mostly due to the 'struct dm_verity_fec_io'), so potentially up to about 1 MB; that's going to fail a lot even on systems with gigabytes of RAM...
>
> (You also need to verify that your new code is compatible with the
> forward error correction feature, with the "ignore_zero_blocks"
> option, and with the new "check_at_most_once" option. From my reading
> of the code, all of those seemed broken; the dm_verity_fec_io
> structures, for example, weren't even being
> initialized...)
>
> I think you need to take a close look at how dm-crypt handles async
> crypto implementations, since it seems to do it properly without
> hurting the common case where the crypto happens synchronously. What
> it does, is it reserves space in the per-bio data for a single cipher
> request. Then, *only* if the cipher implementation actually processes
> the request asynchronously (as indicated by -EINPROGRESS being
> returned) is a new cipher request allocated dynamically, using a
> mempool (not kmalloc, which is prone to fail). Note that unlike your
> patches it also properly handles the case where the hardware crypto
> queue is full, as indicated by the cipher implementation returning -EBUSY; in that case, dm-crypt waits to start another request until there is space in the queue.
>
> I think it would be possible to adapt dm-crypt's solution to dm-verity.
>
> Thanks,
>
> Eric

\
 
 \ /
  Last update: 2018-03-27 11:10    [W:0.070 / U:8.432 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site