| Message ID | 20220912053755.156704-1-senozhatsky@chromium.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | zram: do not waste zram_table_entry flags bits | expand |
On Mon, Sep 12, 2022 at 1:38 AM Sergey Senozhatsky <senozhatsky@chromium.org> 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 <senozhatsky@chromium.org> > --- > 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 <senozhatsky@chromium.org> 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
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 {
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 <senozhatsky@chromium.org> --- drivers/block/zram/zram_drv.h | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-)