[lxc-devel] [PATCH lxcfs 4/5] cgfs: fix dorealloc's batch allocation

[Serge Hallyn]

Quoting Wolfgang Bumiller (w.bumiller at proxmox.com):
> 
> > On January 8, 2016 at 11:23 AM Wolfgang Bumiller <w.bumiller at proxmox.com>
> > wrote:
> > 
> > 
> > 
> > > On January 8, 2016 at 9:50 AM Wolfgang Bumiller <w.bumiller at proxmox.com>
> > > wrote:
> > > 
> > > 
> > > 
> > > > On January 7, 2016 at 8:20 PM Serge Hallyn <serge.hallyn at ubuntu.com>
> > > > wrote:
> > > > Quoting Wolfgang Bumiller (w.bumiller at proxmox.com):
> > > > > > On January 7, 2016 at 7:42 PM Serge Hallyn <serge.hallyn at ubuntu.com>
> > > > > > wrote:
> > > > > > Quoting Wolfgang Bumiller (w.bumiller at proxmox.com):
> > > > > > > -	if (newlen % BATCH_SIZE <= oldlen % BATCH_SIZE)
> > > > > > > +	if (newlen <= oldlen)
> > > > > > 
> > > > > > This will result in extra reallocs (though innocuous).
> > > > > > Is there any reason not to just do
> > > > > > 
> > > > > > 	if (newlen / BATCH_SIZE <= oldlen / BATCH_SIZE)
> > > > > 
> > > > > Ah yes I missed that the old length is a string length not the
> > > > > capacity, so yes, the division makes sense. Just not the modulus.
> > > > > 
> > > > 
> > > > Ok, thanks - per your patch I think the following makes a nice cleanup:
> > > >
> > > > From a72193ff0dba319c3039372bc038708a75d0582f Mon Sep 17 00:00:00 2001
> > > > From: Serge Hallyn <serge.hallyn at ubuntu.com>
> > > > Date: Thu, 7 Jan 2016 11:17:17 -0800
> > > > Subject: [PATCH 1/1] dorealloc: avoid extra reallocs
> > > > 
> > > > The original check was very wrong, using % instead of /.  However
> > > > the length we track is the actual used length, not the allocated
> > > > length, which is always (len / BATCH_SIZE) + 1.  We don't want
> > > > to realloc when newlen is between oldlen and (oldlen / BATCH_SIZE) + 1)
> > > > 
> > > > Signed-off-by: Serge Hallyn <serge.hallyn at ubuntu.com>
> > > 
> > > Yes this seems fine and easier to read. Although the commit message
> > > mixes length and batch count again ;-).
> > > 
> > > Do you care about a Reviewed-by line? If so
> > > 
> > > Reviewed-by: Wolfgang Bumiller <w.bumiller at proxmox.com>
> > >
> > > > ---
> > > >  cgfs.c | 12 +++++++-----
> > > >  1 file changed, 7 insertions(+), 5 deletions(-)
> > > > 
> > > > diff --git a/cgfs.c b/cgfs.c
> > > > index ec0f630..f37bc2b 100644
> > > > --- a/cgfs.c
> > > > +++ b/cgfs.c
> > > > @@ -74,18 +74,20 @@ static inline void drop_trailing_newlines(char *s)
> > > >  #define BATCH_SIZE 50
> > > >  static void dorealloc(char **mem, size_t oldlen, size_t newlen)
> > > >  {
> > > > -	int batches;
> > > > -	if (newlen <= oldlen)
> > > > +	int newbatches = (newlen / BATCH_SIZE) + 1;
> > > > +	int oldbatches = (olden / BATCH_SIZE) + 1;
> > > > +
> > > > +	if (newbatches <= oldbatches)
> > 
> > Ah actually... if oldlen==0 (and newlen is < 50, as in most of the time
> > with the first line) then newbatches == oldbatches so the <= is true
> > and it won't do the first malloc().
> > 
> > realloc() also deals with NULL as first parameter so perhaps a
> > single conditional could do (unless that part of realloc() is not
> > standard C behavior or different on android or something?).
> > 
> > Like so:
> 
> Sorry, it was supposed to be this one (forgot to add the file to the
> git commit --amend line before formatting the patch):
> 
> From 24e98d74ca279ed2dc8e5a025add5a00737ba952 Mon Sep 17 00:00:00 2001
> From: Wolfgang Bumiller <w.bumiller at proxmox.com>
> Date: Fri, 8 Jan 2016 11:09:57 +0100
> Subject: [PATCH lxcfs] cgfs: make dorealloc allocate the first batch, too
> 
> With a short first line the case can be
>  *mem = NULL
>  oldlen = 0
>  newlen = 5 (anything < 50)
> making newbatches == oldbatches == 1 causing the
>  (newbatches <= oldbatches)
> condition to be true.
> 
> Let realloc() handle *mem==NULL and use
> (!*mem || newbatches > oldbatches) as the only condition.
> 
> Signed-off-by: Wolfgang Bumiller <w.bumiller at proxmox.com>

Acked-by: Serge E. Hallyn <serge.hallyn at ubuntu.com>

Looks good to me, thanks.

> ---
>  cgfs.c  | 9 +--------
>  lxcfs.c | 2 +-
>  2 files changed, 2 insertions(+), 9 deletions(-)
> 
> diff --git a/cgfs.c b/cgfs.c
> index 1f31ed1..e6330e3 100644
> --- a/cgfs.c
> +++ b/cgfs.c
> @@ -77,14 +77,7 @@ static void dorealloc(char **mem, size_t oldlen, size_t
> newlen)
>  	int newbatches = (newlen / BATCH_SIZE) + 1;
>  	int oldbatches = (oldlen / BATCH_SIZE) + 1;
>  
> -	if (newbatches <= oldbatches)
> -		return;
> -
> -	if (!*mem) {
> -		do {
> -			*mem = malloc(newbatches * BATCH_SIZE);
> -		} while (!*mem);
> -	} else {
> +	if (!*mem || newbatches > oldbatches) {
>  		char *tmp;
>  		do {
>  			tmp = realloc(*mem, newbatches * BATCH_SIZE);
> -- 
> 2.1.4
>