Re: dm: submit stacked requests in irq enabled context

Re: dm: submit stacked requests in irq enabled context

[Neeraj Soni]

[-- Attachment #1.1: Type: text/plain, Size: 1575 bytes --]

+ Alasdair and dm-devel for awareness and inputs.


On 5/9/2017 12:26 PM, Neeraj Soni wrote:
> Hi Keith/Snitzer,
>
> I have recently started using kernel 4.4 on a Android device and ran 
> Androbench to check storage read/write performance. I found that the 
> Random Read (RR) and Random write(RW) performance with Full Disk 
> Encryption is degraded compared to no Disk Encryption. Initially i 
> thought this must the issue with the storage part used and i compared 
> the performance of similar storage part on a device that was using 
> Android with kernel 3.18. I found that with no Disk Encryption the 
> performance was equivalent to the device which was using 4.4 but with 
> Disk Encryption there was degradation in RR (~20%) and RW(10%).
>
> I then tried to compare the changes that was brought in kernel 4.4 in 
> Full Disk Encryption path. I came across the patch mentioned in 
> subject and found that now a worker thread is scheduled in 
> dm_request_fn() to process the requests instead of directly invoking 
> map_request() as was in kernel 3.18.
>
> I reverted this patch and found that the RR and RW performance was now 
> closer to what we have without Disk Encryption. From the commit 
> message i understand that this change is significant and will be 
> required for blk-mq support but have you came across such degradation 
> issue with your patch and do we have any fix for this degradation 
> available?
>
> BR,
> Neeraj Soni,
> --

Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
     a Linux Foundation Collaborative Project




[-- Attachment #1.2: Type: text/html, Size: 3489 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: dm: submit stacked requests in irq enabled context

[Keith Busch]

On Tue, May 09, 2017 at 03:19:31PM +0530, Neeraj Soni wrote:
>    + Alasdair and dm-devel for awareness and inputs.
> 
>    On 5/9/2017 12:26 PM, Neeraj Soni wrote:
> 
>      Hi Keith/Snitzer,
> 
>      I have recently started using kernel 4.4 on a Android device and ran
>      Androbench to check storage read/write performance. I found that the
>      Random Read (RR) and Random write(RW) performance with Full Disk
>      Encryption is degraded compared to no Disk Encryption. Initially i
>      thought this must the issue with the storage part used and i compared
>      the performance of similar storage part on a device that was using
>      Android with kernel 3.18. I found that with no Disk Encryption the
>      performance was equivalent to the device which was using 4.4 but with
>      Disk Encryption there was degradation in RR (~20%) and RW(10%).
> 
>      I then tried to compare the changes that was brought in kernel 4.4 in
>      Full Disk Encryption path. I came across the patch mentioned in subject
>      and found that now a worker thread is scheduled in dm_request_fn() to
>      process the requests instead of directly invoking map_request() as was
>      in kernel 3.18.
> 
>      I reverted this patch and found that the RR and RW performance was now
>      closer to what we have without Disk Encryption. From the commit message
>      i understand that this change is significant and will be required for
>      blk-mq support but have you came across such degradation issue with your
>      patch and do we have any fix for this degradation available?

Just for comparison, could you check performance on a more recent stable
kernel?

Re: dm: submit stacked requests in irq enabled context

[Neeraj Soni]

[-- Attachment #1.1: Type: text/plain, Size: 2242 bytes --]

As of now i can not move to the latest stable version (4.11 is what i 
see in https://www.kernel.org/) since we do not have support available 
for that.

I assume that this patch will be present even in 4.11 so effectively if 
no remedy is brought in in 4.11 the problem will still exist since we 
are dealing with an additional thread and scheduling delays.

Neeraj

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

On 5/9/2017 9:25 PM, Keith Busch wrote:
> On Tue, May 09, 2017 at 03:19:31PM +0530, Neeraj Soni wrote:
>>     + Alasdair and dm-devel for awareness and inputs.
>>
>>     On 5/9/2017 12:26 PM, Neeraj Soni wrote:
>>
>>       Hi Keith/Snitzer,
>>
>>       I have recently started using kernel 4.4 on a Android device and ran
>>       Androbench to check storage read/write performance. I found that the
>>       Random Read (RR) and Random write(RW) performance with Full Disk
>>       Encryption is degraded compared to no Disk Encryption. Initially i
>>       thought this must the issue with the storage part used and i compared
>>       the performance of similar storage part on a device that was using
>>       Android with kernel 3.18. I found that with no Disk Encryption the
>>       performance was equivalent to the device which was using 4.4 but with
>>       Disk Encryption there was degradation in RR (~20%) and RW(10%).
>>
>>       I then tried to compare the changes that was brought in kernel 4.4 in
>>       Full Disk Encryption path. I came across the patch mentioned in subject
>>       and found that now a worker thread is scheduled in dm_request_fn() to
>>       process the requests instead of directly invoking map_request() as was
>>       in kernel 3.18.
>>
>>       I reverted this patch and found that the RR and RW performance was now
>>       closer to what we have without Disk Encryption. From the commit message
>>       i understand that this change is significant and will be required for
>>       blk-mq support but have you came across such degradation issue with your
>>       patch and do we have any fix for this degradation available?
> Just for comparison, could you check performance on a more recent stable
> kernel?


[-- Attachment #1.2: Type: text/html, Size: 38100 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: dm: submit stacked requests in irq enabled context

[Christoph Hellwig]

On Tue, May 09, 2017 at 11:55:55AM -0400, Keith Busch wrote:
> >      I have recently started using kernel 4.4 on a Android device and ran
> >      Androbench to check storage read/write performance. I found that the
> >      Random Read (RR) and Random write(RW) performance with Full Disk
> >      Encryption is degraded compared to no Disk Encryption. Initially i

dm-crypt doesn't even use request based device mapper.  Something odd
must be going on with your benchmarks.

Re: dm: submit stacked requests in irq enabled context

[Neeraj Soni]

[-- Attachment #1.1: Type: text/plain, Size: 916 bytes --]

Hi Keith,

Request based dm (dm-req-crypt) is being used for Disk Encryption 
solution in Android used by Google. Also as i mentioned reverting this 
fix  improves the RR/RW numbers so this proves the request based dm is 
coming into path and is being used.

Neeraj

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project


On 5/10/2017 12:47 PM, Christoph Hellwig wrote:
> On Tue, May 09, 2017 at 11:55:55AM -0400, Keith Busch wrote:
>>>       I have recently started using kernel 4.4 on a Android device and ran
>>>       Androbench to check storage read/write performance. I found that the
>>>       Random Read (RR) and Random write(RW) performance with Full Disk
>>>       Encryption is degraded compared to no Disk Encryption. Initially i
> dm-crypt doesn't even use request based device mapper.  Something odd
> must be going on with your benchmarks.


[-- Attachment #1.2: Type: text/html, Size: 36868 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: dm: submit stacked requests in irq enabled context

[Gilad Ben-Yossef]

On Wed, May 10, 2017 at 11:49 AM, Neeraj Soni <neersoni@codeaurora.org> wrote:
> Hi Keith,
>
> Request based dm (dm-req-crypt) is being used for Disk Encryption solution
> in Android used by Google. Also as i mentioned reverting this fix  improves
> the RR/RW numbers so this proves the request based dm is coming into path
> and is being used.

Sadly, that is an out of tree module.

Does it still use Qcom specific APIs in its implementation (qcrypto_* funcs)?
It did the last time I've checked - and the driver that implements
those is not upstream either...

It makes it difficult to help - which is a shame since I am interested
in enabling higher performance
of dm-crypt when using HW based crypto transformation myself.

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

need to pick a solution for dm-crypt IV generation and do it! [was: Re: dm: submit stacked requests in irq enabled context]

[Mike Snitzer]

On Wed, May 10 2017 at  9:37am -0400,
Gilad Ben-Yossef <gilad@benyossef.com> wrote:

> On Wed, May 10, 2017 at 11:49 AM, Neeraj Soni <neersoni@codeaurora.org> wrote:
> > Hi Keith,
> >
> > Request based dm (dm-req-crypt) is being used for Disk Encryption solution
> > in Android used by Google. Also as i mentioned reverting this fix  improves
> > the RR/RW numbers so this proves the request based dm is coming into path
> > and is being used.
> 
> Sadly, that is an out of tree module.
> 
> Does it still use Qcom specific APIs in its implementation (qcrypto_* funcs)?
> It did the last time I've checked - and the driver that implements
> those is not upstream either...
> 
> It makes it difficult to help - which is a shame since I am interested
> in enabling higher performance
> of dm-crypt when using HW based crypto transformation myself.

I have absolutely no interest in request-based dm-crypt.  It is a hack
to work-around limitations in crypto IV generation.

If "google" is foolish enough to deploy out-of-tree request-based
dm-crypt in their android kernel then they are easily capable of
reverting the commit in question to prop up their short-cited decision.

The correct way forward is to follow through with the crypto work
discussed here (pick a solution to implement and make it happen):
https://www.redhat.com/archives/dm-devel/2017-March/msg00044.html
and here:
https://www.redhat.com/archives/dm-devel/2017-March/msg00053.html
and here:
https://www.redhat.com/archives/dm-devel/2017-April/msg00132.html

Mike

Re: need to pick a solution for dm-crypt IV generation and do it! [was: Re: dm: submit stacked requests in irq enabled context]

[Gilad Ben-Yossef]

On Wed, May 10, 2017 at 5:45 PM, Mike Snitzer <snitzer@redhat.com> wrote:
> On Wed, May 10 2017 at  9:37am -0400,
> Gilad Ben-Yossef <gilad@benyossef.com> wrote:
>
>> On Wed, May 10, 2017 at 11:49 AM, Neeraj Soni <neersoni@codeaurora.org> wrote:
>> > Hi Keith,
>> >
>> > Request based dm (dm-req-crypt) is being used for Disk Encryption solution
>> > in Android used by Google. Also as i mentioned reverting this fix  improves
>> > the RR/RW numbers so this proves the request based dm is coming into path
>> > and is being used.
>>
>> Sadly, that is an out of tree module.
>>
>> Does it still use Qcom specific APIs in its implementation (qcrypto_* funcs)?
>> It did the last time I've checked - and the driver that implements
>> those is not upstream either...
>>
>> It makes it difficult to help - which is a shame since I am interested
>> in enabling higher performance
>> of dm-crypt when using HW based crypto transformation myself.
>
> I have absolutely no interest in request-based dm-crypt.  It is a hack
> to work-around limitations in crypto IV generation.

I agree. This is why I've said I'm interested in a high performance dm-crypt,
not "request based dm-crypt". They are trying to solve the same problem but
with the wrong solution and doing so out of upstream.

As the parlance of our time seems to go... sad. :-)


Cheers,
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

Re: need to pick a solution for dm-crypt IV generation and do it! [was: Re: dm: submit stacked requests in irq enabled context]

[Neeraj Soni]

Thanks for inputs folks. So shall i conclude that there is no remedy 
available that can be applied on 4.4 and reverting this patch is only 
way forward to solve the degradation?

Neeraj


On 5/10/2017 8:25 PM, Gilad Ben-Yossef wrote:
> On Wed, May 10, 2017 at 5:45 PM, Mike Snitzer <snitzer@redhat.com> wrote:
>> On Wed, May 10 2017 at  9:37am -0400,
>> Gilad Ben-Yossef <gilad@benyossef.com> wrote:
>>
>>> On Wed, May 10, 2017 at 11:49 AM, Neeraj Soni <neersoni@codeaurora.org> wrote:
>>>> Hi Keith,
>>>>
>>>> Request based dm (dm-req-crypt) is being used for Disk Encryption solution
>>>> in Android used by Google. Also as i mentioned reverting this fix  improves
>>>> the RR/RW numbers so this proves the request based dm is coming into path
>>>> and is being used.
>>> Sadly, that is an out of tree module.
>>>
>>> Does it still use Qcom specific APIs in its implementation (qcrypto_* funcs)?
>>> It did the last time I've checked - and the driver that implements
>>> those is not upstream either...
>>>
>>> It makes it difficult to help - which is a shame since I am interested
>>> in enabling higher performance
>>> of dm-crypt when using HW based crypto transformation myself.
>> I have absolutely no interest in request-based dm-crypt.  It is a hack
>> to work-around limitations in crypto IV generation.
> I agree. This is why I've said I'm interested in a high performance dm-crypt,
> not "request based dm-crypt". They are trying to solve the same problem but
> with the wrong solution and doing so out of upstream.
>
> As the parlance of our time seems to go... sad. :-)
>
>
> Cheers,
> Gilad
>
>

Re: need to pick a solution for dm-crypt IV generation and do it! [was: Re: dm: submit stacked requests in irq enabled context]

[Neeraj Soni]

Until we move to some latest stable kernel as Keith mentioned.

On 5/11/2017 11:22 AM, Neeraj Soni wrote:
> Thanks for inputs folks. So shall i conclude that there is no remedy 
> available that can be applied on 4.4 and reverting this patch is only 
> way forward to solve the degradation?
>
> Neeraj
>
>
> On 5/10/2017 8:25 PM, Gilad Ben-Yossef wrote:
>> On Wed, May 10, 2017 at 5:45 PM, Mike Snitzer <snitzer@redhat.com> 
>> wrote:
>>> On Wed, May 10 2017 at  9:37am -0400,
>>> Gilad Ben-Yossef <gilad@benyossef.com> wrote:
>>>
>>>> On Wed, May 10, 2017 at 11:49 AM, Neeraj Soni 
>>>> <neersoni@codeaurora.org> wrote:
>>>>> Hi Keith,
>>>>>
>>>>> Request based dm (dm-req-crypt) is being used for Disk Encryption 
>>>>> solution
>>>>> in Android used by Google. Also as i mentioned reverting this fix  
>>>>> improves
>>>>> the RR/RW numbers so this proves the request based dm is coming 
>>>>> into path
>>>>> and is being used.
>>>> Sadly, that is an out of tree module.
>>>>
>>>> Does it still use Qcom specific APIs in its implementation 
>>>> (qcrypto_* funcs)?
>>>> It did the last time I've checked - and the driver that implements
>>>> those is not upstream either...
>>>>
>>>> It makes it difficult to help - which is a shame since I am interested
>>>> in enabling higher performance
>>>> of dm-crypt when using HW based crypto transformation myself.
>>> I have absolutely no interest in request-based dm-crypt.  It is a hack
>>> to work-around limitations in crypto IV generation.
>> I agree. This is why I've said I'm interested in a high performance 
>> dm-crypt,
>> not "request based dm-crypt". They are trying to solve the same 
>> problem but
>> with the wrong solution and doing so out of upstream.
>>
>> As the parlance of our time seems to go... sad. :-)
>>
>>
>> Cheers,
>> Gilad
>>
>>
>