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 kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id BE37CEB64DA for ; Tue, 18 Jul 2023 12:43:01 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 559AC6B0071; Tue, 18 Jul 2023 08:43:01 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 509348D0002; Tue, 18 Jul 2023 08:43:01 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 3D1018D0001; Tue, 18 Jul 2023 08:43:01 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0010.hostedemail.com [216.40.44.10]) by kanga.kvack.org (Postfix) with ESMTP id 2DF2D6B0071 for ; Tue, 18 Jul 2023 08:43:01 -0400 (EDT) Received: from smtpin11.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay06.hostedemail.com (Postfix) with ESMTP id 09519B0AC5 for ; Tue, 18 Jul 2023 12:43:01 +0000 (UTC) X-FDA: 81024697362.11.C35BA7D Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by imf15.hostedemail.com (Postfix) with ESMTP id 0A410A000A for ; Tue, 18 Jul 2023 12:42:58 +0000 (UTC) Authentication-Results: imf15.hostedemail.com; dkim=none; dmarc=pass (policy=none) header.from=arm.com; spf=pass (imf15.hostedemail.com: domain of ryan.roberts@arm.com designates 217.140.110.172 as permitted sender) smtp.mailfrom=ryan.roberts@arm.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1689684179; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=RlqcRrjE8btuw4rNq/9RXYkdo4qXvc3bA+Fk+InywIk=; b=0fZ+cTkFKTqnSB6maRczTf8Y1/bFqHsudV2iGrSrqADZOH6OLItTJ8F71JP3mIln2tXDEo RSx7SUIIv8/13lgbrQXeQlnl6PAiZA8TJZpfWdXsRoD5L++zbBAJ9zwGO8Kq2JXasWOukb Fjo2BX0AWFj8uWRi0f+Y/1YA8a+oQaY= ARC-Authentication-Results: i=1; imf15.hostedemail.com; dkim=none; dmarc=pass (policy=none) header.from=arm.com; spf=pass (imf15.hostedemail.com: domain of ryan.roberts@arm.com designates 217.140.110.172 as permitted sender) smtp.mailfrom=ryan.roberts@arm.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1689684179; a=rsa-sha256; cv=none; b=SNg1rLj3W/lEsziSWNU4HaY/NAcTpabsOESA9JddLPW9VWG9aXJOEZZw7+SXquost+Vw8j bQPrqf4fdMQsvWxpcdmuW/w+nVPwEXXv3BQMvhjx3kKUpzb+JPu0CCnsP2F7KNVh3/6NVr UuElYdEggnnWdsQr2PP2pBicdd4UMX4= Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 4B5412F4; Tue, 18 Jul 2023 05:43:41 -0700 (PDT) Received: from [10.1.34.52] (C02Z41KALVDN.cambridge.arm.com [10.1.34.52]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 4C7613F6C4; Tue, 18 Jul 2023 05:42:56 -0700 (PDT) Message-ID: Date: Tue, 18 Jul 2023 13:42:54 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0) Gecko/20100101 Thunderbird/102.13.0 Subject: Re: [PATCH v2 6/8] selftests/mm: Make migration test robust to failure To: David Hildenbrand , Andrew Morton , Shuah Khan , =?UTF-8?B?SsOpcsO0bWUgR2xpc3Nl?= , Mark Brown , John Hubbard , Florent Revest , Peter Xu Cc: linux-kernel@vger.kernel.org, linux-mm@kvack.org, linux-kselftest@vger.kernel.org References: <20230717103152.202078-1-ryan.roberts@arm.com> <20230717103152.202078-7-ryan.roberts@arm.com> <20501a7c-19f6-4154-aebc-49df04c9b043@redhat.com> <2e5baba4-c8ef-9da4-a2d6-3cf383ed05bd@arm.com> <1314fe0e-dd32-bf10-0a33-2b571dad70bd@redhat.com> <90a8b224-4566-5051-4903-f9ab83b26150@redhat.com> From: Ryan Roberts In-Reply-To: <90a8b224-4566-5051-4903-f9ab83b26150@redhat.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-Rspam-User: X-Rspamd-Server: rspam12 X-Rspamd-Queue-Id: 0A410A000A X-Stat-Signature: uyjx3aemoqqrtk48hfqoke581f8x1ktg X-HE-Tag: 1689684178-554469 X-HE-Meta: U2FsdGVkX18Kmh+F90xUEwOli8xogMM4m+zRaUZvGPYks00hUe9uqn50m4u/sWR9f+z92A7Zb7YgQM/yeFZfAy1Kcvol28QEs1mDIdgo6HJjYhdjqNvgSm/+mlwP7+TPZkGBrX9/MnTT7WhY6paknP2sB3Clj0xoU/lW9GQJ3XQ0QRrofBEPEqUlEkwOQqOA6c0NAT4WFhiV3gL8rvm1T2P6cqney/vPC+ciI/EUVXeTyAfO68ibCwJDfeOIsMd3tZGAYM/j9UjKlQkFH/0zCJhs4wb7eH4EUxqeyoShn+uTtGEQNOII0Z+Cbk+yUma7AMgmFzt4Y1eMAwOU8sahdNMUR9n23LtOi9FbPcAJHhNQXfpvMfHJOPN3zaATziRgvoVA45JuhxkQ1GuHdY60Tf2lbLjG8rut6Qav5xf1uR4hdlNWDBVhngJc+lfUoNGmtKHC6BNj8phTVqudMjLGSK04CzByNEM50etRNvqPdupgTtbYCsZPiZN/ACEiKtzPyeH7AeVoow/UwvGZazi/jgUaUgL0r1RN6xJCJ6YFYl5oYiaCDO//irNMbdmeJZuBwa/OfURA7ifhOC4WI2O+9UkeiNCNvYKArmKKPWIOYNrQ0KH2+AIJxu/sdDq+E3L4DO5rb3TehxxKvillphVHi3hezpeaxtdl1nsDeXpourfr0lGYMuj3XRRPSXvcCaQjkaTWc3BlGja05x3Vi51sEJIA18tFHHNrT66orhDipGJzvS+IGsEDLozguGLxm1a0BE1Z8WPSzKqWHthAO2hUyuSYFTP6BXf30luLd1QJzWcekLeKKRn6tJz79w3RuyKa2M2ig5FKe6GJW0nayGcz6b6uvkxNayZ3rBG43ik6LKd5lTOD4Kthka4csv1rzV0hZq1CeYgQBZBzPIpRdPyxJX4T8W2pAG8rvBMMyimWCY7mlGp/+ObqxIBMoD2d7y+NGhdIe0GNu+/JVWfTWrq nmqHZx0s G6aNCjkEb3cwwla8d0IxWx9uSygAlSVaILKDASeGVcJeQ8pzdPLX2kZRMhcLjl4FPCJILHfECxOcoVM5m4Fp+Fyu4xv5QzvgyVZLqitH/lifpR26S43R0r4h94acKUioJqHSd+S8IZb/tFbQwjepHUW9HB6Wubm2oZBukpUb8VYll3tg= X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On 18/07/2023 12:24, David Hildenbrand wrote: > On 18.07.23 13:23, David Hildenbrand wrote: >> On 18.07.23 12:49, Ryan Roberts wrote: >>> On 17/07/2023 18:40, David Hildenbrand wrote: >>>> On 17.07.23 12:31, Ryan Roberts wrote: >>>>> The `migration` test currently has a number of robustness problems that >>>>> cause it to hang and leak resources. >>>>> >>>>> Timeout: There are 3 tests, which each previously ran for 60 seconds. >>>>> However, the timeout in mm/settings for a single test binary was set to >>>>> 45 seconds. So when run using run_kselftest.sh, the top level timeout >>>>> would trigger before the test binary was finished. Solve this by meeting >>>>> in the middle; each of the 3 tests now runs for 20 seconds (for a total >>>>> of 60), and the top level timeout is set to 90 seconds. >>>>> >>>>> Leaking child processes: the `shared_anon` test fork()s some children >>>>> but then an ASSERT() fires before the test kills those children. The >>>>> assert causes immediate exit of the parent and leaking of the children. >>>>> Furthermore, if run using the run_kselftest.sh wrapper, the wrapper >>>>> would get stuck waiting for those children to exit, which never happens. >>>>> Solve this by deferring any asserts until after the children are killed. >>>>> The same pattern is used for the threaded tests for uniformity. >>>>> >>>>> With these changes, the test binary now runs to completion on arm64, >>>>> with 2 tests passing and the `shared_anon` test failing. >>>>> >>>>> Signed-off-by: Ryan Roberts >>>>> --- >>>>>     tools/testing/selftests/mm/migration.c | 14 ++++++++++---- >>>>>     tools/testing/selftests/mm/settings    |  2 +- >>>>>     2 files changed, 11 insertions(+), 5 deletions(-) >>>>> >>>>> diff --git a/tools/testing/selftests/mm/migration.c >>>>> b/tools/testing/selftests/mm/migration.c >>>>> index 379581567f27..189d7d9070e8 100644 >>>>> --- a/tools/testing/selftests/mm/migration.c >>>>> +++ b/tools/testing/selftests/mm/migration.c >>>>> @@ -15,7 +15,7 @@ >>>>>     #include >>>>>       #define TWOMEG (2<<20) >>>>> -#define RUNTIME (60) >>>>> +#define RUNTIME (20) >>>>>       #define ALIGN(x, a) (((x) + (a - 1)) & (~((a) - 1))) >>>>>     @@ -118,6 +118,7 @@ TEST_F_TIMEOUT(migration, private_anon, 2*RUNTIME) >>>>>     { >>>>>         uint64_t *ptr; >>>>>         int i; >>>>> +    int ret; >>>>>           if (self->nthreads < 2 || self->n1 < 0 || self->n2 < 0) >>>>>             SKIP(return, "Not enough threads or NUMA nodes available"); >>>>> @@ -131,9 +132,10 @@ TEST_F_TIMEOUT(migration, private_anon, 2*RUNTIME) >>>>>             if (pthread_create(&self->threads[i], NULL, access_mem, ptr)) >>>>>                 perror("Couldn't create thread"); >>>>>     -    ASSERT_EQ(migrate(ptr, self->n1, self->n2), 0); >>>>> +    ret = migrate(ptr, self->n1, self->n2); >>>>>         for (i = 0; i < self->nthreads - 1; i++) >>>>>             ASSERT_EQ(pthread_cancel(self->threads[i]), 0); >>>>> +    ASSERT_EQ(ret, 0); >>>> >>>> Why is that required? This does not involve fork. >>> >>> It's not required. I was just trying to keep everything aligned to the same >>> pattern. >>> >>>> >>>>>     } >>>>>       /* >>>>> @@ -144,6 +146,7 @@ TEST_F_TIMEOUT(migration, shared_anon, 2*RUNTIME) >>>>>         pid_t pid; >>>>>         uint64_t *ptr; >>>>>         int i; >>>>> +    int ret; >>>>>           if (self->nthreads < 2 || self->n1 < 0 || self->n2 < 0) >>>>>             SKIP(return, "Not enough threads or NUMA nodes available"); >>>>> @@ -161,9 +164,10 @@ TEST_F_TIMEOUT(migration, shared_anon, 2*RUNTIME) >>>>>                 self->pids[i] = pid; >>>>>         } >>>>>     -    ASSERT_EQ(migrate(ptr, self->n1, self->n2), 0); >>>>> +    ret = migrate(ptr, self->n1, self->n2); >>>>>         for (i = 0; i < self->nthreads - 1; i++) >>>>>             ASSERT_EQ(kill(self->pids[i], SIGTERM), 0); >>>>> +    ASSERT_EQ(ret, 0); >>>> >>>> >>>> Might be cleaner to also: >>> >>> Or instead of? I agree this is neater, so will undo the moving of the ASSERT() >>> and rely on this prctl. >> >> I was thinking about possible races when our parent process already >> quits before our child managed to set the prctl. prctl() won't do >> anything in that case, hmmmm. >> >> But similarly, existing code might already trigger the migrate() + kill >> before the child processes even started to access_mem(). >> >> Racy :) >> > > Maybe what would work, is checking after the prctl() in the child if the parent > is already gone. Like this? if (!pid) { prctl(PR_SET_PDEATHSIG, SIGHUP); /* Parent may have died before prctl so check now. */ if (getppid() == 1) kill(getpid(), SIGHUP); access_mem(ptr); } >