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 AD56EC2D0CD for ; Mon, 19 May 2025 13:50:49 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 7F1FC6B00C3; Mon, 19 May 2025 09:50:46 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 778F16B00D2; Mon, 19 May 2025 09:50:46 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 61BD16B00D3; Mon, 19 May 2025 09:50:46 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0015.hostedemail.com [216.40.44.15]) by kanga.kvack.org (Postfix) with ESMTP id 3E7306B00C3 for ; Mon, 19 May 2025 09:50:46 -0400 (EDT) Received: from smtpin02.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay07.hostedemail.com (Postfix) with ESMTP id 446FE160313 for ; Mon, 19 May 2025 13:50:48 +0000 (UTC) X-FDA: 83459792976.02.B429767 Received: from mail-pl1-f180.google.com (mail-pl1-f180.google.com [209.85.214.180]) by imf03.hostedemail.com (Postfix) with ESMTP id 70CD420012 for ; Mon, 19 May 2025 13:50:46 +0000 (UTC) Authentication-Results: imf03.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=aWYni2t7; spf=pass (imf03.hostedemail.com: domain of ujwal.kundur@gmail.com designates 209.85.214.180 as permitted sender) smtp.mailfrom=ujwal.kundur@gmail.com; dmarc=pass (policy=none) header.from=gmail.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1747662646; 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=+IutGAVL4WWh1zXRehsI47yja7zQgYOoHFSMk8JjWWw=; b=rUes3E7VyYKbdY50mtSLi8Zv/A//sNk5y9uOBKRhqdAsEpbMSWIL5wQR67VQGu6yoUzrhT e57xtzed1+5ymNby9N6P9nkOInxyiqAGTHZEf2oo9m4v+/AKQndQ42SxXCsTEF3WeigkQy 1lh+A3hnV22hp95EHBifExCXhhr3EqU= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1747662646; a=rsa-sha256; cv=none; b=YVz/58IW7lXDY+ftaL0ehO+5inSozbBGcYzhF3AVfoQ8M2Qe+TEJ3nA6QL1iz2CWs0Utal 8pqlMQNAmB5AQkXaF71jWVBq7xncvYep9tK7gGovnKH0f3UYQJ+JTCDYdLjg83TxGDBQRx tzXZxpYa12z41VPQJIF3jDSC8OzWCUU= ARC-Authentication-Results: i=1; imf03.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=aWYni2t7; spf=pass (imf03.hostedemail.com: domain of ujwal.kundur@gmail.com designates 209.85.214.180 as permitted sender) smtp.mailfrom=ujwal.kundur@gmail.com; dmarc=pass (policy=none) header.from=gmail.com Received: by mail-pl1-f180.google.com with SMTP id d9443c01a7336-231ecd4f2a5so22780005ad.0 for ; Mon, 19 May 2025 06:50:46 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1747662645; x=1748267445; darn=kvack.org; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=+IutGAVL4WWh1zXRehsI47yja7zQgYOoHFSMk8JjWWw=; b=aWYni2t7w9eMBSEgSqsYnzo4mcV+JYNsG74L1B9sF3clGAn9tPXhxstiXUjbv8lsc1 pSkzJqAUz4rIUnHSuBYPnqpY872IMAgtiPzl39DFBHtFlcqZID70Ns93jvMbavZcNCOC Wx3IPVqDnomHyeYOuYd3mKsusKLR/7Z8gqSVodvYZWUgU0gQR6F643ey7Es1JPB4DHkl MsnfYPZq4KkCA2U0yZc7pYoNAU0nV4oI8Iyrk0dH6UrjUHN4tMBDv3Iwa6ofMrg2nYca i51Zd9e7uxnYrBqp3TDynttQgQfPdBS5lcGvYIh52aTJXQ8yCHj3TB4qLhd8FzQjLJht FCqQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1747662645; x=1748267445; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=+IutGAVL4WWh1zXRehsI47yja7zQgYOoHFSMk8JjWWw=; b=LSQqFalZU4Ar4GiqUKWpA4zHUq9IK0s6ReC5mr+pJHNvgp3AWORrDCF5weL5RH6TMD v+eKLCY8jw8Oqxg2JpmQlDXPmic1eAMzJIsNpVqCqRjM4GF2BvxPn2knEPUZCtUMYp2v xepJfdA9hUVC5w55BVr50uawNcfv5NfKxBRS4WsieQeu1OESaAcIQQrnmm+OezbgQWuW GKA742abwS+wOVr4VxQQ7Sw94cV7yF9eJ4fo32Fy9jbcTnNCqCMn+c2cbNc6jxnq2dc9 v8Wp7La0JunYOpBgkJV9mJtS/wOU/YaxHA6UG17PSPFn1Fo7zovEkM7uxSomW9h9gqrv KIww== X-Forwarded-Encrypted: i=1; AJvYcCVeNmLrpja24CeQt33mhvKsFVStZAqpGs9h6yTNIjW3uVbvAwT//FqwV9XRGhMKXnpVMZ9jjpGmtA==@kvack.org X-Gm-Message-State: AOJu0YyaGp41k/gyqyL4eYGWUwsNOuT8DGI1OmM0G9lTPCQCVRY8ICGk dN9YN7RA+oRlMmj4+pLK0TI+0y0vaKUhm3fftNqy3wrtEj8pu2ZvzQVPS5FzQtvbdkHXCK/KWhj rqrTs0VlGMvPE2Qdcq0hETvPb+cyTtw== X-Gm-Gg: ASbGncvpWtslXnQgMU7L+oC9Ohc+57gTJ941Np6G/97eMWTOhbTINQLjir6SZToL6jR tsIL68CAfstm2sYdcver0YbwK/dJXGuk57ECShb+W4BJTNln+WLHAgHsft+fbLUXJLKxRQvmOpZ kgfZgWOJsU6ZeqABsullVUoKqy8DkI+uXYALvA+QmC X-Google-Smtp-Source: AGHT+IHOw/C0cymFVsWT9h2zFuzJv8a6dLH/krZaPX7GaHu5SRtUJOAew2rIPQlCdRZi5X5oJ3dtyFj4eSqKfRmk47w= X-Received: by 2002:a17:903:46d0:b0:223:f408:c3f8 with SMTP id d9443c01a7336-231de3027c4mr178943295ad.14.1747662645131; Mon, 19 May 2025 06:50:45 -0700 (PDT) MIME-Version: 1.0 References: <20250501163827.2598-1-ujwal.kundur@gmail.com> <20250510160335.1898-1-ujwal.kundur@gmail.com> In-Reply-To: From: Ujwal Kundur Date: Mon, 19 May 2025 19:20:31 +0530 X-Gm-Features: AX0GCFte3IuNk4JZd3aGgSh0Dprg-2xwuXlgFjve6MpCE3EWera5vSmU1YghjE4 Message-ID: Subject: Re: [PATCH v3 1/1] selftests/mm/uffd: Refactor non-composite global vars into struct To: Brendan Jackman Cc: akpm@linux-foundation.org, peterx@redhat.com, shuah@kernel.org, linux-mm@kvack.org, linux-kselftest@vger.kernel.org, linux-kernel@vger.kernel.org Content-Type: text/plain; charset="UTF-8" X-Rspamd-Server: rspam08 X-Rspamd-Queue-Id: 70CD420012 X-Stat-Signature: et1ibjszp1fzkucpw5wtj34y7mxmown9 X-Rspam-User: X-HE-Tag: 1747662646-273858 X-HE-Meta: U2FsdGVkX18MwH9HZqfgUdSVEUU/S10zbeY73TM9MJ+A0rmj6oqfUPyP3QroDFcIjXZ1XbHaaP/63kcgO7w75Sww5LKRkhpxYos22azNQfFl+8J61k/Vr8WxLb54ASuRA5tQA467Z3zxIx/9lXwX9se9y2ft/YGPfv+OaX/MzFjZJbuQl47UpS0NxWOYxqkUqm2n/m3Kits8UFqXppoNXCNnsKN6hLXtgA6c+bQca88o/QKd2/SUyv2PiJEkGNub+Sy2sLwNYIvPPVL/3ywoG8jPYiIw/xM4jr8AS9+j95XjlpE8qB7grLrG7egNgqXSyiQ78nx0e8jcAxas7Oms8Ks6rpSbHOpp3OxuYvgEn4X5Kj9fQpUxj/hS/2C82vVJ2TeV8cg/6tGIcgNfd11IEckmRFnsZcLOFgy7/sztXz+nVtqVLHXU+9afa1xVWK6ke3bpSMy0AC9r1iUVNb2IIgDqrvsHlgAM61X9NxxLwqOfyvkRojVsSDW0slC1IiNB4vASwIXO9rRO/50ufW8McUL3l2CsRAQvveEV3epPUpMqq0tE3svkmwqqrNA2jSJt4Wr7d0fF/7cqQG/X3p7YQL6kc3tTzUtRTyDG8jbyCTwlHfy+o2JDziy45351Gzd85fInNIk8rqralfHwI1le7e2oTwbjiGPaUPaD0rs9SdkSr8aqICkPHaFDBHCTeTO9ve5P6VUJ9dhiZKcNC3b8ZfMHPBl247Tsapdmo+II0ZSY+R8c9oYmm9YcRylGxyNaSkd4kJeR8qJasyIdgM0andHpQnw8CyWKGandpro8mEtuISlEM3NmHwYbNjTjhEeNRp3AEMJnIrEUQ5Hy1UQbh0mgF6rVhrsVMUZ7PBbm/J59vksg+PS8mKMIu2W1F/Nx07NeHUiKvWIu7WIZFtGWDm+Av9reWVJp3pD+Yk09PSf9cKg90UpQMmdvbJpIVu6CCfKSDlrbH+RwN9VBh9g 9ZWJzo8s MB/9i1UShbjk/MXo3yodaztFVBhnTB9ivvaCONuBziE774MxmB8NC16X4IXbeZDpzzHYNIlHUkJzrNASMtOy+QNIlfMWQb0lUucgrcRPIQvfeAP5b2ln9yLaxPxFIzj4kzhkuEj0F2zg8XGWBMBoF+A8qI6OQpKUE+/l8N3ORAgx/HFUiqIPaSvzSSYBnyV2/6/5dnFPBkGZ23FBYbWi1gkz1WVczhxTXM+OuZct4t/+MchZdRIVZZxvxF2Oir99J2LbyXl4MwBjR/CmfWX+LdcP9/MQMpaQGd1Crh955QWBMrRiBbvoU0nnhkMn0rtKrwisutmD7uf2R6PpminkndhxN7MTrkdK92XoG 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: 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) static void retry_copy_page(uffd_global_test_opts_t *gopts, struct uffdio_copy *uffdio_copy,$ ^I^I^I^I^I^I^Iunsigned long offset)$ {$ ^Iuffd_test_ops->alias_mapping(gopts,$ ^I^I^I^I^I^I^I^I&uffdio_copy->dst,$ ^I^I^I^I^I^I^I^Iuffdio_copy->len,$ ^I^I^I^I^I^I^I^Ioffset);$ ^Iif (ioctl(gopts->uffd, UFFDIO_COPY, uffdio_copy)) {$ ^I^I/* real retval in ufdio_copy.copy */$ ^I^Iif (uffdio_copy->copy != -EEXIST)$ ^I^I^Ierr("UFFDIO_COPY retry error: %"PRId64,$ ^I^I^I (int64_t)uffdio_copy->copy);$ ^I} else {$ ^I^Ierr("UFFDIO_COPY retry unexpected: %"PRId64,$ ^I^I (int64_t)uffdio_copy->copy);$ ^I}$ I checked this against master: $ cat -A uffd-common.c | grep -A 15 retry_copy_page static void retry_copy_page(int ufd, struct uffdio_copy *uffdio_copy,$ ^I^I^I unsigned long offset)$ {$ ^Iuffd_test_ops->alias_mapping(&uffdio_copy->dst,$ ^I^I^I^I uffdio_copy->len,$ ^I^I^I^I offset);$ ^Iif (ioctl(ufd, UFFDIO_COPY, uffdio_copy)) {$ ^I^I/* real retval in ufdio_copy.copy */$ ^I^Iif (uffdio_copy->copy != -EEXIST)$ ^I^I^Ierr("UFFDIO_COPY retry error: %"PRId64,$ ^I^I^I (int64_t)uffdio_copy->copy);$ ^I} else {$ ^I^Ierr("UFFDIO_COPY retry unexpected: %"PRId64,$ ^I^I (int64_t)uffdio_copy->copy);$ ^I}$ }$ and there seem to be spaces mixed in earlier causing the formatting issues. It looks okay to me after I pulled in the patch to my local repo. > 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. > 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? > + /* TODO: remove this global var.. it's so ugly */ > > That's done :) Oh I misunderstood that as "remove nr_parallel" which is not the case, will fix. > @@ -1734,6 +1737,27 @@ int main(int argc, char *argv[]) > } > for (j = 0; j < n_mems; j++) { > mem_type = &mem_types[j]; > + > + // Initialize global test options > > Wrong comment style here Will fix I'm not sure about the protocol here, should I roll a PATCH v4 or a new patch entirely? Planning on addressing another TODO for dynamically setting the BASE_PMD_ADDR, I can roll the fixes as part of that patch if required.