Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 12 Sep 2022 15:07:49 +0900
From:   Sergey Senozhatsky <senozhatsky@chromium.org>
To:     Brian Geffon <bgeffon@google.com>, Minchan Kim <minchan@kernel.org>
Cc:     Andrew Morton <akpm@linux-foundation.org>,
        Nitin Gupta <ngupta@vflare.org>, linux-kernel@vger.kernel.org,
        Suleiman Souhlal <suleiman@google.com>,
        Rom Lemarchand <romlem@google.com>, linux-mm@kvack.org,
        Sergey Senozhatsky <senozhatsky@chromium.org>
Subject: Re: [RESEND RFC] zram: Allow rw_page when page isn't written back.
Message-ID: <Yx7MtWdQ/WVE+EkG@google.com>
References: <20220908125037.1119114-1-bgeffon@google.com>
 <Yxr5oNaCwjn8cdFF@google.com>
 <Yx63e2lxNmEB3UhE@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <Yx63e2lxNmEB3UhE@google.com>
Precedence: bulk

On (22/09/12 13:37), Sergey Senozhatsky wrote:
> On (22/09/09 17:30), Sergey Senozhatsky wrote:
> > > +++ b/drivers/block/zram/zram_drv.h
> > > @@ -50,6 +50,7 @@ enum zram_pageflags {
> > >  	ZRAM_UNDER_WB,	/* page is under writeback */
> > >  	ZRAM_HUGE,	/* Incompressible page */
> > >  	ZRAM_IDLE,	/* not accessed page since last idle marking */
> > > +	ZRAM_NO_WB,	/* Do not allow page to be written back */
> > >  
> > >  	__NR_ZRAM_PAGEFLAGS,
> > >  };
> > 
> > Unrelated but somehow related.
> > 
> > I wonder if it's time for us to introduce a dedicated, say u16,
> > flags member to struct zram_table_entry. Unless my calculations
> > are extremely wrong, we are about to run out of spare bits in
> > zram_table_entry::flags on 32-bit systems.
> 
> Looking at it more - I wonder why do we define ZRAM_FLAG_SHIFT
> as 24? This is far more than maximum zram object size. Our max
> size needs PAGE_SHIFT bits (which is normally 12 bits, can be up
> to 16 (for 64k arm64 pages)). So we probably can start defining
> ZRAM_FLAG_SHIFT as (PAGE_SHIFT + 1).
> 
> Or am I missing something?

So I think what happened was that flags used to be a u8 member of
zram_table_entry, commit d2d5e762c8990 merged u16 size (which was
too large for object size) and u8 flags and just kept 8 bits for
flags (and somehow assumed 32-bit long? 32 - 8)

We definitely can store size in PAGE_SHIFT bits and have some
extra spare bits for zram pageflags. Would have been even nicer
if we could change type of flags from unsigned long to unsigned
int, but bit_lock requires "volatile unsigned long *" data type,
so because of bit_lock our zram_table_entriy is 4 extra bytes in
size. Which probably isn't too bad; having extra pageflag bits
on 32-bit systems is good news.