Re: [PATCH V3 6/6] crypto/nx: Add P9 NX support for 842 compression engine

[Haren Myneni <haren@linux.vnet.ibm.com>]

On 08/31/2017 06:40 AM, Dan Streetman wrote:
> On Thu, Aug 31, 2017 at 3:44 AM, Haren Myneni <haren@linux.vnet.ibm.com> wrote:
>> Thanks MIchael and Dan for your review comments.
>>
>>
>> On 08/29/2017 06:32 AM, Dan Streetman wrote:
>>> On Mon, Aug 28, 2017 at 7:25 PM, Michael Ellerman <mpe@ellerman.id.au> wrote:
>>>> Hi Haren,
>>>>
>>>> Some comments inline ...
>>>>
>>>> Haren Myneni <haren@linux.vnet.ibm.com> writes:
>>>>
>>>>> diff --git a/drivers/crypto/nx/nx-842-powernv.c b/drivers/crypto/nx/nx-842-powernv.c
>>>>> index c0dd4c7e17d3..13089a0b9dfa 100644
>>>>> --- a/drivers/crypto/nx/nx-842-powernv.c
>>>>> +++ b/drivers/crypto/nx/nx-842-powernv.c
>>>>> @@ -32,6 +33,9 @@ MODULE_ALIAS_CRYPTO("842-nx");
>>>>>
>>>>>  #define WORKMEM_ALIGN        (CRB_ALIGN)
>>>>>  #define CSB_WAIT_MAX (5000) /* ms */
>>>>> +#define VAS_RETRIES  (10)
>>>>
>>>> Where does that number come from?
>>
>> Sometimes HW returns copy/paste failures.
> 
> why?  what is causing the failure?

Vas can return RMA_busy for several reasons - receive / send windows does not have credits or cached and etc.
 
> 
>> So we should retry the request again. With 10 retries, Test running
>> 12 hours was successful for repeated compression/decompression
>> requests with 1024 threads.
>>
>>>>
>>>> Do we have any idea what the trade off is between retrying vs just
>>>> falling back to doing the request in software?
>>
>> Not checked the overhead with falling back to SW compression.
> 
> SW is very, very, very slow, due to 842 being an unaligned compression format.
> 
>>
>>>>
>>>>> +/* # of requests allowed per RxFIFO at a time. 0 for unlimited */
>>>>> +#define MAX_CREDITS_PER_RXFIFO       (1024)
>>>>>
>>>>>  struct nx842_workmem {
>>>>>       /* Below fields must be properly aligned */
>>>>> @@ -42,16 +46,27 @@ struct nx842_workmem {
>>>>>
>>>>>       ktime_t start;
>>>>>
>>>>> +     struct vas_window *txwin;       /* Used with VAS function */
>>>>
>>>> I don't understand how it makes sense to put txwin and start between the
>>>> fields above, and the padding.
>>>
>>> workmem is a scratch buffer and shouldn't be used for something
>>> persistent like this.
>>>
>>>>
>>>> If the workmem pointer you receive is not aligned, then PTR_ALIGN() will
>>>> advance it and mean you end up writing over start and txwin.
>>
>> We always access workmem with PTR_ALIGN even when assigning txwin (nx842_powernv_crypto_init/exit_vas).
>> So we should not overwrite start and txwin,
>>
>> We can add txwin in nx842_crypto_ctx instead of workmem. But nx842_crypto_ctx is used for both powernv and pseries. Hence used workmem. But if nx842_crypto_ctx is preferred, I will send new patch soon.
>>
>>>>
>>>> That's probably not your bug, the code is already like that.
>>>
>>> no, it's a bug in this patch, because workmem is scratch whose
>>> contents are only valid for the duration of each operation (compress
>>> or decompress).
>>>
>>>>
>>>>>       char padding[WORKMEM_ALIGN]; /* unused, to allow alignment */
>>>>>  } __packed __aligned(WORKMEM_ALIGN);
>>>>
>>
>> Thanks
>> Haren
>>
>