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 AF405EB64DA for ; Tue, 18 Jul 2023 15:26:46 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id C49148E0002; Tue, 18 Jul 2023 11:26:45 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id BF9558D0001; Tue, 18 Jul 2023 11:26:45 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id AC0318E0002; Tue, 18 Jul 2023 11:26:45 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0016.hostedemail.com [216.40.44.16]) by kanga.kvack.org (Postfix) with ESMTP id 9BF3A8D0001 for ; Tue, 18 Jul 2023 11:26:45 -0400 (EDT) Received: from smtpin17.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay05.hostedemail.com (Postfix) with ESMTP id 552D040257 for ; Tue, 18 Jul 2023 15:26:45 +0000 (UTC) X-FDA: 81025109970.17.038AE85 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by imf23.hostedemail.com (Postfix) with ESMTP id EFFA7140025 for ; Tue, 18 Jul 2023 15:26:42 +0000 (UTC) Authentication-Results: imf23.hostedemail.com; dkim=pass header.d=redhat.com header.s=mimecast20190719 header.b=cMuS5j7U; dmarc=pass (policy=none) header.from=redhat.com; spf=pass (imf23.hostedemail.com: domain of david@redhat.com designates 170.10.133.124 as permitted sender) smtp.mailfrom=david@redhat.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1689694003; 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:dkim-signature; bh=yHlHwKpoA0bi1nyvFXl3JPkte3c23L/WObN+n1+gtHA=; b=4BJuvWG9dbMZSSKygU73l4BACUxLXDvmhj5qz3ZZyBk/RdLDkAVdg+gglGHtj9JzbUJHd4 LN+FvCsKa4Pki6QeGl4FipEXqC34opCUyR8K/0ZuBTjEHNw9mMhzs5otgtfxYNFoerMzQ4 +aqcaYNFvpY+5ofuJMM97/Z1HaeiEYA= ARC-Authentication-Results: i=1; imf23.hostedemail.com; dkim=pass header.d=redhat.com header.s=mimecast20190719 header.b=cMuS5j7U; dmarc=pass (policy=none) header.from=redhat.com; spf=pass (imf23.hostedemail.com: domain of david@redhat.com designates 170.10.133.124 as permitted sender) smtp.mailfrom=david@redhat.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1689694003; a=rsa-sha256; cv=none; b=kc+VcPCOQrMbGPEbfLQsPF/W3yhwDF7Ncn2oGqB5CDMU1RMwCPGDogCvCCa8gTY8v/x5Nr Q66JOp2K2HXfrX11L9S8suKFJFs0At0ULbcV5f4ON2flzDFDZOaP/jzK1kiRMdZPPr/57P n5OMM4h6A0sR6EdyHClL69qdL0JGHkA= DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1689693992; h=from:from: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=yHlHwKpoA0bi1nyvFXl3JPkte3c23L/WObN+n1+gtHA=; b=cMuS5j7UMG8sBwyCbK6ARs4fW+fdeM3gK/xMF0o84NayXqmF2iezN2axjzVFRkJ0FFbmN/ +N2I41B+dHwoK5OPa/gUyFCIsKE7Lva0vHDNxuE8baUgyA52bPi3C6ALtod0SWaqpImjPZ na34hJJhBt7gQSKKXGj30bs1LZNa4Pg= Received: from mail-wm1-f72.google.com (mail-wm1-f72.google.com [209.85.128.72]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-6-kXJSQPXYOq6g6QdPMfasqg-1; Tue, 18 Jul 2023 11:24:21 -0400 X-MC-Unique: kXJSQPXYOq6g6QdPMfasqg-1 Received: by mail-wm1-f72.google.com with SMTP id 5b1f17b1804b1-3fc08035926so29157225e9.0 for ; Tue, 18 Jul 2023 08:24:13 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1689693846; x=1692285846; h=content-transfer-encoding:in-reply-to:subject:organization:from :references:cc:to:content-language:user-agent:mime-version:date :message-id:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=yHlHwKpoA0bi1nyvFXl3JPkte3c23L/WObN+n1+gtHA=; b=JFPdFZPOmqw5XeUhH/TMu390wj0a+hQ2r385qWl0/4QnkjIXm7mVLG2Kin33X/lG7k PjPvGdHGvKdLb3EgQ2gMSyCVQTTxUvDIPUCgC2aPGzqPVTP1KcLUr1VQ5LN+xpPvdf7Y 0EJ93GGrRc1ha7qd/8dBBVUBAo8Sva1cbAavzj1Q5UQ/MhAUPsF7vLAkCQhm3YXU1CpB nsufVy9NTSPdbu7YYTBO8mEy31X5A7lJEvvfdHEEtf/+4+vhOHRcy+4ciWGID17jeLKn ttO+6CslLudrz8avDIYxZR+hsY1T5+uODPHHZyvZQoM8D/gJ197mhMDC71Q+0XoRjGMH xNGg== X-Gm-Message-State: ABy/qLYlYBUBcOI1d3lIEdoDSz3VEW8xuXea6+0eDFvheUTJx7a6q6Kx Jib2Ow1ri+IQ+Wh7k+zfJUEHKn6ht4WNHP9BNMNI1FvpIwXwbniJCevPuF1tUmHtDvvCzOaEAtj 6lnewTtznCJc= X-Received: by 2002:a05:600c:3ac6:b0:3fa:8c8b:716 with SMTP id d6-20020a05600c3ac600b003fa8c8b0716mr2305402wms.1.1689693845844; Tue, 18 Jul 2023 08:24:05 -0700 (PDT) X-Google-Smtp-Source: APBJJlFaI7LXpL6rcOJvQX9zLaYidgPaCoCUU3cJIXb0c7WgeTD+VlcmNLJ8Q7eYsOeTfcrz8/0FSw== X-Received: by 2002:a05:600c:3ac6:b0:3fa:8c8b:716 with SMTP id d6-20020a05600c3ac600b003fa8c8b0716mr2305379wms.1.1689693845445; Tue, 18 Jul 2023 08:24:05 -0700 (PDT) Received: from ?IPV6:2003:cb:c740:6200:84e1:eabc:8e2:7429? (p200300cbc740620084e1eabc08e27429.dip0.t-ipconnect.de. [2003:cb:c740:6200:84e1:eabc:8e2:7429]) by smtp.gmail.com with ESMTPSA id f22-20020a7bc8d6000000b003fbb5142c4bsm10735127wml.18.2023.07.18.08.24.04 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 18 Jul 2023 08:24:04 -0700 (PDT) Message-ID: <5cee5039-b641-82f6-547f-12d94057d0ad@redhat.com> Date: Tue, 18 Jul 2023 17:24:03 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.12.0 To: Ryan Roberts , 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: David Hildenbrand Organization: Red Hat Subject: Re: [PATCH v2 6/8] selftests/mm: Make migration test robust to failure In-Reply-To: X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Rspamd-Queue-Id: EFFA7140025 X-Rspam-User: X-Rspamd-Server: rspam02 X-Stat-Signature: 5q1ga6d5o9tomzsa3ah49x3brdtqe5db X-HE-Tag: 1689694002-604176 X-HE-Meta: U2FsdGVkX18swRNVhdrvlBn1tbUMy3r8O/gmX2b16wEC0ADz1MtBx7fsfhBIloueNUFv/wv6WvGr8WebEEYDCbRyioNnCszg+G06Q08LqusOhdTJ7WDEtN+1XtB+mOGEOkOdOV/wmCxjSwDdc6jsxN1MBMZkVVT4Vz2vUUJBRN2Vlit0O/VCoGcH+aMGKj2jY+rZ/gBO8sCO07LCEh1Lmanzv/nE0hVJGf0tw0Txh22tiUDd6adj4fnALVWoGLFGGTNaomj6yK5RT/PW1eO01m4pggzzPWgskshF/WZb79wszAnm9hhC7SLLT07r+7Bc3pfbujd/zEdfMhN2m+2sjMaHAgtS3YnuXDgNxrs1MJayfEZB+1Pk+gfOaxzMM5QY4vfqMqLKBFfG7Zm8AKbEssn+w982qaONY+yIL6+9u6OU36Dk/n4CV9Rh0lqBgSIN0Lx+/cfq1IRVHNsptfPRqmpsx8KxdfhVO+y+v/Qe0G2IN9xg9dsaPS8KZomPDOsi7MGpq68K4nrTHzuLHLaZE+4wMV5vcjMQucYE8Ixr2Ay+WIRL9VWXpvNZcU9ZsQgEvVS966+dTVun8XtVvV5MyUCaNnzpNQWEqnANHZLjnHwlgaid+/JveB7qw96bzZVCbRySzm3LDUO5vKSX7km4Y8jOk4NkdT0h/Bsio4Dm+PXhQFJ83QwlzS/KzGyBp1u7qawKH9FsebpjuihXVreyWIgA8i2PWb1mjVNrDGV6LLNZjNfOa7NUB4b6vcDqQf4l3UpBdgivCS6HxgYYbEvBNVlIvPDbRMq68yzsdBK3gPdwyQ652aJkBganrQ73qsr6I1w44hx/ByDJezK1NrCoVqtjmeD720YMHHuzD+6xOI6ra3k+4rj2yuDYKFcUiwxdKuxjW3wqRlB7qj3Dtk/aQXJhNvTyXx8LT+8iQTluQXFJ7FtiwBElmtsF0OHwtPcMYaJWFBiPJ00S5IBvkKO sJ/+xmAw oP/owvOcVk+o7TTBhsKMy+flsopwzzKLVaucX8oKZg0czyqOiqpbwJ2h0zoXzbT47CI4KWJ1kfzvettq9bSNh+eDZp5RzVvnOeTwFbkvq9E2zWTRxEjr1fmqpVwf0pLGdYcEUo2o9XnsFdWOpshjVEOBgxI4jbLvuaRhTc9Kti9SGkRRAUNeHOCc2lG5RoV+oICL84C0eJC3a/zmoiRQIOJKRu49b15kJlnvaX1atth3WssJhhTwEADxo1d08dt2i3fzGEFJA38Zhj2glIrP9Ohie6oeFS7xCB4TVXxIruF3on8+XOiFwrKY/v8yr4RaudSuv61rUT7Bo6CRMjw9HI6Uoz+WSo2TBizGG 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.23 14:42, Ryan Roberts wrote: > 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); > } Staring at forget_original_parent(), that order should work. I do wonder if there is a nicer way to handle that, but maybe that already is the "nice" way. -- Cheers, David / dhildenb