From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:60659) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Vq8X9-00058W-Lh for qemu-devel@nongnu.org; Mon, 09 Dec 2013 16:38:37 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Vq8X2-0001Ox-6B for qemu-devel@nongnu.org; Mon, 09 Dec 2013 16:38:31 -0500 Received: from mx1.redhat.com ([209.132.183.28]:2667) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Vq8X1-0001OU-Kw for qemu-devel@nongnu.org; Mon, 09 Dec 2013 16:38:24 -0500 Date: Mon, 9 Dec 2013 19:37:24 -0200 From: Marcelo Tosatti Message-ID: <20131209213724.GA4453@amt.cnet> References: <1386345276-9803-1-git-send-email-pbonzini@redhat.com> <1386345276-9803-2-git-send-email-pbonzini@redhat.com> <52A6146B.6070008@weilnetz.de> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <52A6146B.6070008@weilnetz.de> Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PULL 2/3] qemu: mempath: prefault pages manually (v4) List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Weil Cc: Paolo Bonzini , qemu-devel@nongnu.org On Mon, Dec 09, 2013 at 08:05:15PM +0100, Stefan Weil wrote: > Hi, >=20 > this patch was recently committed to git master. >=20 > It now causes problems with gcc-4.7 -Werror=3Dclobbered. See more comme= nts > below. >=20 > Am 06.12.2013 16:54, schrieb Paolo Bonzini: > > From: Marcelo Tosatti > > > > v4: s/fail/failed/ (Peter Maydell) > > > > Signed-off-by: Paolo Bonzini > > Signed-off-by: Marcelo Tosatti > > --- > > 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; > > } > > =20 > > +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; > > =20 > > hpagesize =3D gethugepagesize(path); > > @@ -963,21 +967,52 @@ static void *file_ram_alloc(RAMBlock *block, > > if (ftruncate(fd, memory)) > > perror("ftruncate"); > > =20 > > -#ifdef MAP_POPULATE > > - /* NB: MAP_POPULATE won't exhaustively alloc all phys pages in t= he case > > - * MAP_PRIVATE is requested. For mem_prealloc we mmap as MAP_SH= ARED > > - * to sidestep this quirk. > > - */ > > - flags =3D mem_prealloc ? MAP_POPULATE | MAP_SHARED : MAP_PRIVATE= ; > > - area =3D mmap(0, memory, PROT_READ | PROT_WRITE, flags, fd, 0); > > -#else > > area =3D mmap(0, memory, PROT_READ | PROT_WRITE, MAP_PRIVATE, fd= , 0); > > -#endif > > if (area =3D=3D 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 =3D &sigbus_handler; > > + act.sa_flags =3D 0; > > + > > + ret =3D 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); > > + >=20 >=20 > 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). >=20 > > + if (sigsetjmp(sigjump, 1)) { > > + fprintf(stderr, "file_ram_alloc: failed to preallocate p= ages\n"); > > + exit(1); > > + } > > + > > + /* MAP_POPULATE silently ignores failures */ >=20 > 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 =3D 0; i < (memory/hpagesize)-1; i++) { > > + memset(area + (hpagesize*i), 0, 1); > > + } > > + > > + ret =3D sigaction(SIGBUS, &oldact, NULL); > > + if (ret) { > > + perror("file_ram_alloc: failed to reinstall signal handl= er"); > > + exit(1); > > + } > > + > > + pthread_sigmask(SIG_SETMASK, &oldset, NULL); > > + } > > + > > block->fd =3D fd; > > return area; > > } >=20 >=20 > This is the warning shown by newer gcc versions: >=20 > qemu/exec.c:921:11: error: > variable =E2=80=98area=E2=80=99 might be clobbered by =E2=80=98longjmp= =E2=80=99 or =E2=80=98vfork=E2=80=99 > [-Werror=3Dclobbered] >=20 > I want to get support for -Wextra, and -Wclobbered is part of -Wextra. >=20 > 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 futur= e. >=20 > Declaring the local variable 'area' to be volatile also fixes the warni= ng. >=20 > Any opinion how this problem should be solved? >=20 > Thanks, > Stefan The warning is spurious, obviously. Don't see a problem with 'volatile' f= or area pointer.