Re: [mm PATCH v3 1/6] mm: Use mm_zero_struct_page from SPARC on all 64b architectures
From: Alexander Duyck
Date: Wed Oct 17 2018 - 12:31:19 EST
On 10/17/2018 8:40 AM, David Laight wrote:
From: Pavel Tatashin
Sent: 17 October 2018 16:12
On 10/17/18 11:07 AM, Alexander Duyck wrote:
On 10/17/2018 1:47 AM, Michal Hocko wrote:
On Mon 15-10-18 13:26:56, Alexander Duyck wrote:
This change makes it so that we use the same approach that was
already in
use on Sparc on all the archtectures that support a 64b long.
This is mostly motivated by the fact that 8 to 10 store/move
instructions
are likely always going to be faster than having to call into a function
that is not specialized for handling page init.
An added advantage to doing it this way is that the compiler can get
away
with combining writes in the __init_single_page call. As a result the
memset call will be reduced to only about 4 write operations, or at
least
that is what I am seeing with GCC 6.2 as the flags, LRU poitners, and
count/mapcount seem to be cancelling out at least 4 of the 8
assignments on
my system.
One change I had to make to the function was to reduce the minimum page
size to 56 to support some powerpc64 configurations.
This really begs for numbers. I do not mind the change itself with some
minor comments below.
[...]
diff --git a/include/linux/mm.h b/include/linux/mm.h
index bb0de406f8e7..ec6e57a0c14e 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -102,8 +102,42 @@ static inline void set_max_mapnr(unsigned long
limit) { }
ÂÂ * zeroing by defining this macro in <asm/pgtable.h>.
ÂÂ */
 #ifndef mm_zero_struct_page
Do we still need this ifdef? I guess we can wait for an arch which
doesn't like this change and then add the override. I would rather go
simple if possible.
We probably don't, but as soon as I remove it somebody will probably
complain somewhere. I guess I could drop it for now and see if anybody
screams. Adding it back should be pretty straight forward since it would
only be 2 lines.
+#if BITS_PER_LONG == 64
+/* This function must be updated when the size of struct page grows
above 80
+ * or reduces below 64. The idea that compiler optimizes out switch()
+ * statement, and only leaves move/store instructions
+ */
+#defineÂÂÂ mm_zero_struct_page(pp) __mm_zero_struct_page(pp)
+static inline void __mm_zero_struct_page(struct page *page)
+{
+ÂÂÂ unsigned long *_pp = (void *)page;
+
+ÂÂÂÂ /* Check that struct page is either 56, 64, 72, or 80 bytes */
+ÂÂÂ BUILD_BUG_ON(sizeof(struct page) & 7);
+ÂÂÂ BUILD_BUG_ON(sizeof(struct page) < 56);
+ÂÂÂ BUILD_BUG_ON(sizeof(struct page) > 80);
+
+ÂÂÂ switch (sizeof(struct page)) {
+ÂÂÂ case 80:
+ÂÂÂÂÂÂÂ _pp[9] = 0;ÂÂÂ /* fallthrough */
+ÂÂÂ case 72:
+ÂÂÂÂÂÂÂ _pp[8] = 0;ÂÂÂ /* fallthrough */
+ÂÂÂ default:
+ÂÂÂÂÂÂÂ _pp[7] = 0;ÂÂÂ /* fallthrough */
+ÂÂÂ case 56:
+ÂÂÂÂÂÂÂ _pp[6] = 0;
+ÂÂÂÂÂÂÂ _pp[5] = 0;
+ÂÂÂÂÂÂÂ _pp[4] = 0;
+ÂÂÂÂÂÂÂ _pp[3] = 0;
+ÂÂÂÂÂÂÂ _pp[2] = 0;
+ÂÂÂÂÂÂÂ _pp[1] = 0;
+ÂÂÂÂÂÂÂ _pp[0] = 0;
+ÂÂÂ }
This just hit my eyes. I have to confess I have never seen default: to
be not the last one in the switch. Can we have case 64 instead or does
gcc
complain? I would be surprised with the set of BUILD_BUG_ONs.
It was me, C does not really care where default is placed, I was trying
to keep stores sequential for better cache locality, but "case 64"
should be OK, and even better for this purpose.
You'd need to put memory barriers between them to force sequential stores.
I'm also surprised that gcc doesn't inline the memset().
David
We don't need them to be sequential. The general idea is we have have to
fill a given amount of space with 0s. After that we have some calls that
are initialing the memory that doesn't have to be zero. Ideally the
compiler is smart enough to realize that since we don't have barriers
and we are performing assignments after the assignment of zero it can
just combine the two writes into one and drop the zero assignment.
- Alex