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 A1AE8C54756 for ; Tue, 20 May 2025 09:16:43 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 3F4F56B0092; Tue, 20 May 2025 05:16:43 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 3A5856B0096; Tue, 20 May 2025 05:16:43 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 2E32C6B0098; Tue, 20 May 2025 05:16:43 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0014.hostedemail.com [216.40.44.14]) by kanga.kvack.org (Postfix) with ESMTP id 0D7F86B0092 for ; Tue, 20 May 2025 05:16:43 -0400 (EDT) Received: from smtpin21.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay09.hostedemail.com (Postfix) with ESMTP id A7ECE80988 for ; Tue, 20 May 2025 09:16:42 +0000 (UTC) X-FDA: 83462731044.21.E61C84A Received: from mail-wr1-f74.google.com (mail-wr1-f74.google.com [209.85.221.74]) by imf23.hostedemail.com (Postfix) with ESMTP id C7038140008 for ; Tue, 20 May 2025 09:16:40 +0000 (UTC) Authentication-Results: imf23.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b="GMBW/fyk"; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf23.hostedemail.com: domain of 3d0gsaAgKCAIlcemocpdiqqing.eqonkpwz-oomxcem.qti@flex--jackmanb.bounces.google.com designates 209.85.221.74 as permitted sender) smtp.mailfrom=3d0gsaAgKCAIlcemocpdiqqing.eqonkpwz-oomxcem.qti@flex--jackmanb.bounces.google.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1747732600; 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: in-reply-to:in-reply-to:references:references:dkim-signature; bh=M49HFsU6Pb6WX2e+1aRCio5ocQDP8AMxA4CCf+KlIDA=; b=wrpu3ieZEEJBXzpxa8ZSNxpkeaE+hQvPVPs7zUBCjJs0Ba5QMZgKUVWVBQWVhkzTD+44dA u+g1H74amJw0TXbgzCrJKUokshSNXuFYElQ3TQ7Xm+E48FMKc8szQnTLy/PApCSKupnNo5 6ZyzuT8Qhw+3JU15Z5Bt1krJxv2LxhI= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1747732600; a=rsa-sha256; cv=none; b=Gnm9hP7a/XiGbsIr+mw7smzf85FPxcdUoeq2AfUF8i/vekf8vhpSLQXrDxOSBiT903AHn7 f3vncWzPAvT5+zlVbmNa2vHH72OcMtah8MqjQFKCT3n5E4Xh+f11bedbCaUP44RQ+O8vUl y2BsGYRA4F1GOPuQy5Mnu9MwL4X+sYg= ARC-Authentication-Results: i=1; imf23.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b="GMBW/fyk"; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf23.hostedemail.com: domain of 3d0gsaAgKCAIlcemocpdiqqing.eqonkpwz-oomxcem.qti@flex--jackmanb.bounces.google.com designates 209.85.221.74 as permitted sender) smtp.mailfrom=3d0gsaAgKCAIlcemocpdiqqing.eqonkpwz-oomxcem.qti@flex--jackmanb.bounces.google.com Received: by mail-wr1-f74.google.com with SMTP id ffacd0b85a97d-3a3591c42d4so914588f8f.3 for ; Tue, 20 May 2025 02:16:40 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1747732599; x=1748337399; darn=kvack.org; h=cc:to:from:subject:message-id:references:mime-version:in-reply-to :date:from:to:cc:subject:date:message-id:reply-to; bh=M49HFsU6Pb6WX2e+1aRCio5ocQDP8AMxA4CCf+KlIDA=; b=GMBW/fykvEAOipnWLeWmCF3gGcbkUXqpK48idEOcV/F1cFqojQqn0JG9+QlrEgLuH7 s1L6MwEfJ7FUftpBwsAj4sPp2Z4f0Vk0msndzEKUAxmCtNnjbXR7xDPa7AMqVcGP+f1M tr95uxa6dk+rrviX9tBNfhbbentggxHajJpnsnbKxlFTDl/yF+hoBDb8SzQNSTaOIAxe 3R7kNMzUASwMMdHN/DEi48CNh3AS5s/gpo73DHjdF/HMrYNYrviaEc7623Q4SgMed3P/ ihQu56NJ2fwF5UwXnIflnSpMk9fZczGCBbRanoNj4OH/0VH9BEropbensKFxTlKoCzpH I+cA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1747732599; x=1748337399; h=cc:to:from:subject:message-id:references:mime-version:in-reply-to :date:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=M49HFsU6Pb6WX2e+1aRCio5ocQDP8AMxA4CCf+KlIDA=; b=htbSU8cGopctEu+3ppZcdbJxSvRdzAVsv4qGBCFeLSCGDDjTmVAD1sP4b2F2HoLCQA 0wFJnxV6QZpl2/K8ZmS/e7BwHEHaRwSFp+gVrDoDFp+/mj5ROPc4KYEOme/4nB305tJ8 O3g30xY6pT90XuwD9QYMEFJwkAXste+zYu4hL7SO0bL41Dk9wjfwkUkIT8EZ9hp7S9GI G+q/RPOYDzDeOH5wxbDeXEi+QLMH2tGar4c2fP0A/E4SRn1ePoj/m6czr74/l/nQZXwa Hju+Qc9JzD5yGzfY77ZkNr1Oenh+F5UCDs5WDwb8IK/44qbNWi7hq0m+5Kf+L2LCesKs FB5w== X-Forwarded-Encrypted: i=1; AJvYcCUQAaQo0FjtG8DOPTfkK6ysTshQB+Vwbc44etqfoLOHm1rdnTigjdqbiwEJNREA86gliP0oTnDlzw==@kvack.org X-Gm-Message-State: AOJu0Yx4yJSHVhn3rDtLbq8iWs6oMvedcuhob/+XhHsLF/YFAH7DJUtO ixiegItPnogC3sjPcjP0FxE/38r9uFD0yOYOKFR+8RWElCotzbbMR6ZQHqj4iQ2SGdzi1yYbSz6 YZLU/ZsTblLqaag== X-Google-Smtp-Source: AGHT+IEdEysW4HTaGc37Upjr0n86XOGgGu3Xq2zEH7pHo0DLNR5gZ2MKL5FFJktJtfla8a1/c+K0w9OcvR4I+g== X-Received: from wmbfl25.prod.google.com ([2002:a05:600c:b99:b0:442:f8cb:d12a]) (user=jackmanb job=prod-delivery.src-stubby-dispatcher) by 2002:a05:6000:2012:b0:3a0:830a:3d63 with SMTP id ffacd0b85a97d-3a35c821bc7mr14418992f8f.9.1747732599206; Tue, 20 May 2025 02:16:39 -0700 (PDT) Date: Tue, 20 May 2025 09:16:37 +0000 In-Reply-To: Mime-Version: 1.0 References: <20250501163827.2598-1-ujwal.kundur@gmail.com> <20250510160335.1898-1-ujwal.kundur@gmail.com> X-Mailer: aerc 0.20.0 Message-ID: Subject: Re: [PATCH v3 1/1] selftests/mm/uffd: Refactor non-composite global vars into struct From: Brendan Jackman To: Ujwal Kundur Cc: , , , , , Content-Type: text/plain; charset="UTF-8" X-Stat-Signature: e8bazepcqoj9rzqq7gizxgta6kef5sph X-Rspam-User: X-Rspamd-Server: rspam05 X-Rspamd-Queue-Id: C7038140008 X-HE-Tag: 1747732600-850146 X-HE-Meta: U2FsdGVkX19p7xsj/pMvokUj+gRq1jz8W0y32nVHe9e78oSP3kN04Tgl5mKiB8Wh5hxCX+cfTcIFppBPewo15SJHmVWSfj6rfgTZOEs1Ar2DzVSLrEt6iGkfmqNxFWbCwZpIw8TSMrUcBYbBWDiIpX52rbmxHp/hUXVXC1k2q/iHxvY1YYnHajOsJ63/d1WZvRykkwZWFLaqT29HzRuDm/+Ax3rHhcpTUfbHHIj4TL5dN/TExJWQm7EaQ8d42t1P/BSt9mYhvVPsdDV/I5c0lB8S2Eb/OOX9GUCyrKGTQWLhWasLNW7nVot0UP6i2dehVaN9li8OuWQ6xpu/d5fj/gBQIyqfnQPdojpCP7kemMr992l/jWkroObkyCJDeX5CfJDqREAM+U/B6FjiqeM9SJSW0bqT7wPQS2PzOOVmfbgHQud4eOb7OJZxt5Iw6GPtZP94LBg3Sgu5ujknWJyabqfgA5jwZnKAns1yfYiANirnToI+0ZvxNi4yVIrzgA2haRzkD3S8XMblReUZGbfBOQ0pVz0pmsfQQJAXikLq5q4zmwSpHxLPJyXIPa+iGR0xTcdTpTh3CFMhuRnRxE0+20dzMqJUWi1NdrU5tEzxKYohH1AwIaN3dS0uKL3wwoktYplLdE7FJ7jxlvFWRDeYEyRczmS6nSmg3hroQpq9BlRRVvCReiUVKmFn5Gps0VRTFlUEwpG/3Je4kWmSaQQoqLGvMJyGYikLelh6F2dt8kwTKaddCeAbNW7EWsDYY+2xigBFGmpPIFWkPTUOB62ZhUgTU36YYMZj/NYoFgCzIJ/Y5BrPboh1saayau7fK5XPgofvD7hzj9HZtyJC21a4tmaLwLR5gm3zesoASLbohIbYX9e479a7D/4dCCaEJxqeXs2SkOiXvRyw+AhNMTFIXcL/FAx9iwpjWpg8hbLN+btozP0SOLqgoJ0Bwhuglsevalf86yeOk3NilL1JF10 A/f1ZIao jgxidTvus1XVjX/3hCx+KD82ObBUq4n4teQpJYa9s4idsv861ABz+cKE4t9VhsLHejI/OK6IaZLYw2H24XFU0IOhtVh03TGyKCZZppsvipUaJUCApHey+JeItbxanUJDRasBXTIqawhymKtUGsqSzs/F9xua7aQZtSnK37M6IvfoRThkdKppwiNCfXWM60vrvkq5oZyTl+2UHKFsIcxv3OWIWT5q7jgD18NnhiJEMO2waa4YDVSpIjRogRA0wM6EEaUUyIt6LwFDr0zdPK7+0SKx+uBfow5ttdKkuFHA9P+vQJa78KG40qw1dTIVbkkkxOwGCjp8RKkYHcWN+oz+iNVX7c/ceVWeZnCw8WUB71b1H2siP0NJ3tqRB1CW8FL6eNnD8DAVIeXH5R6+5LfawNwnGd6Jsg1BFMTyWQYFYnNDZ89MWN46L+N1t19r30za/g7zFKD2nPzE/c9Z6eE6QEZsYGVr8jNjnb2s8Ddy1mjFMrhFWmGyJhlOX8kVqwJ2gp61Yl7nX+bmB8qw/A5N7x9U+iSXm2JkkunT26BRRCqCC+5rK0R5Pos061sy7ToXzlyyNYC/bQ99a9ntgh5p+KAfqYCkykAwnYrE8kLOGr6leOyC8HVlkBrC0Mo5Av3ZRwrBKH71m5TvpMC8= 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: List-Subscribe: List-Unsubscribe: On Mon May 19, 2025 at 1:50 PM UTC, Ujwal Kundur wrote: > Thanks for the review and testing! > >>> -static void retry_copy_page(int ufd, struct uffdio_copy *uffdio_copy, >>> - unsigned long offset) >>> +static void retry_copy_page(uffd_global_test_opts_t *gopts, struct uffdio_copy *uffdio_copy, >>> + unsigned long offset) >>> { >>> - uffd_test_ops->alias_mapping(&uffdio_copy->dst, >>> - uffdio_copy->len, >>> - offset); >>> - if (ioctl(ufd, UFFDIO_COPY, uffdio_copy)) { >>> + uffd_test_ops->alias_mapping(gopts, >>> + &uffdio_copy->dst, >>> + uffdio_copy->len, >>> + offset); > >> Looks like your editor got a bit excited here :D >> >> There are a few other places where this happened too, e.g. the >> are_count() declaration and there's a pthread_create_call() that's quite >> messed up. > > I use vim with `:set list` turned on to display control characters and > it looked fine to me when I submitted the patch :( > Here's the output of $ cat -A uffd-common.c | grep -A 15 retry_copy_page: > (where ^I represents a tab equivalent to 4 spaces) Sounds like that's your issue - for the kernel, tab is supposed to be as wide as 8 spaces, not 4. >> Unfortunately I don't know of any tool that can find/fix these issues >> automatically without also messing up the whole file. Could you just >> do a visual skim and fix what you can spot? > > Yeah, I ran clang-format and it's playing by its own rules -- do we > have a standard .clang-format file? > Please let me know if there's a better way to find/fix whitespace > formatting issues, I found a few inconsistencies which I will fix. There is a .clang-format in the tree. The issue is that (AFAIK) there's no way to retrofit clang-format really, it has to be applied to the entire source file so if there are pre-existing deviations from its configuration then it will "fix" those too, creating a bunch of diff noise. I think we just have to do it manually. checkpatch.pl can help with some formatting issues (and it is diff-aware) but I don't think it knows anything about this kind of hanging indentation stuff. > >> static void sigalrm(int sig) >> { >> if (sig != SIGALRM) >> abort(); >> - test_uffdio_copy_eexist = true; >> + // TODO: Set this without access to global vars >> + // gopts->test_uffdio_copy_eexist = true; >> >> Did you mean to leave this like that? > > Nice catch! I was hoping someone would suggest a better way to handle > this. As far as I can tell, this was written the way it was as a > consequence of using global variables. > I think this sets up retries for copies in a racy way using alarm(2) / > signal(2), not sure how effective that has proven to be. I believe the > only way to keep this functionality would be to introduce some async > action (libev, libuv, etc.), but is that required? I'm afraid I'm too ignorant of this code to be able to suggest something good here. But, can we just remove the comment and plumb the gopts through to uffd_poll_thread()->uffd_handle_page_fault()->__copy_page()? This is not pretty but it lets us remove the global vars which is clearly a step in the right direciton.