zram_table_entry::flags stores object size in the lower bits and
zram pageflags in the upper bits. However, for some reason, we
use 24 lower bits, while maximum zram object size is PAGE_SIZE,
which requires PAGE_SHIFT bits (up to 16 on arm64). This wastes
24 - PAGE_SHIFT bits that we can use for additional zram pageflags
instead.
Signed-off-by: Sergey Senozhatsky <[email protected]>
---
drivers/block/zram/zram_drv.h | 13 +++++--------
1 file changed, 5 insertions(+), 8 deletions(-)
diff --git a/drivers/block/zram/zram_drv.h b/drivers/block/zram/zram_drv.h
index b4eecef2a11f..138be8326866 100644
--- a/drivers/block/zram/zram_drv.h
+++ b/drivers/block/zram/zram_drv.h
@@ -30,16 +30,13 @@
/*
- * The lower ZRAM_FLAG_SHIFT bits of table.flags is for
- * object size (excluding header), the higher bits is for
- * zram_pageflags.
- *
- * zram is mainly used for memory efficiency so we want to keep memory
- * footprint small so we can squeeze size and flags into a field.
+ * ZRAM is mainly used for memory efficiency so we want to keep memory
+ * footprint small and thus squeeze size and flags into a flags member.
* The lower ZRAM_FLAG_SHIFT bits is for object size (excluding header),
- * the higher bits is for zram_pageflags.
+ * which cannot be larger than PAGE_SIZE (requiring PAGE_SHIFT bits),
+ * the higher bits are for zram_pageflags.
*/
-#define ZRAM_FLAG_SHIFT 24
+#define ZRAM_FLAG_SHIFT (PAGE_SHIFT + 1)
/* Flags for zram pages (table[page_no].flags) */
enum zram_pageflags {
--
2.37.2.789.g6183377224-goog
On Mon, Sep 12, 2022 at 1:38 AM Sergey Senozhatsky
<[email protected]> wrote:
>
> zram_table_entry::flags stores object size in the lower bits and
> zram pageflags in the upper bits. However, for some reason, we
> use 24 lower bits, while maximum zram object size is PAGE_SIZE,
> which requires PAGE_SHIFT bits (up to 16 on arm64). This wastes
> 24 - PAGE_SHIFT bits that we can use for additional zram pageflags
> instead.
>
> Signed-off-by: Sergey Senozhatsky <[email protected]>
> ---
> drivers/block/zram/zram_drv.h | 13 +++++--------
> 1 file changed, 5 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/block/zram/zram_drv.h b/drivers/block/zram/zram_drv.h
> index b4eecef2a11f..138be8326866 100644
> --- a/drivers/block/zram/zram_drv.h
> +++ b/drivers/block/zram/zram_drv.h
> @@ -30,16 +30,13 @@
>
>
> /*
> - * The lower ZRAM_FLAG_SHIFT bits of table.flags is for
> - * object size (excluding header), the higher bits is for
> - * zram_pageflags.
> - *
> - * zram is mainly used for memory efficiency so we want to keep memory
> - * footprint small so we can squeeze size and flags into a field.
> + * ZRAM is mainly used for memory efficiency so we want to keep memory
> + * footprint small and thus squeeze size and flags into a flags member.
> * The lower ZRAM_FLAG_SHIFT bits is for object size (excluding header),
> - * the higher bits is for zram_pageflags.
> + * which cannot be larger than PAGE_SIZE (requiring PAGE_SHIFT bits),
> + * the higher bits are for zram_pageflags.
> */
> -#define ZRAM_FLAG_SHIFT 24
> +#define ZRAM_FLAG_SHIFT (PAGE_SHIFT + 1)
Why not just hard code 16 with an explanation that it cannot be
increased further using the analysis you did in the other thread? It's
going to be tricky to reason about how many free flag bits actually
remain with PAGE_SHIFT across all architectures, especially given we
have no architecture specific flags.
>
> /* Flags for zram pages (table[page_no].flags) */
> enum zram_pageflags {
> --
> 2.37.2.789.g6183377224-goog
>
On (22/09/12 10:20), Brian Geffon wrote:
> > /*
> > - * The lower ZRAM_FLAG_SHIFT bits of table.flags is for
> > - * object size (excluding header), the higher bits is for
> > - * zram_pageflags.
> > - *
> > - * zram is mainly used for memory efficiency so we want to keep memory
> > - * footprint small so we can squeeze size and flags into a field.
> > + * ZRAM is mainly used for memory efficiency so we want to keep memory
> > + * footprint small and thus squeeze size and flags into a flags member.
> > * The lower ZRAM_FLAG_SHIFT bits is for object size (excluding header),
> > - * the higher bits is for zram_pageflags.
> > + * which cannot be larger than PAGE_SIZE (requiring PAGE_SHIFT bits),
> > + * the higher bits are for zram_pageflags.
> > */
> > -#define ZRAM_FLAG_SHIFT 24
> > +#define ZRAM_FLAG_SHIFT (PAGE_SHIFT + 1)
>
> Why not just hard code 16 with an explanation that it cannot be
> increased further using the analysis you did in the other thread? It's
> going to be tricky to reason about how many free flag bits actually
> remain with PAGE_SHIFT across all architectures, especially given we
> have no architecture specific flags.
Well, zram should not make any assumptions on arch code. How do
we know that PAGE_SHIFT 16 is the max value we will ever have?
Some arch can come around someday and use PAGE_SHIFT say, 18,
and we won't be aware of it (using hardcoded value of 16) until
someone hits a really hard to debug problem in zram.
On (22/09/12 23:37), Sergey Senozhatsky wrote:
> > > -#define ZRAM_FLAG_SHIFT 24
> > > +#define ZRAM_FLAG_SHIFT (PAGE_SHIFT + 1)
> >
> > Why not just hard code 16 with an explanation that it cannot be
> > increased further using the analysis you did in the other thread? It's
> > going to be tricky to reason about how many free flag bits actually
> > remain with PAGE_SHIFT across all architectures, especially given we
> > have no architecture specific flags.
>
> Well, zram should not make any assumptions on arch code. How do
> we know that PAGE_SHIFT 16 is the max value we will ever have?
> Some arch can come around someday and use PAGE_SHIFT say, 18,
> and we won't be aware of it (using hardcoded value of 16) until
> someone hits a really hard to debug problem in zram.
And I'd probably also add something like this, to keep us alert should
we run out of bits in the future:
---
diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
index f3948abce2f7..07913bcdb5c2 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -2449,6 +2449,8 @@ static int __init zram_init(void)
{
int ret;
+ BUILD_BUG_ON(__NR_ZRAM_PAGEFLAGS > BITS_PER_LONG);
+
ret = cpuhp_setup_state_multi(CPUHP_ZCOMP_PREPARE, "block/zram:prepare",
zcomp_cpu_up_prepare, zcomp_cpu_dead);
if (ret < 0)
On Mon, Sep 12, 2022 at 10:51 AM Sergey Senozhatsky
<[email protected]> wrote:
>
> On (22/09/12 23:37), Sergey Senozhatsky wrote:
> > > > -#define ZRAM_FLAG_SHIFT 24
> > > > +#define ZRAM_FLAG_SHIFT (PAGE_SHIFT + 1)
> > >
> > > Why not just hard code 16 with an explanation that it cannot be
> > > increased further using the analysis you did in the other thread? It's
> > > going to be tricky to reason about how many free flag bits actually
> > > remain with PAGE_SHIFT across all architectures, especially given we
> > > have no architecture specific flags.
> >
> > Well, zram should not make any assumptions on arch code. How do
> > we know that PAGE_SHIFT 16 is the max value we will ever have?
> > Some arch can come around someday and use PAGE_SHIFT say, 18,
> > and we won't be aware of it (using hardcoded value of 16) until
> > someone hits a really hard to debug problem in zram.
>
> And I'd probably also add something like this, to keep us alert should
> we run out of bits in the future:
>
> ---
>
> diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
> index f3948abce2f7..07913bcdb5c2 100644
> --- a/drivers/block/zram/zram_drv.c
> +++ b/drivers/block/zram/zram_drv.c
> @@ -2449,6 +2449,8 @@ static int __init zram_init(void)
> {
> int ret;
>
> + BUILD_BUG_ON(__NR_ZRAM_PAGEFLAGS > BITS_PER_LONG);
> +
Thanks Sergey, yes, with the BUILD_BUG_ON I think using PAGE_SHIFT is
fine, my concern was primarily that a flag could overwrite a bit of
the size field, a BUILD_BUG_ON addresses that.
> ret = cpuhp_setup_state_multi(CPUHP_ZCOMP_PREPARE, "block/zram:prepare",
> zcomp_cpu_up_prepare, zcomp_cpu_dead);
> if (ret < 0)
Thanks,
Brian