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 8E27BC05027 for ; Thu, 26 Jan 2023 17:27:07 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 1D07E8E0001; Thu, 26 Jan 2023 12:27:07 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 159C66B0073; Thu, 26 Jan 2023 12:27:07 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id F15098E0001; Thu, 26 Jan 2023 12:27:06 -0500 (EST) 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 DBFFD6B0072 for ; Thu, 26 Jan 2023 12:27:06 -0500 (EST) Received: from smtpin23.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay05.hostedemail.com (Postfix) with ESMTP id B416240391 for ; Thu, 26 Jan 2023 17:27:06 +0000 (UTC) X-FDA: 80397630852.23.5498BD1 Received: from outbound-smtp22.blacknight.com (outbound-smtp22.blacknight.com [81.17.249.190]) by imf17.hostedemail.com (Postfix) with ESMTP id D34C140015 for ; Thu, 26 Jan 2023 17:27:03 +0000 (UTC) Authentication-Results: imf17.hostedemail.com; dkim=none; spf=pass (imf17.hostedemail.com: domain of mgorman@techsingularity.net designates 81.17.249.190 as permitted sender) smtp.mailfrom=mgorman@techsingularity.net; dmarc=none ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1674754024; 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; bh=Cs285hn94vV961f69GEDMVt5x0F1UuSZW4uiiqQ2mKE=; b=SjWIo2TAhCpl04OG0NlmKmX5wBC0i6Y6S3KS/8amtGFHJhUGnx7Cghopbymz5CcheMa6Ie 6sz9C8+FDiy/BILntQ3+1XN7yppXddOsAMkZax+HZx8ELY3T4oUgqxf+QqGttL/Xeqf42s 0HXw85RzS0JoLCzOL609nOgaK+roN8E= ARC-Authentication-Results: i=1; imf17.hostedemail.com; dkim=none; spf=pass (imf17.hostedemail.com: domain of mgorman@techsingularity.net designates 81.17.249.190 as permitted sender) smtp.mailfrom=mgorman@techsingularity.net; dmarc=none ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1674754024; a=rsa-sha256; cv=none; b=gPI/Y/4skS+gybpvM2AF7mLS5DcSL4FsePNKa+qe/Bi2/k2wvXWmGOGK9zgFSENgJpOrtb TGDPAFcXfgyxbkpOLXVEfxqNXw+ZdQL6p9DWufCvOW4Hcj2mPKlKsHUqQF8IUqReRLOwEY DeZu4d3oJGj22tWshz7QOdEc8f6ai9k= Received: from mail.blacknight.com (pemlinmail04.blacknight.ie [81.17.254.17]) by outbound-smtp22.blacknight.com (Postfix) with ESMTPS id F22A0BAACE for ; Thu, 26 Jan 2023 17:27:01 +0000 (GMT) Received: (qmail 17492 invoked from network); 26 Jan 2023 17:27:01 -0000 Received: from unknown (HELO techsingularity.net) (mgorman@techsingularity.net@[84.203.198.246]) by 81.17.254.9 with ESMTPSA (AES256-SHA encrypted, authenticated); 26 Jan 2023 17:27:01 -0000 Date: Thu, 26 Jan 2023 17:26:57 +0000 From: Mel Gorman To: Suren Baghdasaryan Cc: akpm@linux-foundation.org, michel@lespinasse.org, jglisse@google.com, mhocko@suse.com, vbabka@suse.cz, hannes@cmpxchg.org, dave@stgolabs.net, willy@infradead.org, liam.howlett@oracle.com, peterz@infradead.org, ldufour@linux.ibm.com, paulmck@kernel.org, mingo@redhat.com, will@kernel.org, luto@kernel.org, songliubraving@fb.com, peterx@redhat.com, david@redhat.com, dhowells@redhat.com, hughd@google.com, bigeasy@linutronix.de, kent.overstreet@linux.dev, punit.agrawal@bytedance.com, lstoakes@gmail.com, peterjung1337@gmail.com, rientjes@google.com, axelrasmussen@google.com, joelaf@google.com, minchan@google.com, jannh@google.com, shakeelb@google.com, tatashin@google.com, edumazet@google.com, gthelen@google.com, gurua@google.com, arjunroy@google.com, soheil@google.com, hughlynch@google.com, leewalsh@google.com, posk@google.com, linux-mm@kvack.org, linux-arm-kernel@lists.infradead.org, linuxppc-dev@lists.ozlabs.org, x86@kernel.org, linux-kernel@vger.kernel.org, kernel-team@android.com Subject: Re: [PATCH v3 4/7] mm: replace vma->vm_flags direct modifications with modifier calls Message-ID: <20230126172657.bmga5hy74ifsbhun@techsingularity.net> References: <20230125233554.153109-1-surenb@google.com> <20230125233554.153109-5-surenb@google.com> <20230126151015.ru2m26jkhwib6x6u@techsingularity.net> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-15 Content-Disposition: inline In-Reply-To: X-Stat-Signature: x6rf9ot5n3th9ohao5zyihhuruh5oshw X-Rspam-User: X-Rspamd-Queue-Id: D34C140015 X-Rspamd-Server: rspam06 X-HE-Tag: 1674754023-620015 X-HE-Meta: U2FsdGVkX1/XK6FEs+I25hQ+8TMRLG2n5hs8DnGI8ZwFjrvDUkjMpLfBkNAJhisO7dEhTquJYHkh4Ieinhj2s9TW5TI8KougC16jFz9lzVnGZ51GJqgnkiFJgxyMjkk/61MeWh3KNoucnv77wjapMh1az3ISvKordzlUG9h8Bw7XOfo5U4iO5WhgoiXFxImYRUi1m+PNzETqG3JZRUDfAePRE8CiYJjSsrsilhtLvSk5LvzEVXe9nP8lJtKqT8mSS6UqqNDgKiJ28Ea3GO56ApFGRvkSYUj6EQmG6OHEY/SxTOKxt2gjWpXdfB6WblJHtBElfT+AA2hAFj1RZlZCrBxz9l+N6Y6DUHHKG1/sdfDz+K3eTnENL+f+DYCBlzctLeUgDoWP4yZ3RDulNsGDcvJ7tub5i2ITmIpL/qz6w2FWAg2x02T3fG053WNZ++LRYLmkwZ788uE+lyqeHeABYXZ4jgJC+j6Zd63UwZnZQV5UMSAXroRZU1QwXiRVxFKJ588djoAXuPFk0KIvuxuGPyGjJTncYjSirCN04a+5EYrT3ZOgS9Dh+vpMt6l6rbkYianlj3UXu3FMOZKpSPdwkk+WNms3wVT7Pf+xBeb0UZpE9Zt8EwyQOthtE4pOW30l55pe35csvGqdLX31on3CgUYyi8BQhFDa9hiaSzKN4Tx+3rsxdBWwYuHNDb8VC8nqbQ6Mx+uwG94pP7rCXImSu7+ezOgTVZgYOiRgm126yn1VwugJ34oKa4fr4jbN7nVcaEwA3wUas4uNQhTIWO4bqZnXdXuMunzNRkhkiZOmsuzb/04hkbr7LwRKDq1j5AURF4FI87tHkwgEYU/SjAVMTaIOAI3w9yB8hUT0iFlSI2WnwW3KLgCYSOeTBYj1TDxJLiMl0R1qIWcLzgxwMR9sQVXopwpnDA9/JD6LrMUMtr1eARjJNoXusvZMsKg/oZUYyl1o3TntSmdg67y0mF1 APlBFLTs tgaxayELy+UA5A/giIXNH41X9Yzbh288gpAfp2vhyynWWzBdIZ+lljik1iX6eCexR+nu5aq2JNPtn6GtEenGghziA0PzhmZLjF9O2C0Oj4ArMthJtqbGe6YImZA6SZBpLSSEpZZaJ4NXPEmFYuze7/1xeM3o2fp635RVRPXoUUmEY76c7hRHSJs6tX4coS0HFXym3SqXwXDahrs0lGDK+y7HgVF47WoC/Xh10WUp/hiDl0csXaVOXtoteVKfgV+V0oCE4Gia6t3Bx9vtz5nf/6U0SeBbmGxV63xkYyJnqq35df9nzzy2saJcgyh4RUbt5zkmTXihzudWNClbKTHKtRobsyQZPKwAdvbJyhyO570E5CM+aUwJu53EmZw== 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 Thu, Jan 26, 2023 at 08:10:26AM -0800, Suren Baghdasaryan wrote: > On Thu, Jan 26, 2023 at 7:10 AM Mel Gorman wrote: > > > > On Wed, Jan 25, 2023 at 03:35:51PM -0800, Suren Baghdasaryan wrote: > > > Replace direct modifications to vma->vm_flags with calls to modifier > > > functions to be able to track flag changes and to keep vma locking > > > correctness. > > > > > > Signed-off-by: Suren Baghdasaryan > > > Acked-by: Michal Hocko > > > > Acked-by: Mel Gorman > > > > Minor comments that are safe to ignore. > > > > I think a better name for mod_vm_flags is set_clear_vm_flags to hint that > > the first flags are to be set and the second flags are to be cleared. > > For this patch, it doesn't matter, but it might avoid accidental swapping > > in the future. > > > > reset_vm_flags might also be better named as reinit_vma_flags (or > > vma_flags_reinit). Maybe also encourage the use of [set|clear_mod]_vm_flags > > where possible in the comment to track exactly what is changing and > > why. Some cases like userfaultfd just want to clear __VM_UFFD_FLAGS but > > altering the flow in this patch is inappropriate and error prone. Others > > such as the infiniband changes and madvise are a lot more complex. > > That's a good point, but I don't want people to use mod_vm_flags() for > the cases when the order of set/clear really matters. In such cases > set_vm_flags() and clear_vm_flags() should be explicitly used. Maybe > to make that clear I should add a comment and rewrite the functions > as: > > void mod_vm_flags(vma, set, clear) { > vma.vm_flags = vma.vm_flags | set & clear; > } > Offhand, I'm not thinking of a case where that really matters and as they are not necessarily ordered, it's raising a read flag so yes, it definitely it needs a comment if the ordering matters. > In this patchset it's not that obvious but mod_vm_flags() was really > introduced in the original per-VMA lock patchset for efficiency to > avoid taking extra per-VMA locks. A combo of > set_vm_flags()+clear_vm_flags() would try to retake the same per-VMA > lock in the second call while mod_vm_flags() takes the lock only once > and does both operations. Ok, that seems fair but still needs a comment on why a mod_vm_flags is not necessarily equivalent to a set_vm_flags + clear_vm_flags in terms of correctness if that is indeed the case. > Not a huge overhead because we check if the > lock is already taken and bail out early but still... > So, would the above modification to mod_vm_flags() address your concern? > My concerns are entirely with the callers, not the implementation. If someone is modifying a call site using mod_vm_flags, they have to read through all the preceding logic to ensure the final combination of flags is valid. It's a code maintenance issue, not a correctness issue. -- Mel Gorman SUSE Labs