Skip to contentSkip to conversation

Implement MTE tag compression for swapped pages

PATCH Superseded Alexander Potapenko 4 participants 14 messages Oct 6, 2023 – Oct 10, 2023

Subsystems (4)

v6 of 12 Superseded Latest is v12 →

lore
Share & tools
lore
#0 lore raw

v6 cover Implement MTE tag compression for swapped pages

Currently, when MTE pages are swapped out, the tags are kept in the memory, occupying PAGE_SIZE/32 bytes per page. This is especially problematic for devices that use zram-backed in-memory swap, because tags stored uncompressed in the heap effectively reduce the available amount of swap memory.

The RLE-based algorithm suggested by Evgenii Stepanov and implemented in this patch series is able to efficiently compress fixed-size tag buffers, resulting in practical compression ratio between 2.5x and 4x. In most cases it is possible to store the compressed data in 63-bit Xarray values, resulting in no extra memory allocations.

Our measurements show that the proposed algorithm provides better compression than existing kernel compression algorithms (LZ4, LZO, LZ4HC, ZSTD) can offer.

To implement compression/decompression, we also extend <linux/bitmap.h> with methods to read/write bit values at arbitrary places in the map.

We refactor arch/arm64/mm/mteswap.c to support both the compressed (CONFIG_ARM64_MTE_COMP) and non-compressed case. For the former, in addition to tag compression, we move tag allocation from kmalloc() to separate kmem caches, providing greater locality and relaxing the alignment requirements.

v6:
- fixed comments by Yury Norov
- fixed handling of sizes divisible by MTE_GRANULES_PER_PAGE / 2 (caught while testing on a real device)

v5:
- fixed comments by Andy Shevchenko, Catalin Marinas, and Yury Norov
- added support for 16K- and 64K pages
- more efficient bitmap_write() implementation

v4:
- fixed a bunch of comments by Andy Shevchenko and Yury Norov
- added Documentation/arch/arm64/mte-tag-compression.rst

v3:
- as suggested by Andy Shevchenko, use bitmap_get_value()/bitmap_set_value() written by Syed Nayyar Waris
- switched to unsigned long to reduce typecasts
- simplified the compression code

v2:
- as suggested by Yuri Norov, replace the poorly implemented struct bitq with <linux/bitmap.h>

Alexander Potapenko (4):
lib/test_bitmap: add tests for bitmap_{read,write}()
arm64: mte: implement CONFIG_ARM64_MTE_COMP
arm64: mte: add a test for MTE tags compression
arm64: mte: add compression support to mteswap.c

Syed Nayyar Waris (1):
lib/bitmap: add bitmap_{read,write}()

 Documentation/arch/arm64/index.rst            |   1 +
 .../arch/arm64/mte-tag-compression.rst        | 266 +++++++++
 arch/arm64/Kconfig                            |  21 +
 arch/arm64/include/asm/mtecomp.h              |  13 +
 arch/arm64/mm/Makefile                        |   7 +
 arch/arm64/mm/mtecomp.c                       | 524 ++++++++++++++++++
 arch/arm64/mm/mtecomp.h                       |  12 +
 arch/arm64/mm/mteswap.c                       |  20 +-
 arch/arm64/mm/mteswap.h                       |  12 +
 arch/arm64/mm/mteswap_comp.c                  |  60 ++
 arch/arm64/mm/mteswap_nocomp.c                |  38 ++
 arch/arm64/mm/test_mtecomp.c                  | 377 +++++++++++++
 include/linux/bitmap.h                        |  68 +++
 lib/test_bitmap.c                             | 118 ++++
 14 files changed, 1526 insertions(+), 11 deletions(-)
 create mode 100644 Documentation/arch/arm64/mte-tag-compression.rst
 create mode 100644 arch/arm64/include/asm/mtecomp.h
 create mode 100644 arch/arm64/mm/mtecomp.c
 create mode 100644 arch/arm64/mm/mtecomp.h
 create mode 100644 arch/arm64/mm/mteswap.h
 create mode 100644 arch/arm64/mm/mteswap_comp.c
 create mode 100644 arch/arm64/mm/mteswap_nocomp.c
 create mode 100644 arch/arm64/mm/test_mtecomp.c
Signature · 1 line
2.42.0.609.gbb76f46606-goog
#1 lore raw

PATCH v6 1/5 lib/bitmap: add bitmap_{read,write}()

From: Syed Nayyar Waris <[email protected]>

The two new functions allow reading/writing values of length up to BITS_PER_LONG bits at arbitrary position in the bitmap.

The code was taken from "bitops: Introduce the for_each_set_clump macro" by Syed Nayyar Waris with a number of changes and simplifications:
- instead of using roundup(), which adds an unnecessary dependency on <linux/math.h>, we calculate space as BITS_PER_LONG-offset;
- indentation is reduced by not using else-clauses (suggested by checkpatch for bitmap_get_value());
- bitmap_get_value()/bitmap_set_value() are renamed to bitmap_read() and bitmap_write();
- some redundant computations are omitted.

---
This patch was previously called "lib/bitmap: add
bitmap_{set,get}_value()"
(https://lore.kernel.org/lkml/[email protected]/)

v6:
- As suggested by Yury Norov, do not require bitmap_read(..., 0) to return 0.

v5:
- Address comments by Yury Norov:
- updated code comments and patch title/description
- replace GENMASK(nbits - 1, 0) with BITMAP_LAST_WORD_MASK(nbits)
- more compact bitmap_write() implementation

v4:
- Address comments by Andy Shevchenko and Yury Norov:
- prevent passing values >= 64 to GENMASK()
- fix commit authorship
- change comments
- check for unlikely(nbits==0)
- drop unnecessary const declarations
- fix kernel-doc comments
- rename bitmap_{get,set}_value() to bitmap_{read,write}()

---
 include/linux/bitmap.h | 68 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 68 insertions(+)
include/linux/bitmap.h +68 -0
index 03644237e1efb..e72c054d21d48 100644
@@ -76,7 +76,11 @@ struct device;
  *  bitmap_to_arr32(buf, src, nbits)            Copy nbits from buf to u32[] dst
  *  bitmap_to_arr64(buf, src, nbits)            Copy nbits from buf to u64[] dst
  *  bitmap_get_value8(map, start)               Get 8bit value from map at start
+ *  bitmap_read(map, start, nbits)              Read an nbits-sized value from
+ *                                              map at start
  *  bitmap_set_value8(map, value, start)        Set 8bit value to map at start
+ *  bitmap_write(map, value, start, nbits)      Write an nbits-sized value to
+ *                                              map at start
  *
  * Note, bitmap_zero() and bitmap_fill() operate over the region of
  * unsigned longs, that is, bits behind bitmap till the unsigned long
@@ -583,6 +587,33 @@ static inline unsigned long bitmap_get_value8(const unsigned long *map,
 	return (map[index] >> offset) & 0xFF;
 }
 
+/**
+ * bitmap_read - read a value of n-bits from the memory region
+ * @map: address to the bitmap memory region
+ * @start: bit offset of the n-bit value
+ * @nbits: size of value in bits, nonzero, up to BITS_PER_LONG
+ *
+ * Returns: value of nbits located at the @start bit offset within the @map
+ * memory region.
+ */
+static inline unsigned long bitmap_read(const unsigned long *map,
+					unsigned long start,
+					unsigned long nbits)
+{
+	size_t index = BIT_WORD(start);
+	unsigned long offset = start % BITS_PER_LONG;
+	unsigned long space = BITS_PER_LONG - offset;
+	unsigned long value_low, value_high;
+
+	if (unlikely(!nbits))
+		return 0;
+	if (space >= nbits)
+		return (map[index] >> offset) & GENMASK(nbits - 1, 0);
+	value_low = map[index] & BITMAP_FIRST_WORD_MASK(start);
+	value_high = map[index + 1] & BITMAP_LAST_WORD_MASK(start + nbits);
+	return (value_low >> offset) | (value_high << space);
+}
+
 /**
  * bitmap_set_value8 - set an 8-bit value within a memory region
  * @map: address to the bitmap memory region
@@ -599,6 +630,43 @@ static inline void bitmap_set_value8(unsigned long *map, unsigned long value,
 	map[index] |= value << offset;
 }
 
+/**
+ * bitmap_write - write n-bit value within a memory region
+ * @map: address to the bitmap memory region
+ * @value: value to write, clamped to nbits
+ * @start: bit offset of the n-bit value
+ * @nbits: size of value in bits, nonzero, up to BITS_PER_LONG.
+ *
+ * bitmap_write() behaves similarly to @nbits calls of assign_bit(), i.e. bits
+ * beyond @nbits are ignored:
+ *
+ *   for (bit = 0; bit < nbits; bit++)
+ *           assign_bit(start + bit, bitmap, val & BIT(bit));
+ */
+static inline void bitmap_write(unsigned long *map,
+				unsigned long value,
+				unsigned long start, unsigned long nbits)
+{
+	size_t index = BIT_WORD(start);
+	unsigned long offset = start % BITS_PER_LONG;
+	unsigned long space = BITS_PER_LONG - offset;
+	unsigned long mask;
+
+	if (unlikely(!nbits))
+		return;
+	mask = BITMAP_LAST_WORD_MASK(nbits);
+	value &= mask;
+	if (space >= nbits) {
+		map[index] &= ~(mask << offset);
+		map[index] |= value << offset;
+		return;
+	}
+	map[index] &= ~BITMAP_FIRST_WORD_MASK(start);
+	map[index] |= value << offset;
+	map[index + 1] &= ~BITMAP_LAST_WORD_MASK(start + nbits);
+	map[index + 1] |= (value >> space);
+}
+
 #endif /* __ASSEMBLY__ */
 
 #endif /* __LINUX_BITMAP_H */
Signature · 1 line
2.42.0.609.gbb76f46606-goog
#2 lore raw

Re: [PATCH v6 1/5] lib/bitmap: add bitmap_{read,write}()

Quoted 14 lines · Alexander Potapenko

From: Syed Nayyar Waris <[email protected]>

The two new functions allow reading/writing values of length up to BITS_PER_LONG bits at arbitrary position in the bitmap.

The code was taken from "bitops: Introduce the for_each_set_clump macro" by Syed Nayyar Waris with a number of changes and simplifications:
- instead of using roundup(), which adds an unnecessary dependency on <linux/math.h>, we calculate space as BITS_PER_LONG-offset;
- indentation is reduced by not using else-clauses (suggested by checkpatch for bitmap_get_value());
- bitmap_get_value()/bitmap_set_value() are renamed to bitmap_read() and bitmap_write();
- some redundant computations are omitted.

...

Quoted 3 lines

v6:
- As suggested by Yury Norov, do not require bitmap_read(..., 0) to return 0.

Hmm... See below.

...

Quoted 2 lines
  *  bitmap_to_arr32(buf, src, nbits)            Copy nbits from buf to u32[] dst
  *  bitmap_to_arr64(buf, src, nbits)            Copy nbits from buf to u64[] dst

With the grouping as below I would add a blank line here. But was the intention to group _arrXX() to these groups?

Quoted 6 lines
  *  bitmap_get_value8(map, start)               Get 8bit value from map at start
+ *  bitmap_read(map, start, nbits)              Read an nbits-sized value from
+ *                                              map at start
  *  bitmap_set_value8(map, value, start)        Set 8bit value to map at start
+ *  bitmap_write(map, value, start, nbits)      Write an nbits-sized value to
+ *                                              map at start

...

include/linux/bitmap.h static inline unsigned long bitmap_get_value8(const unsigned long *map, · line 609

Quoted 11 lines
include/linux/bitmap.h +10 -0
+static inline unsigned long bitmap_read(const unsigned long *map,
+					unsigned long start,
+					unsigned long nbits)
+{
+	size_t index = BIT_WORD(start);
+	unsigned long offset = start % BITS_PER_LONG;
+	unsigned long space = BITS_PER_LONG - offset;
+	unsigned long value_low, value_high;
 
+	if (unlikely(!nbits))
+		return 0;

Hmm... I didn't get was the comment to add or to remove these checks?

include/linux/bitmap.h static inline unsigned long bitmap_get_value8(const unsigned long *map, · line 611

Quoted 2 lines
include/linux/bitmap.h +2 -0
+	if (space >= nbits)
+		return (map[index] >> offset) & GENMASK(nbits - 1, 0);

And don't you want to replace this GENMASK() as well?

Quoted 4 lines
include/linux/bitmap.h +4 -0
+	value_low = map[index] & BITMAP_FIRST_WORD_MASK(start);
+	value_high = map[index + 1] & BITMAP_LAST_WORD_MASK(start + nbits);
+	return (value_low >> offset) | (value_high << space);
+}
Signature · 2 lines
With Best Regards,
Andy Shevchenko
maintainer
#3 lore raw

Re: [PATCH v6 1/5] lib/bitmap: add bitmap_{read,write}()

Quoted 23 lines · Andy Shevchenko
Quoted 14 lines · Alexander Potapenko

From: Syed Nayyar Waris <[email protected]>

The two new functions allow reading/writing values of length up to BITS_PER_LONG bits at arbitrary position in the bitmap.

The code was taken from "bitops: Introduce the for_each_set_clump macro" by Syed Nayyar Waris with a number of changes and simplifications:
- instead of using roundup(), which adds an unnecessary dependency on <linux/math.h>, we calculate space as BITS_PER_LONG-offset;
- indentation is reduced by not using else-clauses (suggested by checkpatch for bitmap_get_value());
- bitmap_get_value()/bitmap_set_value() are renamed to bitmap_read() and bitmap_write();
- some redundant computations are omitted.

...

Quoted 3 lines

v6:
- As suggested by Yury Norov, do not require bitmap_read(..., 0) to return 0.

Hmm... See below.

[...]

Quoted 9 lines
Quoted 8 lines
include/linux/bitmap.h +8 -0
+static inline unsigned long bitmap_read(const unsigned long *map,
+					unsigned long start,
+					unsigned long nbits)
+{
+	size_t index = BIT_WORD(start);
+	unsigned long offset = start % BITS_PER_LONG;
+	unsigned long space = BITS_PER_LONG - offset;
+	unsigned long value_low, value_high;
Quoted 4 lines

include/linux/bitmap.h static inline unsigned long bitmap_get_value8(const unsigned long *map, · line 609

Quoted 2 lines
include/linux/bitmap.h +2 -0
+	if (unlikely(!nbits))
+		return 0;

Hmm... I didn't get was the comment to add or to remove these checks?

The sentence relates to the test, and the comment that confused you should to to the 2nd patch.

I.e., bitmap_read(..., 0) is not required to return 0, and it's purely an implementation details.

Quoted 5 lines

include/linux/bitmap.h static inline unsigned long bitmap_get_value8(const unsigned long *map, · line 611

Quoted 3 lines
include/linux/bitmap.h +2 -0
+	if (space >= nbits)
+		return (map[index] >> offset) & GENMASK(nbits - 1, 0);

And don't you want to replace this GENMASK() as well?

+1

Quoted 9 lines
Quoted 4 lines
include/linux/bitmap.h +4 -0
+	value_low = map[index] & BITMAP_FIRST_WORD_MASK(start);
+	value_high = map[index + 1] & BITMAP_LAST_WORD_MASK(start + nbits);
+	return (value_low >> offset) | (value_high << space);
+}
Signature · 2 lines
With Best Regards,
Andy Shevchenko
#4 lore raw

Re: [PATCH v6 1/5] lib/bitmap: add bitmap_{read,write}()

In reply to Andy Shevchenko
Quoted 25 lines · Andy Shevchenko
Quoted 14 lines · Alexander Potapenko

From: Syed Nayyar Waris <[email protected]>

The two new functions allow reading/writing values of length up to BITS_PER_LONG bits at arbitrary position in the bitmap.

The code was taken from "bitops: Introduce the for_each_set_clump macro" by Syed Nayyar Waris with a number of changes and simplifications:
- instead of using roundup(), which adds an unnecessary dependency on <linux/math.h>, we calculate space as BITS_PER_LONG-offset;
- indentation is reduced by not using else-clauses (suggested by checkpatch for bitmap_get_value());
- bitmap_get_value()/bitmap_set_value() are renamed to bitmap_read() and bitmap_write();
- some redundant computations are omitted.

...

Quoted 3 lines

v6:
- As suggested by Yury Norov, do not require bitmap_read(..., 0) to return 0.

Hmm... See below.

Quoted 7 lines

...

Quoted 2 lines
  *  bitmap_to_arr32(buf, src, nbits)            Copy nbits from buf to u32[] dst
  *  bitmap_to_arr64(buf, src, nbits)            Copy nbits from buf to u64[] dst

With the grouping as below I would add a blank line here. But was the intention to group _arrXX() to these groups?

Note that there's no single blank line in this long list. What if I swap bitmap_read with bitmap_set_value8(), would the grouping become more logical?

I.e.

*  bitmap_get_value8(map, start)               Get 8bit value from map at start
*  bitmap_set_value8(map, value, start)        Set 8bit value to map at start
*  bitmap_read(map, start, nbits)              Read an nbits-sized value from
*                                              map at start
*  bitmap_write(map, value, start, nbits)      Write an nbits-sized value to
*                                              map at start
Quoted 4 lines

include/linux/bitmap.h static inline unsigned long bitmap_get_value8(const unsigned long *map, · line 609

Quoted 2 lines
include/linux/bitmap.h +2 -0
+     if (unlikely(!nbits))
+             return 0;

Hmm... I didn't get was the comment to add or to remove these checks?

As Yury said, we should not require the return value to be 0, so I added "nonzero" to the descriptions of the @nbits parameter. The check stays in place, but the users relying on it is now a mistake.

Quoted 5 lines

include/linux/bitmap.h static inline unsigned long bitmap_get_value8(const unsigned long *map, · line 611

Quoted 3 lines
include/linux/bitmap.h +2 -0
+     if (space >= nbits)
+             return (map[index] >> offset) & GENMASK(nbits - 1, 0);

And don't you want to replace this GENMASK() as well?

See my next reply to Yury, tl;dr this is a stale code version :(

#5 lore raw

Re: [PATCH v6 1/5] lib/bitmap: add bitmap_{read,write}()

Quoted 7 lines
Quoted 5 lines

include/linux/bitmap.h static inline unsigned long bitmap_get_value8(const unsigned long *map, · line 611

Quoted 3 lines
include/linux/bitmap.h +2 -0
+     if (space >= nbits)
+             return (map[index] >> offset) & GENMASK(nbits - 1, 0);

And don't you want to replace this GENMASK() as well?

See my next reply to Yury, tl;dr this is a stale code version :(

Actually, no, we haven't discussed improvements to bitmap_read(), and this is the correct version.
I'll fix this one.

maintainer
#6 lore raw

Re: [PATCH v6 1/5] lib/bitmap: add bitmap_{read,write}()

In reply to Alexander Potapenko
Quoted 107 lines · Alexander Potapenko

From: Syed Nayyar Waris <[email protected]>

The two new functions allow reading/writing values of length up to BITS_PER_LONG bits at arbitrary position in the bitmap.

The code was taken from "bitops: Introduce the for_each_set_clump macro" by Syed Nayyar Waris with a number of changes and simplifications:
- instead of using roundup(), which adds an unnecessary dependency on <linux/math.h>, we calculate space as BITS_PER_LONG-offset;
- indentation is reduced by not using else-clauses (suggested by checkpatch for bitmap_get_value());
- bitmap_get_value()/bitmap_set_value() are renamed to bitmap_read() and bitmap_write();
- some redundant computations are omitted.

---
This patch was previously called "lib/bitmap: add
bitmap_{set,get}_value()"
(https://lore.kernel.org/lkml/[email protected]/)

v6:
- As suggested by Yury Norov, do not require bitmap_read(..., 0) to return 0.

v5:
- Address comments by Yury Norov:
- updated code comments and patch title/description
- replace GENMASK(nbits - 1, 0) with BITMAP_LAST_WORD_MASK(nbits)
- more compact bitmap_write() implementation

v4:
- Address comments by Andy Shevchenko and Yury Norov:
- prevent passing values >= 64 to GENMASK()
- fix commit authorship
- change comments
- check for unlikely(nbits==0)
- drop unnecessary const declarations
- fix kernel-doc comments
- rename bitmap_{get,set}_value() to bitmap_{read,write}()

---
 include/linux/bitmap.h | 68 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 68 insertions(+)
include/linux/bitmap.h +33 -0
index 03644237e1efb..e72c054d21d48 100644
@@ -76,7 +76,11 @@ struct device;
  *  bitmap_to_arr32(buf, src, nbits)            Copy nbits from buf to u32[] dst
  *  bitmap_to_arr64(buf, src, nbits)            Copy nbits from buf to u64[] dst
  *  bitmap_get_value8(map, start)               Get 8bit value from map at start
+ *  bitmap_read(map, start, nbits)              Read an nbits-sized value from
+ *                                              map at start
  *  bitmap_set_value8(map, value, start)        Set 8bit value to map at start
+ *  bitmap_write(map, value, start, nbits)      Write an nbits-sized value to
+ *                                              map at start
  *
  * Note, bitmap_zero() and bitmap_fill() operate over the region of
  * unsigned longs, that is, bits behind bitmap till the unsigned long
@@ -583,6 +587,33 @@ static inline unsigned long bitmap_get_value8(const unsigned long *map,
 	return (map[index] >> offset) & 0xFF;
 }
 
+/**
+ * bitmap_read - read a value of n-bits from the memory region
+ * @map: address to the bitmap memory region
+ * @start: bit offset of the n-bit value
+ * @nbits: size of value in bits, nonzero, up to BITS_PER_LONG
+ *
+ * Returns: value of nbits located at the @start bit offset within the @map
+ * memory region.
+ */
+static inline unsigned long bitmap_read(const unsigned long *map,
+					unsigned long start,
+					unsigned long nbits)
+{
+	size_t index = BIT_WORD(start);
+	unsigned long offset = start % BITS_PER_LONG;
+	unsigned long space = BITS_PER_LONG - offset;
+	unsigned long value_low, value_high;
+
+	if (unlikely(!nbits))
+		return 0;
+	if (space >= nbits)
+		return (map[index] >> offset) & GENMASK(nbits - 1, 0);
+	value_low = map[index] & BITMAP_FIRST_WORD_MASK(start);
+	value_high = map[index + 1] & BITMAP_LAST_WORD_MASK(start + nbits);
+	return (value_low >> offset) | (value_high << space);
+}
+
 /**
  * bitmap_set_value8 - set an 8-bit value within a memory region
  * @map: address to the bitmap memory region
@@ -599,6 +630,43 @@ static inline void bitmap_set_value8(unsigned long *map, unsigned long value,
 	map[index] |= value << offset;
 }
 
+/**
+ * bitmap_write - write n-bit value within a memory region

include/linux/bitmap.h static inline void bitmap_set_value8(unsigned long *map, unsigned long value, · line 644

Quoted 10 lines
include/linux/bitmap.h +10 -0
+ * @map: address to the bitmap memory region
+ * @value: value to write, clamped to nbits
+ * @start: bit offset of the n-bit value
+ * @nbits: size of value in bits, nonzero, up to BITS_PER_LONG.
+ *
+ * bitmap_write() behaves similarly to @nbits calls of assign_bit(), i.e. bits
+ * beyond @nbits are ignored:
+ *
+ *   for (bit = 0; bit < nbits; bit++)
+ *           assign_bit(start + bit, bitmap, val & BIT(bit));

__assign_bit()

include/linux/bitmap.h static inline void bitmap_set_value8(unsigned long *map, unsigned long value, · line 645

Quoted 1 line
include/linux/bitmap.h +1 -0
+ */

'behaves similarly' sounds like an understatement. I think, it behaves much faster because it can assign up to 64 bits at once, not mentioning the pressure on cache lines traffic.

How faster - that's a good question. I'd be really pleased if you add a performance test for bitmap_write/read. Or I can do it myself later. You can find examples in the same lib/test_bitmap.c.

include/linux/bitmap.h static inline void bitmap_set_value8(unsigned long *map, unsigned long value, · line 656

Quoted 11 lines
include/linux/bitmap.h +11 -0
+static inline void bitmap_write(unsigned long *map,
+				unsigned long value,
+				unsigned long start, unsigned long nbits)
+{
+	size_t index = BIT_WORD(start);
+	unsigned long offset = start % BITS_PER_LONG;
+	unsigned long space = BITS_PER_LONG - offset;
+	unsigned long mask;
+
+	if (unlikely(!nbits))
+		return;

can you please add more empty lines to separate blocks visually?

include/linux/bitmap.h static inline void bitmap_set_value8(unsigned long *map, unsigned long value, · line 668

Quoted 12 lines
include/linux/bitmap.h +12 -0
+	mask = BITMAP_LAST_WORD_MASK(nbits);
+	value &= mask;
+	if (space >= nbits) {
+		map[index] &= ~(mask << offset);
+		map[index] |= value << offset;
+		return;
+	}
+	map[index] &= ~BITMAP_FIRST_WORD_MASK(start);
+	map[index] |= value << offset;
+	map[index + 1] &= ~BITMAP_LAST_WORD_MASK(start + nbits);
+	map[index + 1] |= (value >> space);
+}

I compiled the below fix on spark64 BE machine:

include/linux/bitmap.h +3 -3
@@ -608,7 +608,7 @@ static inline unsigned long bitmap_read(const unsigned long *map,
        if (unlikely(!nbits))
                return 0;
        if (space >= nbits)
-               return (map[index] >> offset) & GENMASK(nbits - 1, 0);
+               return (map[index] >> offset) & BITMAP_LAST_WORD_MASK(nbits);
        value_low = map[index] & BITMAP_FIRST_WORD_MASK(start);
        value_high = map[index + 1] & BITMAP_LAST_WORD_MASK(start + nbits);
        return (value_low >> offset) | (value_high << space);
@@ -661,9 +661,9 @@ static inline void bitmap_write(unsigned long *map,
                map[index] |= value << offset;
                return;
        }
-       map[index] &= ~BITMAP_FIRST_WORD_MASK(start);
+       map[index] &= BITMAP_LAST_WORD_MASK(start);
        map[index] |= value << offset;
-       map[index + 1] &= ~BITMAP_LAST_WORD_MASK(start + nbits);
+       map[index + 1] &= BITMAP_FIRST_WORD_MASK(start + nbits);
        map[index + 1] |= (value >> space);
 }
 

All the tests are passed just as before, and there's no any difference reported by bloat-o-meter. Can you please use non-negation versions as they are more straightforward?

Quoted 6 lines
include/linux/bitmap.h +1 -0
+
 #endif /* __ASSEMBLY__ */
 
 #endif /* __LINUX_BITMAP_H */
Signature · 1 line
2.42.0.609.gbb76f46606-goog
#7 lore raw

Re: [PATCH v6 1/5] lib/bitmap: add bitmap_{read,write}()

Quoted 5 lines
Quoted 3 lines

+ *
+ * for (bit = 0; bit < nbits; bit++)
+ * assign_bit(start + bit, bitmap, val & BIT(bit));

__assign_bit()

Ack

Quoted 6 lines
Quoted 2 lines

+ */

'behaves similarly' sounds like an understatement. I think, it behaves much faster because it can assign up to 64 bits at once, not mentioning the pressure on cache lines traffic.

My intent was to describe the visible behavior, of course the generated code is better, and the number of memory accesses lower.

How about the following description:

* The result of bitmap_write() is similar to @nbits calls of assign_bit(), i.e.
* bits beyond @nbits are ignored:
*
* for (bit = 0; bit < nbits; bit++)
* assign_bit(start + bit, bitmap, val & BIT(bit));

?

Quoted 4 lines

How faster - that's a good question. I'd be really pleased if you add a performance test for bitmap_write/read. Or I can do it myself later. You can find examples in the same lib/test_bitmap.c.

I can add two separate functions doing some bitmap_read and bitmap_write calls in a loop to measure their performance independently - along the lines of what you did here:
https://lore.kernel.org/lkml/ZL9X0TZb%2FQhCbEiC@yury-ThinkPad/

Quoted 4 lines

Review

Quoted 2 lines
(quoted diff) +2 -0
+     if (unlikely(!nbits))
+             return;

can you please add more empty lines to separate blocks visually?

Sure, will do.

Quoted 39 lines

Review

Quoted 13 lines
(quoted diff) +12 -0
+     mask = BITMAP_LAST_WORD_MASK(nbits);
+     value &= mask;
+     if (space >= nbits) {
+             map[index] &= ~(mask << offset);
+             map[index] |= value << offset;
+             return;
+     }
+     map[index] &= ~BITMAP_FIRST_WORD_MASK(start);
+     map[index] |= value << offset;
+     map[index + 1] &= ~BITMAP_LAST_WORD_MASK(start + nbits);
+     map[index + 1] |= (value >> space);
+}

I compiled the below fix on spark64 BE machine:

include/linux/bitmap.h +3 -3
@@ -608,7 +608,7 @@ static inline unsigned long bitmap_read(const unsigned long *map,
        if (unlikely(!nbits))
                return 0;
        if (space >= nbits)
-               return (map[index] >> offset) & GENMASK(nbits - 1, 0);
+               return (map[index] >> offset) & BITMAP_LAST_WORD_MASK(nbits);
        value_low = map[index] & BITMAP_FIRST_WORD_MASK(start);
        value_high = map[index + 1] & BITMAP_LAST_WORD_MASK(start + nbits);
        return (value_low >> offset) | (value_high << space);
@@ -661,9 +661,9 @@ static inline void bitmap_write(unsigned long *map,
                map[index] |= value << offset;
                return;
        }
-       map[index] &= ~BITMAP_FIRST_WORD_MASK(start);
+       map[index] &= BITMAP_LAST_WORD_MASK(start);
        map[index] |= value << offset;
-       map[index + 1] &= ~BITMAP_LAST_WORD_MASK(start + nbits);
+       map[index + 1] &= BITMAP_FIRST_WORD_MASK(start + nbits);
        map[index + 1] |= (value >> space);
 }
 
Quoted 3 lines

All the tests are passed just as before, and there's no any difference reported by bloat-o-meter. Can you please use non-negation versions as they are more straightforward?

I am terribly sorry for this, but this code version is completely different from what we've discussed here:
https://lore.kernel.org/lkml/CAG_fn=VrPJj6YowHNki5RGAAs8qvwZpUVN4K9qw=cf4aW7Qw9A@mail.gmail.com/

The correct version roughly looks as follows:

void bitmap_write(unsigned long *map, unsigned long value,
unsigned long start, unsigned long nbits)

{
        unsigned long offset, space, mask;
        size_t index;
        bool fit;

        if (unlikely(!nbits))
                return;

        mask = BITMAP_LAST_WORD_MASK(nbits);
        value &= mask;
        offset = start % BITS_PER_LONG;
        space = BITS_PER_LONG - offset;
        index = BIT_WORD(start);
        fit = space >= nbits;

        map[index] &= (fit ? (~(mask << offset)) :
~BITMAP_FIRST_WORD_MASK(start));
        map[index] |= value << offset;
        if (fit)
                return;

        map[index + 1] &= BITMAP_FIRST_WORD_MASK(start + nbits);
        map[index + 1] |= (value >> space);
}

Note that here replacing ~BITMAP_FIRST_WORD_MASK() with BITMAP_LAST_WORD_MASK() costs us 25 bytes (116 bytes with ~BITMAP_FIRST_WORD_MASK() vs. 141 bytes without).

#8 lore raw

Re: [PATCH v6 1/5] lib/bitmap: add bitmap_{read,write}()

Quoted 9 lines · Alexander Potapenko
Quoted 3 lines

'behaves similarly' sounds like an understatement. I think, it behaves much faster because it can assign up to 64 bits at once, not mentioning the pressure on cache lines traffic.

My intent was to describe the visible behavior, of course the generated code is better, and the number of memory accesses lower.

How about the following description:

Quoted 7 lines

* The result of bitmap_write() is similar to @nbits calls of assign_bit(), i.e.
* bits beyond @nbits are ignored:
*
* for (bit = 0; bit < nbits; bit++)
* assign_bit(start + bit, bitmap, val & BIT(bit));

?

C programmers should know the meaning of the term "as-if". Perhaps use that. Smth like "bitmap_write() behaves as-if implemented as @nbits calls of __assign_bit()".

Rasmus

#9 lore raw

Re: [PATCH v6 1/5] lib/bitmap: add bitmap_{read,write}()

Quoted 18 lines · Rasmus Villemoes
Quoted 15 lines · Alexander Potapenko
Quoted 3 lines

'behaves similarly' sounds like an understatement. I think, it behaves much faster because it can assign up to 64 bits at once, not mentioning the pressure on cache lines traffic.

My intent was to describe the visible behavior, of course the generated code is better, and the number of memory accesses lower.

How about the following description:

* The result of bitmap_write() is similar to @nbits calls of assign_bit(), i.e.
* bits beyond @nbits are ignored:
*
* for (bit = 0; bit < nbits; bit++)
* assign_bit(start + bit, bitmap, val & BIT(bit));

Quoted 5 lines
Quoted 1 line

?

C programmers should know the meaning of the term "as-if". Perhaps use that. Smth like "bitmap_write() behaves as-if implemented as @nbits calls of __assign_bit()".

Good idea, I'll go with this one.
Thank you!

#10 lore raw

PATCH v6 2/5 lib/test_bitmap: add tests for bitmap_{read,write}()

In reply to Alexander Potapenko

Add basic tests ensuring that values can be added at arbitrary positions of the bitmap, including those spanning into the adjacent unsigned longs.

---
This patch was previously called
"lib/test_bitmap: add tests for bitmap_{set,get}_value()"
(https://lore.kernel.org/lkml/[email protected]/)
and
"lib/test_bitmap: add tests for bitmap_{set,get}_value_unaligned"
(https://lore.kernel.org/lkml/[email protected]/)

v6:
- use bitmap API to initialize test bitmaps
- as requested by Yury Norov, do not check the return value of bitmap_read(..., 0)
- fix a compiler warning on 32-bit systems

v5:
- update patch title
- address Yury Norov's comments:
- rename the test cases
- factor out test_bitmap_write_helper() to test writing over

     different background patterns;

- add a test case copying a nontrivial value bit-by-bit;
- drop volatile

v4:
- Address comments by Andy Shevchenko: added Reviewed-by: and a link to the previous discussion
- Address comments by Yury Norov:
- expand the bitmap to catch more corner cases
- add code testing that bitmap_set_value() does not touch adjacent bits
- add code testing the nbits==0 case
- rename bitmap_{get,set}_value() to bitmap_{read,write}()

v3:
- switch to using bitmap_{set,get}_value()
- change the expected bit pattern in test_set_get_value(), as the test was incorrectly assuming 0 is the LSB.

---
 lib/test_bitmap.c | 118 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 118 insertions(+)
lib/test_bitmap.c +118 -0
index 187f5b2db4cf1..172ecf3292e46 100644
@@ -71,6 +71,17 @@ __check_eq_uint(const char *srcfile, unsigned int line,
 	return true;
 }
 
+static bool __init
+__check_eq_ulong(const char *srcfile, unsigned int line,
+		 const unsigned long exp_ulong, unsigned long x)
+{
+	if (exp_ulong != x) {
+		pr_err("[%s:%u] expected %lu, got %lu\n",
+			srcfile, line, exp_ulong, x);
+		return false;
+	}
+	return true;
+}
 
 static bool __init
 __check_eq_bitmap(const char *srcfile, unsigned int line,
@@ -186,6 +197,7 @@ __check_eq_str(const char *srcfile, unsigned int line,
 	})
 
 #define expect_eq_uint(...)		__expect_eq(uint, ##__VA_ARGS__)
+#define expect_eq_ulong(...)		__expect_eq(ulong, ##__VA_ARGS__)
 #define expect_eq_bitmap(...)		__expect_eq(bitmap, ##__VA_ARGS__)
 #define expect_eq_pbl(...)		__expect_eq(pbl, ##__VA_ARGS__)
 #define expect_eq_u32_array(...)	__expect_eq(u32_array, ##__VA_ARGS__)
@@ -1222,6 +1234,111 @@ static void __init test_bitmap_const_eval(void)
 	BUILD_BUG_ON(~var != ~BIT(25));
 }
 
+/*
+ * Test bitmap should be big enough to include the cases when start is not in
+ * the first word, and start+nbits lands in the following word.
+ */
+#define TEST_BIT_LEN (1000)
+
+/*
+ * Helper function to test bitmap_write() overwriting the chosen byte pattern.
+ */
+static void __init test_bitmap_write_helper(const char *pattern)
+{
+	DECLARE_BITMAP(bitmap, TEST_BIT_LEN);
+	DECLARE_BITMAP(exp_bitmap, TEST_BIT_LEN);
+	DECLARE_BITMAP(pat_bitmap, TEST_BIT_LEN);
+	unsigned long w, r, bit;
+	int i, n, nbits;
+
+	/*
+	 * Only parse the pattern once and store the result in the intermediate
+	 * bitmap.
+	 */
+	bitmap_parselist(pattern, pat_bitmap, TEST_BIT_LEN);
+
+	/*
+	 * Check that setting a single bit does not accidentally touch the
+	 * adjacent bits.
+	 */
+	for (i = 0; i < TEST_BIT_LEN; i++) {
+		bitmap_copy(bitmap, pat_bitmap, TEST_BIT_LEN);
+		bitmap_copy(exp_bitmap, pat_bitmap, TEST_BIT_LEN);
+		for (bit = 0; bit <= 1; bit++) {
+			bitmap_write(bitmap, bit, i, 1);
+			__assign_bit(i, exp_bitmap, bit);
+			expect_eq_bitmap(exp_bitmap, bitmap,
+					 TEST_BIT_LEN);
+		}
+	}
+
+	/* Ensure setting 0 bits does not change anything. */
+	bitmap_copy(bitmap, pat_bitmap, TEST_BIT_LEN);
+	bitmap_copy(exp_bitmap, pat_bitmap, TEST_BIT_LEN);
+	for (i = 0; i < TEST_BIT_LEN; i++) {
+		bitmap_write(bitmap, ~0UL, i, 0);
+		expect_eq_bitmap(exp_bitmap, bitmap, TEST_BIT_LEN);
+	}
+
+	for (nbits = BITS_PER_LONG; nbits >= 1; nbits--) {
+		w = IS_ENABLED(CONFIG_64BIT) ? 0xdeadbeefdeadbeefUL
+					     : 0xdeadbeefUL;
+		w >>= (BITS_PER_LONG - nbits);
+		for (i = 0; i <= TEST_BIT_LEN - nbits; i++) {
+			bitmap_copy(bitmap, pat_bitmap, TEST_BIT_LEN);
+			bitmap_copy(exp_bitmap, pat_bitmap, TEST_BIT_LEN);
+			for (n = 0; n < nbits; n++)
+				__assign_bit(i + n, exp_bitmap, w & BIT(n));
+			bitmap_write(bitmap, w, i, nbits);
+			expect_eq_bitmap(exp_bitmap, bitmap, TEST_BIT_LEN);
+			r = bitmap_read(bitmap, i, nbits);
+			expect_eq_ulong(r, w);
+		}
+	}
+}
+
+static void __init test_bitmap_read_write(void)
+{
+	unsigned char *pattern[3] = {"", "all:1/2", "all"};
+	DECLARE_BITMAP(bitmap, TEST_BIT_LEN);
+	unsigned long zero_bits = 0;
+	unsigned long val;
+	int i, pi;
+
+	/*
+	 * Setting/getting zero bytes should not crash the kernel.
+	 * READ_ONCE() prevents constant folding.
+	 */
+	bitmap_write(NULL, 0, 0, READ_ONCE(zero_bits));
+	/* Return value of bitmap_read() is undefined here. */
+	bitmap_read(NULL, 0, READ_ONCE(zero_bits));
+
+	/*
+	 * Ensure that bitmap_read() reads the same value that was previously
+	 * written, and two consequent values are correctly merged.
+	 * The resulting bit pattern is asymmetric to rule out possible issues
+	 * with bit numeration order.
+	 */
+	for (i = 0; i < TEST_BIT_LEN - 7; i++) {
+		bitmap_zero(bitmap, TEST_BIT_LEN);
+
+		bitmap_write(bitmap, 0b10101UL, i, 5);
+		val = bitmap_read(bitmap, i, 5);
+		expect_eq_ulong(0b10101UL, val);
+
+		bitmap_write(bitmap, 0b101UL, i + 5, 3);
+		val = bitmap_read(bitmap, i + 5, 3);
+		expect_eq_ulong(0b101UL, val);
+
+		val = bitmap_read(bitmap, i, 8);
+		expect_eq_ulong(0b10110101UL, val);
+	}
+
+	for (pi = 0; pi < ARRAY_SIZE(pattern); pi++)
+		test_bitmap_write_helper(pattern[pi]);
+}
+#undef TEST_BIT_LEN
+
 static void __init selftest(void)
 {
 	test_zero_clear();
@@ -1237,6 +1354,7 @@ static void __init selftest(void)
 	test_bitmap_cut();
 	test_bitmap_print_buf();
 	test_bitmap_const_eval();
+	test_bitmap_read_write();
 
 	test_find_nth_bit();
 	test_for_each_set_bit();
Signature · 7 lines
2.42.0.609.gbb76f46606-goog


_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
#11 lore raw

PATCH v6 3/5 arm64: mte: implement CONFIG_ARM64_MTE_COMP

In reply to Alexander Potapenko

The config implements the algorithm compressing memory tags for ARM MTE during swapping.

The algorithm is based on RLE and specifically targets buffers of tags corresponding to a single page. In the common case a buffer can be compressed into 63 bits, making it possible to store it without additional memory allocation.

---
v6:
- shuffle bits in inline handles so that they can't be confused with

    canonical pointers;

- use kmem_cache_zalloc() to allocate compressed storage
- correctly handle range size overflow
- minor documentation fixes, clarify the special cases

v5:
- make code PAGE_SIZE-agnostic, remove hardcoded constants, updated the docs
- implement debugfs interface
- Address comments by Andy Shevchenko:
- update description of mtecomp.c
- remove redundant assignments, simplify mte_tags_to_ranges()
- various code simplifications
- introduce mtecomp.h
- add :export: to Documentation/arch/arm64/mte-tag-compression.rst

v4:
- Addressed comments by Andy Shevchenko:
- expanded "MTE" to "Memory Tagging Extension" in Kconfig
- fixed kernel-doc comments, moved them to C source
- changed variables to unsigned where applicable
- some code simplifications, fewer unnecessary assignments
- added the mte_largest_idx_bits() helper
- added namespace prefixes to all functions
- added missing headers (but removed bits.h)
- Addressed comments by Yury Norov:
- removed test-only functions from mtecomp.h
- dropped the algoritm name (all functions are now prefixed with "mte")
- added more comments
- got rid of MTE_RANGES_INLINE
- renamed bitmap_{get,set}_value() to bitmap_{read,write}()
- moved the big comment explaining the algorithm to Documentation/arch/arm64/mte-tag-compression.rst, expanded it, add a link to it from Documentation/arch/arm64/index.rst
- removed hardcoded ranges from mte_alloc_size()/mte_size_to_ranges()

v3:
- Addressed comments by Andy Shevchenko:
- use bitmap_{set,get}_value() writte by Syed Nayyar Waris
- switched to unsigned long everywhere (fewer casts)
- simplified the code, removed redundant checks
- dropped ea0_compress_inline()
- added bit size constants and helpers to access the bitmap
- explicitly initialize all compressed sizes in ea0_compress_to_buf()
- initialize all handle bits

v2:
- as suggested by Yury Norov, switched from struct bitq (which is not needed anymore) to <linux/bitmap.h>
- add missing symbol exports

---
 Documentation/arch/arm64/index.rst            |   1 +
 .../arch/arm64/mte-tag-compression.rst        | 266 +++++++++
 arch/arm64/Kconfig                            |  11 +
 arch/arm64/include/asm/mtecomp.h              |  13 +
 arch/arm64/mm/Makefile                        |   1 +
 arch/arm64/mm/mtecomp.c                       | 524 ++++++++++++++++++
 arch/arm64/mm/mtecomp.h                       |  12 +
 7 files changed, 828 insertions(+)
 create mode 100644 Documentation/arch/arm64/mte-tag-compression.rst
 create mode 100644 arch/arm64/include/asm/mtecomp.h
 create mode 100644 arch/arm64/mm/mtecomp.c
 create mode 100644 arch/arm64/mm/mtecomp.h
Documentation/arch/arm64/index.rst +1 -0
index d08e924204bf1..bf6c1583233a9 100644
@@ -19,6 +19,7 @@ ARM64 Architecture
     legacy_instructions
     memory
     memory-tagging-extension
+    mte-tag-compression
     perf
     pointer-authentication
     ptdump
Documentation/arch/arm64/mte-tag-compression.rst new file +266 -0

266 lines not on this page · raw message

arch/arm64/Kconfig +11 -0
index a2511b30d0f67..d4fb3b8d11d77 100644
@@ -2093,6 +2093,17 @@ config ARM64_EPAN
 	  if the cpu does not implement the feature.
 endmenu # "ARMv8.7 architectural features"
 
+config ARM64_MTE_COMP
+	bool "Tag compression for ARM64 Memory Tagging Extension"
+	default y
+	depends on ARM64_MTE
+	help
+	  Enable tag compression support for ARM64 Memory Tagging Extension.
+
+	  Tag buffers corresponding to swapped RAM pages are compressed using
+	  RLE to conserve heap memory. In the common case compressed tags
+	  occupy 2.5x less memory.
+
 config ARM64_SVE
 	bool "ARM Scalable Vector Extension support"
 	default y
arch/arm64/include/asm/mtecomp.h new file +13 -0
new file mode 100644
index 0000000000000..71552bc429882
@@ -0,0 +1,13 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#ifndef __ASM_MTECOMP_H
+#define __ASM_MTECOMP_H
+
+#include <linux/types.h>
+
+unsigned long mte_compress(u8 *tags);
+bool mte_decompress(unsigned long handle, u8 *tags);
+void mte_release_handle(unsigned long handle);
+size_t mte_storage_size(unsigned long handle);
+
+#endif // __ASM_MTECOMP_H
arch/arm64/mm/Makefile +1 -0
index dbd1bc95967d0..46778f6dd83c2 100644
@@ -10,6 +10,7 @@ obj-$(CONFIG_TRANS_TABLE)	+= trans_pgd.o
 obj-$(CONFIG_TRANS_TABLE)	+= trans_pgd-asm.o
 obj-$(CONFIG_DEBUG_VIRTUAL)	+= physaddr.o
 obj-$(CONFIG_ARM64_MTE)		+= mteswap.o
+obj-$(CONFIG_ARM64_MTE_COMP)	+= mtecomp.o
 KASAN_SANITIZE_physaddr.o	+= n
 
 obj-$(CONFIG_KASAN)		+= kasan_init.o
arch/arm64/mm/mtecomp.c new file +524 -0

524 lines not on this page · raw message

arch/arm64/mm/mtecomp.h new file +12 -0
new file mode 100644
index 0000000000000..b94cf0384f2af
@@ -0,0 +1,12 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#ifndef ARCH_ARM64_MM_MTECOMP_H_
+#define ARCH_ARM64_MM_MTECOMP_H_
+
+/* Functions exported from mtecomp.c for test_mtecomp.c. */
+void mte_tags_to_ranges(u8 *tags, u8 *out_tags, unsigned short *out_sizes,
+			size_t *out_len);
+void mte_ranges_to_tags(u8 *r_tags, unsigned short *r_sizes, size_t r_len,
+			u8 *tags);
+
+#endif  // ARCH_ARM64_MM_TEST_MTECOMP_H_
Signature · 7 lines
2.42.0.609.gbb76f46606-goog


_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
#12 lore raw

PATCH v6 4/5 arm64: mte: add a test for MTE tags compression

In reply to Alexander Potapenko

Ensure that tag sequences containing alternating values are compressed to buffers of expected size and correctly decompressed afterwards.

---
v6:
- add test_decompress_invalid() to ensure invalid handles are ignored;
- add test_upper_bits(), which is a regression test for a case where

    an inline handle looked like an out-of-line one;
  - add test_compress_nonzero() to ensure a full nonzero tag array is
    compressed correctly;

- add test_two_ranges() to test cases when the input buffer is divided into two ranges.

v5:
- remove hardcoded constants, added test setup/teardown;
- support 16- and 64K pages;
- replace nested if-clauses with expected_size_from_ranges();
- call mte_release_handle() after tests that perform

    compression/decompression;

- address comments by Andy Shevchenko:
- fix include order;
- use mtecomp.h instead of function prototypes.

v4:
- addressed comments by Andy Shevchenko:
- expanded MTE to "Memory Tagging Extension" in Kconfig
- changed signed variables to unsigned where applicable
- added missing header dependencies

- addressed comments by Yury Norov:
- moved test-only declarations from mtecomp.h into this test
- switched to the new "mte"-prefixed function names, dropped the mentions of "EA0"
- added test_tag_to_ranges_n()

v3:
- addressed comments by Andy Shevchenko in another patch:
- switched from u64 to unsigned long
- added MODULE_IMPORT_NS(MTECOMP)
- fixed includes order

---
 arch/arm64/Kconfig           |  10 +
 arch/arm64/mm/Makefile       |   1 +
 arch/arm64/mm/test_mtecomp.c | 377 +++++++++++++++++++++++++++++++++++
 3 files changed, 388 insertions(+)
 create mode 100644 arch/arm64/mm/test_mtecomp.c
arch/arm64/Kconfig +10 -0
index d4fb3b8d11d77..ffe3bec89df82 100644
@@ -2104,6 +2104,16 @@ config ARM64_MTE_COMP
 	  RLE to conserve heap memory. In the common case compressed tags
 	  occupy 2.5x less memory.
 
+config ARM64_MTE_COMP_KUNIT_TEST
+	tristate "Test tag compression for ARM64 Memory Tagging Extension" if !KUNIT_ALL_TESTS
+	default KUNIT_ALL_TESTS
+	depends on KUNIT && ARM64_MTE_COMP
+	help
+	  Test MTE compression algorithm enabled by CONFIG_ARM64_MTE_COMP.
+
+	  Ensure that tag sequences containing alternating values are compressed
+	  to buffers of expected size and correctly decompressed afterwards.
+
 config ARM64_SVE
 	bool "ARM Scalable Vector Extension support"
 	default y
arch/arm64/mm/Makefile +1 -0
index 46778f6dd83c2..170dc62b010b9 100644
@@ -11,6 +11,7 @@ obj-$(CONFIG_TRANS_TABLE)	+= trans_pgd-asm.o
 obj-$(CONFIG_DEBUG_VIRTUAL)	+= physaddr.o
 obj-$(CONFIG_ARM64_MTE)		+= mteswap.o
 obj-$(CONFIG_ARM64_MTE_COMP)	+= mtecomp.o
+obj-$(CONFIG_ARM64_MTE_COMP_KUNIT_TEST) += test_mtecomp.o
 KASAN_SANITIZE_physaddr.o	+= n
 
 obj-$(CONFIG_KASAN)		+= kasan_init.o
arch/arm64/mm/test_mtecomp.c new file +377 -0

377 lines not on this page · raw message

Signature · 7 lines
2.42.0.609.gbb76f46606-goog


_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
#13 lore raw

PATCH v6 5/5 arm64: mte: add compression support to mteswap.c

In reply to Alexander Potapenko

Define the internal mteswap.h interface:
- _mte_alloc_and_save_tags()
- _mte_free_saved_tags()
- _mte_restore_tags()

, that encapsulates saving tags for a struct page (together with memory allocation), restoring tags, and deleting the storage allocated for them.

These functions accept opaque pointers, which may point to tag buffers of size MTE_PAGE_TAG_STORAGE, as well as smaller buffers containing compressed tags, or may have compressed tags stored directly in the pointers.

The existing code from mteswap.c operating with uncompressed tags is split away into mteswap_nocomp.c, and the newly introduced mteswap_comp.c provides compression support. The latter implementation
is picked if CONFIG_ARM64_MTE_COMP=y.

Soon after booting Android, tag compression saves ~2.5x memory previously spent by mteswap.c on tag allocations. With the growing uptime, the savings may reach 4x and even more. On a moderately loaded device with ~20% pages using tagging, the compressed tags for swapped pages only occupied 2.9Mb instead of 16.5Mb:

8 bytes: 118277 allocations, 16089 deallocations 16 bytes: 10762 allocations, 6738 deallocations 32 bytes: 10748 allocations, 6823 deallocations 64 bytes: 10510 allocations, 6644 deallocations 128 bytes: 68375 allocations, 47378 deallocations uncompressed tag storage size: 17280000 compressed tag storage size: 3125024

---
v5:
- drop a dead variable from _mte_free_saved_tags() in mteswap_comp.c
- ensure MTE compression works with arbitrary page sizes
- update patch description

v4:
- minor code simplifications suggested by Andy Shevchenko, added missing header dependencies
- changed compression API names to reflect modifications made to memcomp.h (as suggested by Yury Norov)

v3:
- Addressed comments by Andy Shevchenko in another patch:
- fixed includes order
- replaced u64 with unsigned long
- added MODULE_IMPORT_NS(MTECOMP)

---
 arch/arm64/mm/Makefile         |  5 +++
 arch/arm64/mm/mteswap.c        | 20 +++++-------
 arch/arm64/mm/mteswap.h        | 12 +++++++
 arch/arm64/mm/mteswap_comp.c   | 60 ++++++++++++++++++++++++++++++++++
 arch/arm64/mm/mteswap_nocomp.c | 38 +++++++++++++++++++++
 5 files changed, 124 insertions(+), 11 deletions(-)
 create mode 100644 arch/arm64/mm/mteswap.h
 create mode 100644 arch/arm64/mm/mteswap_comp.c
 create mode 100644 arch/arm64/mm/mteswap_nocomp.c
arch/arm64/mm/Makefile +5 -0
index 170dc62b010b9..46a798e2b67cb 100644
@@ -11,6 +11,11 @@ obj-$(CONFIG_TRANS_TABLE)	+= trans_pgd-asm.o
 obj-$(CONFIG_DEBUG_VIRTUAL)	+= physaddr.o
 obj-$(CONFIG_ARM64_MTE)		+= mteswap.o
 obj-$(CONFIG_ARM64_MTE_COMP)	+= mtecomp.o
+ifdef CONFIG_ARM64_MTE_COMP
+obj-$(CONFIG_ARM64_MTE)		+= mteswap_comp.o
+else
+obj-$(CONFIG_ARM64_MTE)		+= mteswap_nocomp.o
+endif
 obj-$(CONFIG_ARM64_MTE_COMP_KUNIT_TEST) += test_mtecomp.o
 KASAN_SANITIZE_physaddr.o	+= n
 
arch/arm64/mm/mteswap.c +9 -11
index cd508ba80ab1b..9d8f87fd191a2 100644
@@ -5,8 +5,11 @@
 #include <linux/slab.h>
 #include <linux/swap.h>
 #include <linux/swapops.h>
+
 #include <asm/mte.h>
 
+#include "mteswap.h"
+
 static DEFINE_XARRAY(mte_pages);
 
 void *mte_allocate_tag_storage(void)
@@ -27,20 +30,18 @@ int mte_save_tags(struct page *page)
 	if (!page_mte_tagged(page))
 		return 0;
 
-	tag_storage = mte_allocate_tag_storage();
+	tag_storage = _mte_alloc_and_save_tags(page);
 	if (!tag_storage)
 		return -ENOMEM;
 
-	mte_save_page_tags(page_address(page), tag_storage);
-
 	/* page_private contains the swap entry.val set in do_swap_page */
 	ret = xa_store(&mte_pages, page_private(page), tag_storage, GFP_KERNEL);
 	if (WARN(xa_is_err(ret), "Failed to store MTE tags")) {
-		mte_free_tag_storage(tag_storage);
+		_mte_free_saved_tags(tag_storage);
 		return xa_err(ret);
 	} else if (ret) {
 		/* Entry is being replaced, free the old entry */
-		mte_free_tag_storage(ret);
+		_mte_free_saved_tags(ret);
 	}
 
 	return 0;
@@ -53,10 +54,7 @@ void mte_restore_tags(swp_entry_t entry, struct page *page)
 	if (!tags)
 		return;
 
-	if (try_page_mte_tagging(page)) {
-		mte_restore_page_tags(page_address(page), tags);
-		set_page_mte_tagged(page);
-	}
+	_mte_restore_tags(tags, page);
 }
 
 void mte_invalidate_tags(int type, pgoff_t offset)
@@ -64,7 +62,7 @@ void mte_invalidate_tags(int type, pgoff_t offset)
 	swp_entry_t entry = swp_entry(type, offset);
 	void *tags = xa_erase(&mte_pages, entry.val);
 
-	mte_free_tag_storage(tags);
+	_mte_free_saved_tags(tags);
 }
 
 void mte_invalidate_tags_area(int type)
@@ -78,7 +76,7 @@ void mte_invalidate_tags_area(int type)
 	xa_lock(&mte_pages);
 	xas_for_each(&xa_state, tags, last_entry.val - 1) {
 		__xa_erase(&mte_pages, xa_state.xa_index);
-		mte_free_tag_storage(tags);
+		_mte_free_saved_tags(tags);
 	}
 	xa_unlock(&mte_pages);
 }
arch/arm64/mm/mteswap.h new file +12 -0
new file mode 100644
index 0000000000000..4c576b76785d1
@@ -0,0 +1,12 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#ifndef ARCH_ARM64_MM_MTESWAP_H_
+#define ARCH_ARM64_MM_MTESWAP_H_
+
+struct page;
+
+void *_mte_alloc_and_save_tags(struct page *page);
+void _mte_free_saved_tags(void *tags);
+void _mte_restore_tags(void *tags, struct page *page);
+
+#endif // ARCH_ARM64_MM_MTESWAP_H_
arch/arm64/mm/mteswap_comp.c new file +60 -0

60 lines not on this page · raw message

arch/arm64/mm/mteswap_nocomp.c new file +38 -0
new file mode 100644
index 0000000000000..1e665a4b5f940
@@ -0,0 +1,38 @@
+// SPDX-License-Identifier: GPL-2.0
+
+/* MTE tag storage management without compression support. */
+
+#include <linux/pagemap.h>
+#include <linux/slab.h>
+#include <linux/swap.h>
+#include <linux/swapops.h>
+#include <linux/xarray.h>
+
+#include <asm/mte.h>
+
+#include "mteswap.h"
+
+void *_mte_alloc_and_save_tags(struct page *page)
+{
+	void *storage;
+
+	storage = mte_allocate_tag_storage();
+	if (!storage)
+		return NULL;
+
+	mte_save_page_tags(page_address(page), storage);
+	return storage;
+}
+
+void _mte_free_saved_tags(void *storage)
+{
+	mte_free_tag_storage(storage);
+}
+
+void _mte_restore_tags(void *tags, struct page *page)
+{
+	if (!try_page_mte_tagging(page))
+		return;
+	mte_restore_page_tags(page_address(page), tags);
+	set_page_mte_tagged(page);
+}
Signature · 7 lines
2.42.0.609.gbb76f46606-goog


_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel