Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.34 as permitted sender) client-ip=23.128.96.34;
Date:   Tue, 19 Sep 2023 09:18:31 -0400
From:   Brian Foster <bfoster@redhat.com>
To:     Christophe JAILLET <christophe.jaillet@wanadoo.fr>
Cc:     Kent Overstreet <kent.overstreet@linux.dev>,
        linux-kernel@vger.kernel.org, kernel-janitors@vger.kernel.org,
        linux-bcachefs@vger.kernel.org
Subject: Re: [PATCH] bcachefs: Avoid a potential memory over-allocation in
 bch2_printbuf_make_room()
Message-ID: <ZQmfpzxX+qjLtJjm@bfoster>
References: <2e6a82a83d0ddd9ce7f36ea889dd7ffc30f5fbc9.1694853900.git.christophe.jaillet@wanadoo.fr>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <2e6a82a83d0ddd9ce7f36ea889dd7ffc30f5fbc9.1694853900.git.christophe.jaillet@wanadoo.fr>
Precedence: bulk

On Sat, Sep 16, 2023 at 10:45:23AM +0200, Christophe JAILLET wrote:
> kmalloc() and co. don't always allocate a power of 2 number of bytes.
> There are some special handling for 64<n<=96 and 128<n<=192 cases.
> 

It's not immediately clear to me what you mean by "special handling."
Taking a quick look at slabinfo, it looks like what you mean is that
slab rounding is a bit more granular than power of two, particularly in
these ranges. Is that right? If so, JFYI it would be helpful to describe
that more explicitly in the commit log.

> So trust kmalloc() algorithm instead of forcing a power of 2 allocation.
> This can saves a few bytes of memory and still make use of all the
> memory allocated.
> 
> On the other side, it may require an additional realloc() in some cases.
> 

Well, I feel like this isn't the only place I've seen the power of two
buffer size realloc algorithm thing, but in thinking about it this seems
fairly harmless and reasonable for printbufs. FWIW:

Reviewed-by: Brian Foster <bfoster@redhat.com>

> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
>  fs/bcachefs/printbuf.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/bcachefs/printbuf.c b/fs/bcachefs/printbuf.c
> index 77bee9060bfe..34527407e950 100644
> --- a/fs/bcachefs/printbuf.c
> +++ b/fs/bcachefs/printbuf.c
> @@ -28,7 +28,7 @@ int bch2_printbuf_make_room(struct printbuf *out, unsigned extra)
>  	if (out->pos + extra < out->size)
>  		return 0;
>  
> -	new_size = roundup_pow_of_two(out->size + extra);
> +	new_size = kmalloc_size_roundup(out->size + extra);
>  
>  	/*
>  	 * Note: output buffer must be freeable with kfree(), it's not required
> -- 
> 2.34.1
>