From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id AFFCFC6FA82 for ; Thu, 8 Sep 2022 23:19:47 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229729AbiIHXTr (ORCPT ); Thu, 8 Sep 2022 19:19:47 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:57898 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229551AbiIHXTq (ORCPT ); Thu, 8 Sep 2022 19:19:46 -0400 Received: from dfw.source.kernel.org (dfw.source.kernel.org [IPv6:2604:1380:4641:c500::1]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 8255D22298; Thu, 8 Sep 2022 16:19:44 -0700 (PDT) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id 0DA9E61E2C; Thu, 8 Sep 2022 23:19:44 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id D1F44C433C1; Thu, 8 Sep 2022 23:19:42 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1662679183; bh=iOyyWbAaz1hh5SNuHyuGAi6HyRZtoAMjl33qrdVB8kY=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=BCev5ieZ7COM056f56yk/NxR7hauUKJYz4b3Ci3rbhk/jRE8V6Jw8pC4lYio8iVc0 3FI41qWJQIcojQ6rAiYdFrNf/515IGYeTLD8D7kASVLTSQz+kE7aLvKcSSPzabUXZo PkVolO1I+hOsAyNo+BrHTZcemvWQapeQrISaVnwGZMMdAUufxyf/6pkx7oI6jgHhXo maZwzIaVUETjmvjPt1ks84Ax0QuA4WNl9O/SNHR2VZmQCK/seM31upXfHw7F5AUMpi QajG2XVQsqFkLceKgLrSwwyKo0Zc0DiHIPfUderNTU9oX9fNHaSJ+aIpkXsPyY7OhJ jnPgTzfq0OqeA== Date: Fri, 9 Sep 2022 02:19:36 +0300 From: Jarkko Sakkinen To: Reinette Chatre Cc: linux-sgx@vger.kernel.org, Haitao Huang , Vijay Dhanraj , Dave Hansen , Shuah Khan , "open list:KERNEL SELFTEST FRAMEWORK" , open list Subject: Re: [PATCH v2 1/5] selftests/sgx: Retry the ioctl()'s returned with EAGAIN Message-ID: References: <20220905020411.17290-1-jarkko@kernel.org> <20220905020411.17290-2-jarkko@kernel.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Precedence: bulk List-ID: X-Mailing-List: linux-sgx@vger.kernel.org On Thu, Sep 08, 2022 at 03:43:06PM -0700, Reinette Chatre wrote: > Hi Jarkko and Haitao, > > On 9/4/2022 7:04 PM, Jarkko Sakkinen wrote: > > From: Haitao Huang > > > > For EMODT and EREMOVE ioctl()'s with a large range, kernel > > may not finish in one shot and return EAGAIN error code > > and count of bytes of EPC pages on that operations are > > finished successfully. > > > > Change the unclobbered_vdso_oversubscribed_remove test > > to rerun the ioctl()'s in a loop, updating offset and length > > using the byte count returned in each iteration. > > > > Fixes: 6507cce561b4 ("selftests/sgx: Page removal stress test") > > Should this patch be moved to the "critical fixes for v6.0" series? I think not because it does not risk stability of the kernel itself. It's "nice to have" but not mandatory. > > > Signed-off-by: Haitao Huang > > Tested-by: Jarkko Sakkinen > > Signed-off-by: Jarkko Sakkinen > > --- > > v3: > > * Added a fixes tag. The bug is in v6.0 patches. > > * Added my tested-by (the bug reproduced in my NUC often). > > v2: > > * Changed branching in EAGAIN condition so that else branch > > is not required. > > * Addressed Reinette's feedback: > > --- > > tools/testing/selftests/sgx/main.c | 42 ++++++++++++++++++++++++------ > > 1 file changed, 34 insertions(+), 8 deletions(-) > > > > diff --git a/tools/testing/selftests/sgx/main.c b/tools/testing/selftests/sgx/main.c > > index 9820b3809c69..59cca806eda1 100644 > > --- a/tools/testing/selftests/sgx/main.c > > +++ b/tools/testing/selftests/sgx/main.c > > @@ -390,6 +390,7 @@ TEST_F_TIMEOUT(enclave, unclobbered_vdso_oversubscribed_remove, 900) > > struct encl_segment *heap; > > unsigned long total_mem; > > int ret, errno_save; > > + unsigned long count; > > unsigned long addr; > > unsigned long i; > > > > @@ -453,16 +454,30 @@ TEST_F_TIMEOUT(enclave, unclobbered_vdso_oversubscribed_remove, 900) > > modt_ioc.offset = heap->offset; > > modt_ioc.length = heap->size; > > modt_ioc.page_type = SGX_PAGE_TYPE_TRIM; > > - > > + count = 0; > > TH_LOG("Changing type of %zd bytes to trimmed may take a while ...", > > heap->size); > > - ret = ioctl(self->encl.fd, SGX_IOC_ENCLAVE_MODIFY_TYPES, &modt_ioc); > > - errno_save = ret == -1 ? errno : 0; > > + do { > > + ret = ioctl(self->encl.fd, SGX_IOC_ENCLAVE_MODIFY_TYPES, &modt_ioc); > > + > > + errno_save = ret == -1 ? errno : 0; > > + if (errno_save != EAGAIN) > > + break; > > + > > + EXPECT_EQ(modt_ioc.result, 0); > > If this check triggers then there is something seriously wrong and in that case > it may also be that this loop may be unable to terminate or the error condition would > keep appearing until the loop terminates (which may be many iterations). Considering > the severity and risk I do think that ASSERT_EQ() would be more appropriate, > similar to how ASSERT_EQ() is used in patch 5/5. > > Apart from that I think that this looks good. > > Thank you very much for adding this. > > Reinette Hmm... I could along the lines: /* * Get time since Epoch is milliseconds. */ unsigned long get_time(void) { struct timeval start; gettimeofday(&start, NULL); return (unsigneg long)start.tv_sec * 1000L + (unsigned long)start.tv_usec / 1000L; } and #define IOCTL_RETRY_TIMEOUT 100 In the test function: unsigned long start_time; /* ... */ start_time = get_time(); do { EXPECT_LT(get_time() - start_time(), IOCTL_RETRY_TIMEOUT); /* ... */ } /* ... */ What do you think? BR, Jarkko