> On January 8, 2016 at 9:50 AM Wolfgang Bumiller <w.bumil...@proxmox.com>
> wrote:
>
>
>
> > On January 7, 2016 at 8:20 PM Serge Hallyn <serge.hal...@ubuntu.com> wrote:
> > Quoting Wolfgang Bumiller (w.bumil...@proxmox.com):
> > > > On January 7, 2016 at 7:42 PM Serge Hallyn <serge.hal...@ubuntu.com>
> > > > wrote:
> > > > Quoting Wolfgang Bumiller (w.bumil...@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.hal...@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.hal...@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.bumil...@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:
From 0d5dcaa2ad1e39b8f5c65eb5182ef7acd409c812 Mon Sep 17 00:00:00 2001
From: Wolfgang Bumiller <w.bumil...@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.bumil...@proxmox.com>
---
cgfs.c | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/cgfs.c b/cgfs.c
index 1f31ed1..f667456 100644
--- a/cgfs.c
+++ b/cgfs.c
@@ -77,14 +77,11 @@ 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 {
+ } else if (newbatches > oldbatches) {
char *tmp;
do {
tmp = realloc(*mem, newbatches * BATCH_SIZE);
--
2.1.4
_______________________________________________
lxc-devel mailing list
lxc-devel@lists.linuxcontainers.org
http://lists.linuxcontainers.org/listinfo/lxc-devel
