From: Suren Baghdasaryan <surenb@google.com>
To: Mel Gorman <mgorman@techsingularity.net>
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
Date: Thu, 26 Jan 2023 08:10:26 -0800 [thread overview]
Message-ID: <CAJuCfpEzAbpy9rZ5KeZXQsqFTPOGYv6CZQfP9SHqcqFi0s7neg@mail.gmail.com> (raw)
In-Reply-To: <20230126151015.ru2m26jkhwib6x6u@techsingularity.net>
On Thu, Jan 26, 2023 at 7:10 AM Mel Gorman <mgorman@techsingularity.net> 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 <surenb@google.com>
> > Acked-by: Michal Hocko <mhocko@suse.com>
>
> Acked-by: Mel Gorman <mgorman@techsingularity.net>
>
> 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;
}
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. 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?
>
> --
> Mel Gorman
> SUSE Labs
WARNING: multiple messages have this Message-ID (diff)
From: Suren Baghdasaryan <surenb@google.com>
To: Mel Gorman <mgorman@techsingularity.net>
Cc: michel@lespinasse.org, joelaf@google.com, songliubraving@fb.com,
mhocko@suse.com, leewalsh@google.com, david@redhat.com,
peterz@infradead.org, bigeasy@linutronix.de, peterx@redhat.com,
dhowells@redhat.com, linux-mm@kvack.org, edumazet@google.com,
jglisse@google.com, punit.agrawal@bytedance.com, will@kernel.org,
arjunroy@google.com, dave@stgolabs.net, minchan@google.com,
x86@kernel.org, hughd@google.com, willy@infradead.org,
gurua@google.com, mingo@redhat.com,
linux-arm-kernel@lists.infradead.org, rientjes@google.com,
axelrasmussen@google.com, kernel-team@android.com,
soheil@google.com, paulmck@kernel.org, jannh@google.com,
liam.howlett@oracle.com, shakeelb@google.com, luto@kernel.org,
gthelen@google.com, ldufour@linux.ibm.com, vbabka@suse.cz,
posk@google.com, lstoakes@gmail.com, peterjung1337@gmail.com,
kent.overstreet@linux.dev, hughlynch@google.com,
linux-kernel@vger.kernel.org, hannes@cmpxchg.org,
akpm@linux-foundation.org, tatashin@google.com,
linuxppc-dev@lists.ozlabs.org
Subject: Re: [PATCH v3 4/7] mm: replace vma->vm_flags direct modifications with modifier calls
Date: Thu, 26 Jan 2023 08:10:26 -0800 [thread overview]
Message-ID: <CAJuCfpEzAbpy9rZ5KeZXQsqFTPOGYv6CZQfP9SHqcqFi0s7neg@mail.gmail.com> (raw)
In-Reply-To: <20230126151015.ru2m26jkhwib6x6u@techsingularity.net>
On Thu, Jan 26, 2023 at 7:10 AM Mel Gorman <mgorman@techsingularity.net> 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 <surenb@google.com>
> > Acked-by: Michal Hocko <mhocko@suse.com>
>
> Acked-by: Mel Gorman <mgorman@techsingularity.net>
>
> 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;
}
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. 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?
>
> --
> Mel Gorman
> SUSE Labs
WARNING: multiple messages have this Message-ID (diff)
From: Suren Baghdasaryan <surenb@google.com>
To: Mel Gorman <mgorman@techsingularity.net>
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
Date: Thu, 26 Jan 2023 08:10:26 -0800 [thread overview]
Message-ID: <CAJuCfpEzAbpy9rZ5KeZXQsqFTPOGYv6CZQfP9SHqcqFi0s7neg@mail.gmail.com> (raw)
In-Reply-To: <20230126151015.ru2m26jkhwib6x6u@techsingularity.net>
On Thu, Jan 26, 2023 at 7:10 AM Mel Gorman <mgorman@techsingularity.net> 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 <surenb@google.com>
> > Acked-by: Michal Hocko <mhocko@suse.com>
>
> Acked-by: Mel Gorman <mgorman@techsingularity.net>
>
> 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;
}
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. 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?
>
> --
> Mel Gorman
> SUSE Labs
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2023-01-26 16:10 UTC|newest]
Thread overview: 118+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-01-25 23:35 [PATCH v3 0/7] introduce vm_flags modifier functions Suren Baghdasaryan
2023-01-25 23:35 ` Suren Baghdasaryan
2023-01-25 23:35 ` Suren Baghdasaryan
2023-01-25 23:35 ` [PATCH v3 1/7] kernel/fork: convert vma assignment to a memcpy Suren Baghdasaryan
2023-01-25 23:35 ` Suren Baghdasaryan
2023-01-25 23:35 ` Suren Baghdasaryan
2023-01-26 0:21 ` Andrew Morton
2023-01-26 0:21 ` Andrew Morton
2023-01-26 0:21 ` Andrew Morton
2023-01-26 0:50 ` Suren Baghdasaryan
2023-01-26 0:50 ` Suren Baghdasaryan
2023-01-26 0:50 ` Suren Baghdasaryan
2023-01-26 1:34 ` Andrew Morton
2023-01-26 1:34 ` Andrew Morton
2023-01-26 1:34 ` Andrew Morton
2023-01-26 11:52 ` Mel Gorman
2023-01-26 11:52 ` Mel Gorman
2023-01-26 11:52 ` Mel Gorman
2023-01-26 15:59 ` Suren Baghdasaryan
2023-01-26 15:59 ` Suren Baghdasaryan
2023-01-26 15:59 ` Suren Baghdasaryan
2023-01-26 17:27 ` Paul E. McKenney
2023-01-26 17:27 ` Paul E. McKenney
2023-01-26 17:27 ` Paul E. McKenney
2023-01-26 17:27 ` Paul E. McKenney
2023-02-07 17:16 ` Marco Elver
2023-02-07 17:16 ` Marco Elver
2023-02-07 17:16 ` Marco Elver
2023-02-07 17:23 ` Suren Baghdasaryan
2023-02-07 17:23 ` Suren Baghdasaryan
2023-02-07 17:23 ` Suren Baghdasaryan
2023-02-07 17:51 ` Marco Elver
2023-02-07 17:51 ` Marco Elver
2023-02-07 17:51 ` Marco Elver
2023-01-25 23:35 ` [PATCH v3 2/7] mm: introduce vma->vm_flags wrapper functions Suren Baghdasaryan
2023-01-25 23:35 ` Suren Baghdasaryan
2023-01-25 23:35 ` Suren Baghdasaryan
2023-01-26 0:24 ` Andrew Morton
2023-01-26 0:24 ` Andrew Morton
2023-01-26 0:24 ` Andrew Morton
2023-01-26 0:51 ` Suren Baghdasaryan
2023-01-26 0:51 ` Suren Baghdasaryan
2023-01-26 0:51 ` Suren Baghdasaryan
2023-01-26 17:48 ` Davidlohr Bueso
2023-01-26 17:48 ` Davidlohr Bueso
2023-01-26 17:48 ` Davidlohr Bueso
2023-01-26 0:28 ` Andrew Morton
2023-01-26 0:28 ` Andrew Morton
2023-01-26 0:28 ` Andrew Morton
2023-01-26 0:56 ` Suren Baghdasaryan
2023-01-26 0:56 ` Suren Baghdasaryan
2023-01-26 0:56 ` Suren Baghdasaryan
2023-01-26 7:59 ` Michal Hocko
2023-01-26 7:59 ` Michal Hocko
2023-01-26 7:59 ` Michal Hocko
2023-01-26 8:33 ` Michal Hocko
2023-01-26 8:33 ` Michal Hocko
2023-01-26 8:33 ` Michal Hocko
2023-01-26 13:58 ` Mel Gorman
2023-01-26 13:58 ` Mel Gorman
2023-01-26 13:58 ` Mel Gorman
2023-01-26 16:01 ` Suren Baghdasaryan
2023-01-26 16:01 ` Suren Baghdasaryan
2023-01-26 16:01 ` Suren Baghdasaryan
2023-01-25 23:35 ` [PATCH v3 3/7] mm: replace VM_LOCKED_CLEAR_MASK with VM_LOCKED_MASK Suren Baghdasaryan
2023-01-25 23:35 ` Suren Baghdasaryan
2023-01-25 23:35 ` Suren Baghdasaryan
2023-01-26 13:59 ` Mel Gorman
2023-01-26 13:59 ` Mel Gorman
2023-01-26 13:59 ` Mel Gorman
2023-01-26 14:16 ` Mel Gorman
2023-01-26 14:16 ` Mel Gorman
2023-01-26 14:16 ` Mel Gorman
2023-01-25 23:35 ` [PATCH v3 4/7] mm: replace vma->vm_flags direct modifications with modifier calls Suren Baghdasaryan
2023-01-25 23:35 ` Suren Baghdasaryan
2023-01-25 23:35 ` Suren Baghdasaryan
2023-01-26 15:10 ` Mel Gorman
2023-01-26 15:10 ` Mel Gorman
2023-01-26 15:10 ` Mel Gorman
2023-01-26 16:10 ` Suren Baghdasaryan [this message]
2023-01-26 16:10 ` Suren Baghdasaryan
2023-01-26 16:10 ` Suren Baghdasaryan
2023-01-26 17:26 ` Mel Gorman
2023-01-26 17:26 ` Mel Gorman
2023-01-26 17:26 ` Mel Gorman
2023-01-26 17:28 ` Suren Baghdasaryan
2023-01-26 17:28 ` Suren Baghdasaryan
2023-01-26 17:28 ` Suren Baghdasaryan
2023-01-25 23:35 ` [PATCH v3 5/7] mm: replace vma->vm_flags indirect modification in ksm_madvise Suren Baghdasaryan
2023-01-25 23:35 ` Suren Baghdasaryan
2023-01-25 23:35 ` Suren Baghdasaryan
2023-01-26 15:19 ` Mel Gorman
2023-01-26 15:19 ` Mel Gorman
2023-01-26 15:19 ` Mel Gorman
2023-01-26 16:11 ` Suren Baghdasaryan
2023-01-26 16:11 ` Suren Baghdasaryan
2023-01-26 16:11 ` Suren Baghdasaryan
2023-01-25 23:35 ` [PATCH v3 6/7] mm: introduce mod_vm_flags_nolock and use it in untrack_pfn Suren Baghdasaryan
2023-01-25 23:35 ` Suren Baghdasaryan
2023-01-25 23:35 ` Suren Baghdasaryan
2023-01-26 8:34 ` Michal Hocko
2023-01-26 8:34 ` Michal Hocko
2023-01-26 8:34 ` Michal Hocko
2023-01-26 15:47 ` Mel Gorman
2023-01-26 15:47 ` Mel Gorman
2023-01-26 15:47 ` Mel Gorman
2023-01-26 16:18 ` Suren Baghdasaryan
2023-01-26 16:18 ` Suren Baghdasaryan
2023-01-26 16:18 ` Suren Baghdasaryan
2023-01-26 17:32 ` Mel Gorman
2023-01-26 17:32 ` Mel Gorman
2023-01-26 17:32 ` Mel Gorman
2023-01-26 17:34 ` Suren Baghdasaryan
2023-01-26 17:34 ` Suren Baghdasaryan
2023-01-26 17:34 ` Suren Baghdasaryan
2023-01-25 23:35 ` [PATCH v3 7/7] mm: export dump_mm() Suren Baghdasaryan
2023-01-25 23:35 ` Suren Baghdasaryan
2023-01-25 23:35 ` Suren Baghdasaryan
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAJuCfpEzAbpy9rZ5KeZXQsqFTPOGYv6CZQfP9SHqcqFi0s7neg@mail.gmail.com \
--to=surenb@google.com \
--cc=akpm@linux-foundation.org \
--cc=arjunroy@google.com \
--cc=axelrasmussen@google.com \
--cc=bigeasy@linutronix.de \
--cc=dave@stgolabs.net \
--cc=david@redhat.com \
--cc=dhowells@redhat.com \
--cc=edumazet@google.com \
--cc=gthelen@google.com \
--cc=gurua@google.com \
--cc=hannes@cmpxchg.org \
--cc=hughd@google.com \
--cc=hughlynch@google.com \
--cc=jannh@google.com \
--cc=jglisse@google.com \
--cc=joelaf@google.com \
--cc=kent.overstreet@linux.dev \
--cc=kernel-team@android.com \
--cc=ldufour@linux.ibm.com \
--cc=leewalsh@google.com \
--cc=liam.howlett@oracle.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=linuxppc-dev@lists.ozlabs.org \
--cc=lstoakes@gmail.com \
--cc=luto@kernel.org \
--cc=mgorman@techsingularity.net \
--cc=mhocko@suse.com \
--cc=michel@lespinasse.org \
--cc=minchan@google.com \
--cc=mingo@redhat.com \
--cc=paulmck@kernel.org \
--cc=peterjung1337@gmail.com \
--cc=peterx@redhat.com \
--cc=peterz@infradead.org \
--cc=posk@google.com \
--cc=punit.agrawal@bytedance.com \
--cc=rientjes@google.com \
--cc=shakeelb@google.com \
--cc=soheil@google.com \
--cc=songliubraving@fb.com \
--cc=tatashin@google.com \
--cc=vbabka@suse.cz \
--cc=will@kernel.org \
--cc=willy@infradead.org \
--cc=x86@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.