Hi Dave,
Appreciate for your review!
> On Apr 27, 2024, at 01:06, Dave Hansen <dave.hansen@xxxxxxxxx> wrote:
>
> On 4/26/24 07:18, Bojun Zhu wrote:
>> for (c = 0 ; c < modp->length; c += PAGE_SIZE) {
>> + if (sgx_check_signal_and_resched()) {
>> + if (!c)
>> + ret = -ERESTARTSYS;
>> +
>> + goto out;
>> + }
>
> This construct is rather fugly. Let's not perpetuate it, please. Why
> not do:
>
> int ret = -ERESTARTSYS;
>
> ...
> for (c = 0 ; c < modp->length; c += PAGE_SIZE) {
> if (sgx_check_signal_and_resched())
> goto out;
>
> Then, voila, when c==0 on the first run through the loop, you'll get a
> ret=-ERESTARTSYS.
>
Okay, I will refine it later.
> But honestly, it seems kinda silly to annotate all these loops with
> explicit cond_resched()s. I'd much rather do this once and, for
> instance, just wrap the enclave locks:
>
> - mutex_lock(&encl->lock);
> + sgx_lock_enclave(encl);
>
> and then have the lock function do the rescheds. I assume that
> mutex_lock() isn't doing this generically for performance reasons. But
> we don't care in SGX land and can just resched to our heart's content.
`mutex_lock(&encl->lock)` appears in everywhere in SGX in-tree driver.
But it seems that we only need to additionally invoke `cond_resched()` for
the sgx_enclave_{restrict_permissions | modify_types | remove_pages }
and sgx_ioc_add_pages()’s ioctl()s.
Shall we replace all the `mutex_lock(&encl->lock) with `sgx_lock_enclave(encl)`
in SGX in-tree driver and then wrap reschedule operation in
`sgx_lock_enclave()` ?
Regards,
Bojun
