Re: [PATCH v3 1/4] s390/crypto: Replace cond_resched() with msleep(1)

[Holger Dengler <dengler@linux.ibm.com>]

On 7/30/26 14:48, Peter Zijlstra wrote:
> On Thu, Jul 30, 2026 at 01:34:31PM +0200, Holger Dengler wrote:
>> Peter,
>>
>> On 7/30/26 12:11, Peter Zijlstra wrote:
>>> On Thu, Jul 30, 2026 at 07:29:04AM +0200, Heiko Carstens wrote:
>>>> With [1] cond_resched() is always compiled away and becomes a no-op.
>>>>
>>>> The comments for all cond_resched() calls in crypto code however indicate
>>>> that the current process should be scheduled away to avoid instant
>>>> re-invocation of a callback. This is not what cond_resched() would do or
>>>> did.
>>>>
>>>> Instead of just removing the cond_resched() calls, replace them with
>>>> msleep() calls, as suggested by Holger Dengler. This forces the current
>>>> task to be scheduled away (sleeps) like originally intended.
>>>>
>>>> [1] commit 7dadeaa6e851 ("sched: Further restrict the preemption modes")
>>>>
>>>> Signed-off-by: Heiko Carstens <hca@linux.ibm.com>
>>>> ---
>>>>  arch/s390/crypto/paes_s390.c  | 8 ++++----
>>>>  arch/s390/crypto/phmac_s390.c | 4 ++--
>>>>  2 files changed, 6 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/arch/s390/crypto/paes_s390.c b/arch/s390/crypto/paes_s390.c
>>>> index 8cfe6166c193..511cb6105436 100644
>>>> --- a/arch/s390/crypto/paes_s390.c
>>>> +++ b/arch/s390/crypto/paes_s390.c
>>>> @@ -555,7 +555,7 @@ static int ecb_paes_do_one_request(struct crypto_engine *engine, void *areq)
>>>>  		 * To avoid immediately re-invocation of this callback,
>>>>  		 * tell the scheduler to voluntarily give up the CPU here.
>>>>  		 */
>>>> -		cond_resched();
>>>> +		msleep(1);
>>>>  		pr_debug("rescheduling request\n");
>>>>  		return -ENOSPC;
>>>>  	} else if (rc) {
>>>
>>> I am somewhat conflicted on this. It will add a 'random' delay to this
>>> crypto user (which might be real-time task) that is not related to the
>>> actual event this is waiting for.
>>>
>>> That is, it could be that this key expiration thing is sorted way faster
>>> than this one milisecond.
>>>
>>> Is there nothing the crypto layer can do that is more clever; like a
>>> condition variable on the key update when -ENOSPC is returned or
>>> something.
>>
>> Let me give a bit of background here: The protected key can only get
>> invalid, if the linux instance (z/VM or KVM guest) is moved to another
>> hypervisor on a different machine (aka life guest relocation). In such a
>> case, the crypto accelerator card and the host has to exchange the "real
>> key", which is wrapped by the host and handed back to the guest as the
>> re-newed protected key. Unfortunately there is no asynchronous trigger
>> on completion, you have to re-try (and maybe get another "in progress"
>> return).
>>
>> And as if that weren't bad enough, if this key exchange between card and
>> host is the first one, card and host has to instanciate a secure
>> communication channel (including a key exchange for the transport layer).
>>
>> I agree, this sounds rally bad for real-time tasks. But we're talking
>> about 2nd-level virtualization (with non-real-time hypervisors below)
>> and about cases, which can only happen right after a guest relocation to
>> another machine. Would the current solution be acceptable under these
>> circumstances?
> 
> Yes, guest migration is very likely far more disruptive than most
> anything else. Perhaps clarify the code comment to include some of this
> explanation?

I agree, the comment is not telling all main key points. What about the
following?

/*
 * Protected key expired due to relocation to another
 * host. The long runnning re-wrap has no asynchronous
 * completion notification, so polling is required.
 * Trigger a re-schedule of this request by returning
 * -ENOSPC ("hardware queue full") to the crypto engine.
 * To avoid immediately re-invocation of this callback,
 * tell scheduler to voluntarily give up the CPU here.
 */

(I would leave it up to Heiko to merge the comment in his series, or let
Harald/me do it in a separate patch)

-- 
Mit freundlichen Grüßen / Kind regards
Holger Dengler