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

[Peter Xu <peterx@redhat.com>]

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?

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