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 393F2EB64DA for ; Wed, 5 Jul 2023 18:17:12 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id D11388D0001; Wed, 5 Jul 2023 14:17:11 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id C9B2C8E0006; Wed, 5 Jul 2023 14:17:11 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id AC6AF8D0008; Wed, 5 Jul 2023 14:17:11 -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 82DD88D0001 for ; Wed, 5 Jul 2023 14:17:11 -0400 (EDT) Received: from smtpin08.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay02.hostedemail.com (Postfix) with ESMTP id 563DE120441 for ; Wed, 5 Jul 2023 18:17:11 +0000 (UTC) X-FDA: 80978365062.08.58D0BCC Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by imf01.hostedemail.com (Postfix) with ESMTP id 2067C4001A for ; Wed, 5 Jul 2023 18:17:08 +0000 (UTC) Authentication-Results: imf01.hostedemail.com; dkim=pass header.d=redhat.com header.s=mimecast20190719 header.b=YFQOyAQ2; dmarc=pass (policy=none) header.from=redhat.com; spf=pass (imf01.hostedemail.com: domain of peterx@redhat.com designates 170.10.129.124 as permitted sender) smtp.mailfrom=peterx@redhat.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1688581029; a=rsa-sha256; cv=none; b=vTB7iOlpXwdF24CCI+fQByrBh9zyJNDCAaZ2HTgEOxsTqWmsxrFlWzbYciX0UmnBA8Qy9W EBFa/TbHhR9XKAlHgcvOK4DClWUf1xT0i/5pirIoYyzHh4o6Dj3TvkOVPsV7bxDzg69Ait bxRhHRb74FHrnLjRPKQPoCPxWWwqcNo= ARC-Authentication-Results: i=1; imf01.hostedemail.com; dkim=pass header.d=redhat.com header.s=mimecast20190719 header.b=YFQOyAQ2; dmarc=pass (policy=none) header.from=redhat.com; spf=pass (imf01.hostedemail.com: domain of peterx@redhat.com designates 170.10.129.124 as permitted sender) smtp.mailfrom=peterx@redhat.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1688581029; 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=PzsylmtcmFNsmgGBg8Pm9OUDSeIGp5f091Q7tc9LleI=; b=LH6UNapOI/yYuDO/c+KXJbLEKXJsX/u0tsP8SbkzFSc/s1R7nRIkG2paLeepq5C/co1XI1 2ofWRPWlWRrso2l3KO5NiUwytMPxawMzXLrS+jKC87dtDls3fyOScuR9iv1VG3XABdU2NZ SmKUhe/jIzYZzHgozQWbBHRP2wd+ats= DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1688581028; 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=PzsylmtcmFNsmgGBg8Pm9OUDSeIGp5f091Q7tc9LleI=; b=YFQOyAQ2JT+/Xfo5orFii3QgGHORMAdb5A0C6znAJFlt1BiHYzwhdstnz/pdNSC9KQ9EuF EgKCFqNDz94XZAiEck+9xE0EhsXLbBbTif1+kE9mXt1Ej97eD+y1Q6+AXVYZz8FJ/YwVyj hRhgREeygpP3tJtIXAsh6cmWRpaihdg= Received: from mail-qv1-f72.google.com (mail-qv1-f72.google.com [209.85.219.72]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-365-xmVP5HVQPqyelq197i_TsA-1; Wed, 05 Jul 2023 14:17:05 -0400 X-MC-Unique: xmVP5HVQPqyelq197i_TsA-1 Received: by mail-qv1-f72.google.com with SMTP id 6a1803df08f44-635eb5b04e1so15370886d6.1 for ; Wed, 05 Jul 2023 11:17:04 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1688581024; x=1689185824; h=in-reply-to:content-transfer-encoding:content-disposition :mime-version:references:message-id:subject:cc:to:from:date :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=PzsylmtcmFNsmgGBg8Pm9OUDSeIGp5f091Q7tc9LleI=; b=OajsaBqLg8vmSdY4bqj8dinUFMk+B7RaNr8TCd2EPrEDT7vYYHGIVyx+5QofhB+49D AXum7EvYM4Nl6L8wdY/Fo42tcB2/x241+14o/BXJFYljmWjaw+MNUWKc67KXDiqXdj3v vZ1ma8N+HiAjpgL0vKyOHgIfGAYCtZg7kVeXxTRHu8T6B+RZHG+oVzJw6cG8cNw7XsWZ di/U5TaY3r3zrok/MQE9QhjByhfXQ7Bak8t9f1xACUCHadRueES+AQrvj6Ip+DFBCeG+ oNhF90kljGMUOrVas8J92l7DR3V6DJQpkGzAox/fX0H5tSQJN1PgZpkQThXHQdlcwFd2 CE2w== X-Gm-Message-State: ABy/qLbc7lAUTIzy2oBajxNntEjfUYLosdLmzpnYJHX3rodjZknWWlnS 8qpp8+z1XRafzUx6l4zF7difm55sorXY1ZQX4VLaO4EgYkJnkjzBh0Mfg0wj/Wa+lV23hs0rUTk YMnHUMEmbk84= X-Received: by 2002:a0c:ea2b:0:b0:635:dfe1:c1f2 with SMTP id t11-20020a0cea2b000000b00635dfe1c1f2mr17717923qvp.0.1688581024473; Wed, 05 Jul 2023 11:17:04 -0700 (PDT) X-Google-Smtp-Source: APBJJlEl96/J1yNKCHluvfZQOMJkmNFqvRJ5M9uxuly3x1gZ1FBowCv3rY84RzXq6qWZEIAdEnHPdg== X-Received: by 2002:a0c:ea2b:0:b0:635:dfe1:c1f2 with SMTP id t11-20020a0cea2b000000b00635dfe1c1f2mr17717904qvp.0.1688581024161; Wed, 05 Jul 2023 11:17:04 -0700 (PDT) Received: from x1n (cpe5c7695f3aee0-cm5c7695f3aede.cpe.net.cable.rogers.com. [99.254.144.39]) by smtp.gmail.com with ESMTPSA id l15-20020ad4444f000000b0062439f05b87sm13920874qvt.45.2023.07.05.11.17.02 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 05 Jul 2023 11:17:03 -0700 (PDT) Date: Wed, 5 Jul 2023 14:17:02 -0400 From: Peter Xu To: Axel Rasmussen Cc: Alexander Viro , Andrew Morton , Christian Brauner , David Hildenbrand , Huang Ying , Hugh Dickins , James Houghton , Jiaqi Yan , Jonathan Corbet , "Liam R. Howlett" , Miaohe Lin , Mike Kravetz , "Mike Rapoport (IBM)" , Muchun Song , Nadav Amit , Naoya Horiguchi , Shuah Khan , ZhangPeng , linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org, linux-mm@kvack.org, linux-kselftest@vger.kernel.org Subject: Re: [PATCH v2 4/6] selftests/mm: refactor uffd_poll_thread to allow custom fault handlers Message-ID: References: <20230629205040.665834-1-axelrasmussen@google.com> <20230629205040.665834-4-axelrasmussen@google.com> MIME-Version: 1.0 In-Reply-To: X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit X-Rspam-User: X-Rspamd-Server: rspam06 X-Rspamd-Queue-Id: 2067C4001A X-Stat-Signature: n3edz7opsfr6bty69npt5aattneufq3z X-HE-Tag: 1688581028-988493 X-HE-Meta: U2FsdGVkX1/je2dXGGjQMFdBRtROB9GOwYxy2JmYfU/JoQwVLKYD9tq8IpN6PAhR5u01N8Yf3x7bHoCr8L6mSIzeU9lUk7Ag0BDZ+rDYXuNOwORl8uNxk7Ak9k9iFf5rPETXK4KuLAD4NGB/fz7ec8woz3ekFqYeM73jx9CHys9z4T6c6Ac7n+ql2dKiK+wZqcJlf3mhe3W9a556n57yqFcScf597A1k1xalrKpuZ11dW3XeTkFfSakpNpWGfL0XLjr3PAjq//9U4icwCECou8nxBmVgdF4ht97orlOHxg9RmBAiDiot9n+5uOr+uBYCfxvBXe3WbTuGvkeh3UO62bHQUx11kpRwQydYHT1Obclov2c8SahNN22tcRXS/V/T+lDcx3yAe/4m99YgFLAWKKb3Cq2+zziUePIom0KH2OjWxtrD0gllLIK7Kt84ezzPo+LkWHzM1T2dMoSnwovNkg1DTsnbPNSEzxzjN3B0+BfY7Ca5u2k/z29lzjo9CACjj0Lc3qXF3kkgCIBCHhZO6gK0TLFeWg+7IGkQHh2RoCTQ8A6DBFbZJXYV3qLehgIODgHLmg/xr2/UHtMVxFYubGAx1rqYOlWRj39+ZNJf9KYB346su5RlZWs2ySYOaP6uiDTZONZvSQ7A9Jf1Wf/xRyvMVPaPHrOI9+q4drTOshx4k1+YodtzUye4yYRkIl0ZJYtU41bE6czgVRXxCox1hWpt9/xuzs4yj4jiwL6V25ycnEjnJUb5wjBIF0U40U/165txll0D7w+wJrY3wTYmslIdehZ6T5+MVbER5avxgM2/dKcw52r9SiwFXBmVUMBtkClOe1RldfIsiclasvEig355gn/rMGKdduA52R99sI4QYWdFCQ33d9hx5rS8A7i8eKg76pjyS8S7q1mTi/WhrtY5Gafa6HuawTcAvNmXJiewjynvdZGCjOkRpKBd/twrR+GPWUVmccfg/CFkW4r Ehlg3S0+ KRzTR42OOYkkCP+MhcYQGIte6ErOBa7dkjLKHPjb0XgrcWfWlRrzrdiRoJkA04MMeFN5mKjyJQgemSHT0VfshVTGub+OHGYbVkVlfu+MvoIz4QYJVKMfcUsVDAgAsNLzocqeKBT3mwE0w49uJ5ug8lhJUyJIYNUMhSEt3DQNjYsjSTL/Spj/V2Nm2rPnWe06ZzROkJrE2emE8nlobwQqMhbswawUDz+4/YU7Qz6v3O5tsGUBI7RKIZDzOKa5zYE/IN7l+Ou+t7okWZyRBaLNrMdOSK74I7STs+EkTc8BcLzM9rXD2wuQ9RBZuEQzvfmDl99a1fllnpkUA+7lHPb6c768OfeOJKRFetmuwhHAKPa8OM6IzqKwE4zg9ekjvMr74OEoIOVzkrMuGL2d8ip9OCkbqKYzfyY7EfjW2FxK+6OQMVnZTgRY+LMq8V2XAd2/UWHRA3uMWlbNIqvU= 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 Wed, Jul 05, 2023 at 10:50:34AM -0700, Axel Rasmussen wrote: > On Tue, Jul 4, 2023 at 2:03 PM Peter Xu wrote: > > > > On Thu, Jun 29, 2023 at 01:50:38PM -0700, Axel Rasmussen wrote: > > > Previously, we had "one fault handler to rule them all", which used > > > several branches to deal with all of the scenarios required by all of > > > the various tests. > > > > > > In upcoming patches, I plan to add a new test, which has its own > > > slightly different fault handling logic. Instead of continuing to add > > > cruft to the existing fault handler, let's allow tests to define custom > > > ones, separate from other tests. > > > > > > Signed-off-by: Axel Rasmussen > > > --- > > > tools/testing/selftests/mm/uffd-common.c | 5 ++++- > > > tools/testing/selftests/mm/uffd-common.h | 3 +++ > > > tools/testing/selftests/mm/uffd-stress.c | 12 +++++++----- > > > 3 files changed, 14 insertions(+), 6 deletions(-) > > > > > > diff --git a/tools/testing/selftests/mm/uffd-common.c b/tools/testing/selftests/mm/uffd-common.c > > > index ba20d7504022..02b89860e193 100644 > > > --- a/tools/testing/selftests/mm/uffd-common.c > > > +++ b/tools/testing/selftests/mm/uffd-common.c > > > @@ -499,6 +499,9 @@ void *uffd_poll_thread(void *arg) > > > int ret; > > > char tmp_chr; > > > > > > + if (!args->handle_fault) > > > + args->handle_fault = uffd_handle_page_fault; > > > + > > > pollfd[0].fd = uffd; > > > pollfd[0].events = POLLIN; > > > pollfd[1].fd = pipefd[cpu*2]; > > > @@ -527,7 +530,7 @@ void *uffd_poll_thread(void *arg) > > > err("unexpected msg event %u\n", msg.event); > > > break; > > > case UFFD_EVENT_PAGEFAULT: > > > - uffd_handle_page_fault(&msg, args); > > > + args->handle_fault(&msg, args); > > > break; > > > case UFFD_EVENT_FORK: > > > close(uffd); > > > diff --git a/tools/testing/selftests/mm/uffd-common.h b/tools/testing/selftests/mm/uffd-common.h > > > index 197f5262fe0d..7c4fa964c3b0 100644 > > > --- a/tools/testing/selftests/mm/uffd-common.h > > > +++ b/tools/testing/selftests/mm/uffd-common.h > > > @@ -77,6 +77,9 @@ struct uffd_args { > > > unsigned long missing_faults; > > > unsigned long wp_faults; > > > unsigned long minor_faults; > > > + > > > + /* A custom fault handler; defaults to uffd_handle_page_fault. */ > > > + void (*handle_fault)(struct uffd_msg *msg, struct uffd_args *args); > > > }; > > > > > > struct uffd_test_ops { > > > diff --git a/tools/testing/selftests/mm/uffd-stress.c b/tools/testing/selftests/mm/uffd-stress.c > > > index 995ff13e74c7..50b1224d72c7 100644 > > > --- a/tools/testing/selftests/mm/uffd-stress.c > > > +++ b/tools/testing/selftests/mm/uffd-stress.c > > > @@ -189,10 +189,8 @@ static int stress(struct uffd_args *args) > > > locking_thread, (void *)cpu)) > > > return 1; > > > if (bounces & BOUNCE_POLL) { > > > - if (pthread_create(&uffd_threads[cpu], &attr, > > > - uffd_poll_thread, > > > - (void *)&args[cpu])) > > > - return 1; > > > + if (pthread_create(&uffd_threads[cpu], &attr, uffd_poll_thread, &args[cpu])) > > > + err("uffd_poll_thread create"); > > > > irrelevant change? > > Right, I'll revert this. In an earlier version I had a more > substantial change here, and just didn't fully revert it. > > > > > > } else { > > > if (pthread_create(&uffd_threads[cpu], &attr, > > > uffd_read_thread, > > > @@ -247,9 +245,13 @@ static int userfaultfd_stress(void) > > > { > > > void *area; > > > unsigned long nr; > > > - struct uffd_args args[nr_cpus]; > > > + struct uffd_args *args; > > > uint64_t mem_size = nr_pages * page_size; > > > > > > + args = calloc(nr_cpus, sizeof(struct uffd_args)); > > > + if (!args) > > > + err("allocating args array failed"); > > > + > > > > It's leaked? > > > > Isn't "args[] = { 0 }" already working? > > That works, but GCC can warn in this case (-Wmissing-braces) depending > on the definition of struct uffd_args. I liked switching to calloc > because it avoids any possibility of that even as we add/remove things > to struct uffd_args in the future. But afaict we also have other places using the static way, I hope we can still keep them the same. > > Since it's a selftest and this function is only called exactly once, > it didn't seem worth the code making certain we free it, instead just > leaving it to be cleaned up when the process exits. I'm also fine if you like to allocate it, but in that case please free it even if used only once - we're already at it, no good reason to explicitly leak it. Let's just assume if userfaultfd_stress() can succeed it can be called 100000.. times without leaking anything. Thanks, -- Peter Xu