Re: [PULL v2 11/14] os: add an ability to lock memory on_fault

["Daniel P. Berrangé" <berrange@redhat.com>]

On Wed, Feb 12, 2025 at 04:33:51PM -0500, Peter Xu wrote:
> On Wed, Feb 12, 2025 at 06:03:30PM +0000, Daniel P. Berrangé wrote:
> > On Wed, Feb 12, 2025 at 12:56:46PM -0500, Peter Xu wrote:
> > > On Wed, Feb 12, 2025 at 05:48:46PM +0000, Daniel P. Berrangé wrote:
> > > > On Wed, Feb 12, 2025 at 12:38:23PM -0500, Peter Xu wrote:
> > > > > From: Daniil Tatianin <d-tatianin@yandex-team.ru>
> > > > > 
> > > > > This will be used in the following commits to make it possible to only
> > > > > lock memory on fault instead of right away.
> > > > > 
> > > > > Signed-off-by: Daniil Tatianin <d-tatianin@yandex-team.ru>
> > > > > Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> > > > > Link: https://lore.kernel.org/r/20250212143920.1269754-2-d-tatianin@yandex-team.ru
> > > > > [peterx: fail os_mlock(on_fault=1) when not supported]
> > > > > [peterx: use G_GNUC_UNUSED instead of "(void)on_fault", per Dan]
> > > > > Signed-off-by: Peter Xu <peterx@redhat.com>
> > > > > ---
> > > > >  meson.build               |  6 ++++++
> > > > >  include/system/os-posix.h |  2 +-
> > > > >  include/system/os-win32.h |  2 +-
> > > > >  migration/postcopy-ram.c  |  2 +-
> > > > >  os-posix.c                | 15 +++++++++++++--
> > > > >  system/vl.c               |  2 +-
> > > > >  6 files changed, 23 insertions(+), 6 deletions(-)
> > > > > 
> > > > > diff --git a/meson.build b/meson.build
> > > > > index 18cf9e2913..59953cbe6b 100644
> > > > > --- a/meson.build
> > > > > +++ b/meson.build
> > > > > @@ -2885,6 +2885,12 @@ config_host_data.set('HAVE_MLOCKALL', cc.links(gnu_source_prefix + '''
> > > > >      return mlockall(MCL_FUTURE);
> > > > >    }'''))
> > > > >  
> > > > > +config_host_data.set('HAVE_MLOCK_ONFAULT', cc.links(gnu_source_prefix + '''
> > > > > +  #include <sys/mman.h>
> > > > > +  int main(void) {
> > > > > +      return mlockall(MCL_FUTURE | MCL_ONFAULT);
> > > > > +  }'''))
> > > > > +
> > > > >  have_l2tpv3 = false
> > > > >  if get_option('l2tpv3').allowed() and have_system
> > > > >    have_l2tpv3 = cc.has_type('struct mmsghdr',
> > > > > diff --git a/include/system/os-posix.h b/include/system/os-posix.h
> > > > > index b881ac6c6f..ce5b3bccf8 100644
> > > > > --- a/include/system/os-posix.h
> > > > > +++ b/include/system/os-posix.h
> > > > > @@ -53,7 +53,7 @@ bool os_set_runas(const char *user_id);
> > > > >  void os_set_chroot(const char *path);
> > > > >  void os_setup_limits(void);
> > > > >  void os_setup_post(void);
> > > > > -int os_mlock(void);
> > > > > +int os_mlock(bool on_fault);
> > > > >  
> > > > >  /**
> > > > >   * qemu_alloc_stack:
> > > > > diff --git a/include/system/os-win32.h b/include/system/os-win32.h
> > > > > index b82a5d3ad9..bc623061d8 100644
> > > > > --- a/include/system/os-win32.h
> > > > > +++ b/include/system/os-win32.h
> > > > > @@ -123,7 +123,7 @@ static inline bool is_daemonized(void)
> > > > >      return false;
> > > > >  }
> > > > >  
> > > > > -static inline int os_mlock(void)
> > > > > +static inline int os_mlock(bool on_fault G_GNUC_UNUSED)
> > > > 
> > > > So did this actually generate a warning ? We don' even need
> > > > G_GNUC_UNUSED unless we're actually seeing warnings about this.
> > > 
> > > I didn't try to hit a warning without it, as we can use different compilers
> > > and I thought the results could be different, even if I try it and it
> > > didn't raise a warning?
> > 
> > We strictly only permit use of clang & gcc.
> 
> I meant I am also not sure whether the versions could matter.. Totally not
> expert on compilers.  Hence I chose to be safe with the attribute applied,
> because I know it'll always be safe when with it.
> 
> I tried to grep QEMU code base:
> 
> $ git grep unused-parameter
> $ git grep -w Wall
> pc-bios/optionrom/Makefile:override CFLAGS += -march=i486 -Wall $(EXTRA_CFLAGS) -m16
> pc-bios/s390-ccw/Makefile:EXTRA_CFLAGS += -Wall
> tests/multiboot/Makefile:CCFLAGS=-m32 -Wall -Wextra -Werror -fno-stack-protector -nostdinc -fno-builtin
> tests/tcg/Makefile.target:CFLAGS+=-Wall -Werror -O0 -g -fno-strict-aliasing
> tests/tcg/mips/user/isa/r5900/Makefile:CFLAGS  = -Wall -mabi=32 -march=r5900 -static
> 
> We don't seem to have explicit requirement on that (as I grepped nothing
> for "unused-parameter"), meanwhile indeed we also seem to have zero usage
> at least in qemu root with enabling -Wall.  I'm not sure whether other
> compiler option could matter here.
> 
> Can this be justifed as we can safely drop G_GNUC_UNUSED in this patch?
> 
> OTOH:
> 
> $ git grep G_GNUC_UNUSED | wc -l
> 169
> 
> So we have 169 existing such use cases.  Does it also mean we could
> potentially drop all of them?

Yes, or actually turn on the warning about unused params and mark
the rest. It is initially noisey, but IME does end up flagging
real problems periodically. Anyway, given we're inconsistent already
there's no need to respin this series.

> 
> It'll definitely be easier for me (and hopefully Stefan too..) if this can
> be done later, but I'm OK respin v3.
> 
> Dan, do you have any preference / suggestion?

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|