Re: [PATCH 1/5] mm: Introduce mm_struct.has_pinned
From: John Hubbard
Date: Tue Sep 22 2020 - 15:11:09 EST
On 9/22/20 8:17 AM, Peter Xu wrote:
On Mon, Sep 21, 2020 at 04:53:38PM -0700, John Hubbard wrote:
On 9/21/20 2:17 PM, Peter Xu wrote:
...
diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
index 496c3ff97cce..6f291f8b74c6 100644
--- a/include/linux/mm_types.h
+++ b/include/linux/mm_types.h
@@ -441,6 +441,16 @@ struct mm_struct {
#endif
int map_count; /* number of VMAs */
+ /**
+ * @has_pinned: Whether this mm has pinned any pages. This can
+ * be either replaced in the future by @pinned_vm when it
+ * becomes stable, or grow into a counter on its own. We're
+ * aggresive on this bit now - even if the pinned pages were
+ * unpinned later on, we'll still keep this bit set for the
+ * lifecycle of this mm just for simplicity.
+ */
+ int has_pinned;
I think this would be elegant as an atomic_t, and using atomic_set() and
atomic_read(), which seem even more self-documenting that what you have here.
But it's admittedly a cosmetic point, combined with my perennial fear that
I'm missing something when I look at a READ_ONCE()/WRITE_ONCE() pair. :)
Yeah but I hope I'm using it right.. :) I used READ_ONCE/WRITE_ONCE explicitly
because I think they're cheaper than atomic operations, (which will, iiuc, lock
the bus).
The "cheaper" argument vanishes if the longer-term plan is to use atomic ops.
Given the intent of this patchset, simpler is better, and "simpler that has the
same performance as the longer term solution" is definitely OK.
It's completely OK to just ignore this comment, but I didn't want to completely
miss the opportunity to make it a tiny bit cleaner to the reader.
This can always become an atomic in the future, or am I wrong? Actually if
we're going to the counter way I feel like it's a must.
Yes it can change. But you can get the simplicity and clarity now, rather
than waiting.
thanks,
--
John Hubbard
NVIDIA