Re: [Qemu-devel] [PULL 2/3] qemu: mempath: prefault pages manually (v4)

[Marcelo Tosatti]

On Mon, Dec 09, 2013 at 08:05:15PM +0100, Stefan Weil wrote:
> Hi,
> 
> this patch was recently committed to git master.
> 
> It now causes problems with gcc-4.7 -Werror=clobbered. See more comments
> below.
> 
> Am 06.12.2013 16:54, schrieb Paolo Bonzini:
> > From: Marcelo Tosatti <address@hidden>
> >
> > v4: s/fail/failed/  (Peter Maydell)
> >
> > Signed-off-by: Paolo Bonzini <address@hidden>
> > Signed-off-by: Marcelo Tosatti <address@hidden>
> > ---
> >  exec.c          |   59 
> > +++++++++++++++++++++++++++++++++++++++++++-----------
> >  qemu-options.hx |    2 -
> >  vl.c            |    4 ---
> >  3 files changed, 47 insertions(+), 18 deletions(-)
> >
> > diff --git a/exec.c b/exec.c
> > index 95c4356..f4b9ef2 100644
> > --- a/exec.c
> > +++ b/exec.c
> > @@ -904,6 +904,13 @@ static long gethugepagesize(const char *path)
> >      return fs.f_bsize;
> >  }
> >  
> > +static sigjmp_buf sigjump;
> > +
> > +static void sigbus_handler(int signal)
> > +{
> > +    siglongjmp(sigjump, 1);
> > +}
> > +
> >  static void *file_ram_alloc(RAMBlock *block,
> >                              ram_addr_t memory,
> >                              const char *path)
> > @@ -913,9 +920,6 @@ static void *file_ram_alloc(RAMBlock *block,
> >      char *c;
> >      void *area;
> >      int fd;
> > -#ifdef MAP_POPULATE
> > -    int flags;
> > -#endif
> >      unsigned long hpagesize;
> >  
> >      hpagesize = gethugepagesize(path);
> > @@ -963,21 +967,52 @@ static void *file_ram_alloc(RAMBlock *block,
> >      if (ftruncate(fd, memory))
> >          perror("ftruncate");
> >  
> > -#ifdef MAP_POPULATE
> > -    /* NB: MAP_POPULATE won't exhaustively alloc all phys pages in the case
> > -     * MAP_PRIVATE is requested.  For mem_prealloc we mmap as MAP_SHARED
> > -     * to sidestep this quirk.
> > -     */
> > -    flags = mem_prealloc ? MAP_POPULATE | MAP_SHARED : MAP_PRIVATE;
> > -    area = mmap(0, memory, PROT_READ | PROT_WRITE, flags, fd, 0);
> > -#else
> >      area = mmap(0, memory, PROT_READ | PROT_WRITE, MAP_PRIVATE, fd, 0);
> > -#endif
> >      if (area == MAP_FAILED) {
> >          perror("file_ram_alloc: can't mmap RAM pages");
> >          close(fd);
> >          return (NULL);
> >      }
> > +
> > +    if (mem_prealloc) {
> > +        int ret, i;
> > +        struct sigaction act, oldact;
> > +        sigset_t set, oldset;
> > +
> > +        memset(&act, 0, sizeof(act));
> > +        act.sa_handler = &sigbus_handler;
> > +        act.sa_flags = 0;
> > +
> > +        ret = sigaction(SIGBUS, &act, &oldact);
> > +        if (ret) {
> > +            perror("file_ram_alloc: failed to install signal handler");
> > +            exit(1);
> > +        }
> > +
> > +        /* unblock SIGBUS */
> > +        sigemptyset(&set);
> > +        sigaddset(&set, SIGBUS);
> > +        pthread_sigmask(SIG_UNBLOCK, &set, &oldset);
> > +
> 
> 
> The sigsetjmp instruction causes the compiler to assume that 'area'
> might be clobbered (sigsetjmp is declared to return twice, and gcc does
> not now anything about its semantics).
> 
> > +        if (sigsetjmp(sigjump, 1)) {
> > +            fprintf(stderr, "file_ram_alloc: failed to preallocate 
> > pages\n");
> > +            exit(1);
> > +        }
> > +
> > +        /* MAP_POPULATE silently ignores failures */
> 
> Yes, but why this comment in this context? Here we don't use
> MAP_POPULATE and can get SIGBUS (which is not silent). Or am I wrong?

It explains why MAP_POPULATE cannot be used.

> > +        for (i = 0; i < (memory/hpagesize)-1; i++) {
> > +            memset(area + (hpagesize*i), 0, 1);
> > +        }
> > +
> > +        ret = sigaction(SIGBUS, &oldact, NULL);
> > +        if (ret) {
> > +            perror("file_ram_alloc: failed to reinstall signal handler");
> > +            exit(1);
> > +        }
> > +
> > +        pthread_sigmask(SIG_SETMASK, &oldset, NULL);
> > +    }
> > +
> >      block->fd = fd;
> >      return area;
> >  }
> 
> 
> This is the warning shown by newer gcc versions:
> 
> qemu/exec.c:921:11: error:
>  variable ‘area’ might be clobbered by ‘longjmp’ or ‘vfork’
> [-Werror=clobbered]
> 
> I want to get support for -Wextra, and -Wclobbered is part of -Wextra.
> 
> Of course we could disable -Wclobbered and still use the other
> advantages of -Wextra,
> but this might hide real problems with clobbered variables in the future.
> 
> Declaring the local variable 'area' to be volatile also fixes the warning.
> 
> Any opinion how this problem should be solved?
> 
> Thanks,
> Stefan

The warning is spurious, obviously. Don't see a problem with 'volatile' for 
area pointer.