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 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 do see though that we have plenty of such uses in the current tree,
> though.  Does it mean it's a broader question to ask, rather than this
> patch only?
> 
> Thanks,
> 
> -- 
> Peter Xu
> 

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 :|