From 8b3ced3bec4dc4396305d4ce07984ff836dbaa48 Mon Sep 17 00:00:00 2001 From: Jay Lee Date: Sun, 11 Jan 2026 18:45:45 +0800 Subject: [PATCH] jemalloc-ctl: fix invalid update implementation oldp should not be the same as newp, otherwise jemalloc may write to oldp first and the new value will be lost. Also fix invalid test value for max_background_threads and epoch. Signed-off-by: Jay Lee --- jemalloc-ctl/src/lib.rs | 7 ++- jemalloc-ctl/src/macros.rs | 118 +++++++++++++++++++------------------ jemalloc-ctl/src/raw.rs | 50 ++++++++-------- 3 files changed, 91 insertions(+), 84 deletions(-) diff --git a/jemalloc-ctl/src/lib.rs b/jemalloc-ctl/src/lib.rs index 487f719fe..8b36bacc5 100644 --- a/jemalloc-ctl/src/lib.rs +++ b/jemalloc-ctl/src/lib.rs @@ -192,7 +192,8 @@ option! { /// `jemalloc` epoch. /// /// Many of the statistics tracked by `jemalloc` are cached. The epoch - /// controls when they are refreshed. + /// controls when they are refreshed. Both `write` and `update` are the + /// same as calling `advance()` and ignore any provided value. /// /// # Example /// @@ -217,14 +218,14 @@ option! { } impl epoch { - /// Advances the epoch returning its old value - see [`epoch`]. + /// Advances the epoch returning its latest value - see [`epoch`]. pub fn advance() -> crate::error::Result { Self::update(1) } } impl epoch_mib { - /// Advances the epoch returning its old value - see [`epoch`]. + /// Advances the epoch returning its latest value - see [`epoch`]. pub fn advance(self) -> crate::error::Result { self.0.update(1) } diff --git a/jemalloc-ctl/src/macros.rs b/jemalloc-ctl/src/macros.rs index 23f48e7f5..94050e354 100644 --- a/jemalloc-ctl/src/macros.rs +++ b/jemalloc-ctl/src/macros.rs @@ -60,31 +60,6 @@ macro_rules! r { self.0.read() } } - - #[cfg(test)] - #[test] - #[allow(unused)] - fn [<$id _read_test>]() { - match stringify!($id) { - "background_thread" | - "max_background_threads" - if cfg!(target_os = "macos") => return, - _ => (), - } - - let a = $id::read().unwrap(); - - let mib = $id::mib().unwrap(); - let b = mib.read().unwrap(); - - #[cfg(feature = "use_std")] - println!( - concat!( - stringify!($id), - " (read): \"{}\" - \"{}\""), - a, b - ); - } } }; } @@ -108,31 +83,6 @@ macro_rules! w { self.0.write(value) } } - - #[cfg(test)] - #[test] - fn [<$id _write_test>]() { - match stringify!($id) { - "background_thread" | - "max_background_threads" - if cfg!(target_os = "macos") => return, - _ => (), - } - - let _ = $id::write($ret_ty::default()).unwrap(); - - let mib = $id::mib().unwrap(); - let _ = mib.write($ret_ty::default()).unwrap(); - - #[cfg(feature = "use_std")] - println!( - concat!( - stringify!($id), - " (write): \"{}\""), - $ret_ty::default() - ); - - } } }; } @@ -156,11 +106,26 @@ macro_rules! u { self.0.update(value) } } + } + }; +} +macro_rules! make_test { + ($id:ident, $ret_ty:ty, ()) => {}; + (max_background_threads, $ret_ty:ty, ($($ops:ident),+)) => { + make_test!(max_background_threads, $ret_ty, |_| 1, $($ops),+); + }; + (epoch, $ret_ty:ty, ($($ops:ident),+)) => { + make_test!(epoch, $ret_ty, |k| k + 1, $($ops),+); + }; + ($id:ident, $ret_ty:ty, ($($ops:ident),+)) => { + make_test!($id, $ret_ty, |_| Default::default(), $($ops),+); + }; + ($id:ident, $ret_ty:ty, $test_val:expr, r,w,u) => { + paste::paste! { #[cfg(test)] #[test] - #[allow(unused)] - fn [<$id _update_test>]() { + fn [<$id _read_write_update_test>]() { match stringify!($id) { "background_thread" | "max_background_threads" @@ -168,17 +133,56 @@ macro_rules! u { _ => (), } - let a = $id::update($ret_ty::default()).unwrap(); + let a = $id::read().unwrap(); + let b = $test_val(a); + let _ = $id::write(b).unwrap(); + let c = $id::read().unwrap(); + assert_eq!(b, c); + let d = $id::update(a).unwrap(); + let e = $id::read().unwrap(); + if stringify!($id) == "epoch" { + assert_eq!(d, e); + assert_ne!(a, e); + } else { + assert_eq!(d, c); + assert_eq!(a, e); + } let mib = $id::mib().unwrap(); - let b = mib.update($ret_ty::default()).unwrap(); + let f = mib.read().unwrap(); + assert_eq!(e, f); + let g = $test_val(f); + let _ = mib.write(g).unwrap(); + let h = mib.read().unwrap(); + assert_eq!(g, h); + let i = mib.update(f).unwrap(); + let j = mib.read().unwrap(); + if stringify!($id) == "epoch" { + assert_eq!(i, j); + assert_ne!(f, j); + } else { + assert_eq!(i, h); + assert_eq!(f, j); + } + } + } + }; + ($id:ident, $ret_ty:ty, $test_val:expr, r) => { + paste::paste! { + #[cfg(test)] + #[test] + fn [<$id _read_test>]() { + let a = $id::read().unwrap(); + let mib = $id::mib().unwrap(); + let b = mib.read().unwrap(); + assert_eq!(a, b); #[cfg(feature = "use_std")] println!( concat!( stringify!($id), - " (update): (\"{}\", \"{}\") - \"{}\""), - a, b, $ret_ty::default() + " (read): \"{}\" - \"{}\""), + a, b ); } } @@ -202,6 +206,8 @@ macro_rules! option { $( $ops!($id => $ret_ty); )* + + make_test!($id, $ret_ty, ($($ops),*)); }; // Non-string option: ($id:ident[ str: $byte_string:expr, non_str: $mib_len:expr ] => $ret_ty:ty | diff --git a/jemalloc-ctl/src/raw.rs b/jemalloc-ctl/src/raw.rs index e34d13e3d..e46406c7f 100644 --- a/jemalloc-ctl/src/raw.rs +++ b/jemalloc-ctl/src/raw.rs @@ -1,7 +1,10 @@ //! Raw `unsafe` access to the `malloctl` API. use crate::error::{cvt, Result}; -use crate::{mem, ptr, slice}; +use crate::{ + mem::{self, MaybeUninit}, + ptr, slice, +}; use libc::c_char; /// Translates `name` to a `mib` (Management Information Base) @@ -64,18 +67,18 @@ pub fn name_to_mib(name: &[u8], mib: &mut [usize]) -> Result<()> { /// sizes of `bool` and `u8` match, but `bool` cannot represent all values that /// `u8` can. pub unsafe fn read_mib(mib: &[usize]) -> Result { - let mut value = MaybeUninit { init: () }; + let mut value = MaybeUninit::::uninit(); let mut len = mem::size_of::(); cvt(tikv_jemalloc_sys::mallctlbymib( mib.as_ptr(), mib.len(), - &mut value.init as *mut _ as *mut _, + value.as_mut_ptr() as *mut _, &mut len, ptr::null_mut(), 0, ))?; assert_eq!(len, mem::size_of::()); - Ok(value.maybe_uninit) + Ok(value.assume_init()) } /// Uses the null-terminated string `name` as key to the _MALLCTL NAMESPACE_ and @@ -90,17 +93,17 @@ pub unsafe fn read_mib(mib: &[usize]) -> Result { pub unsafe fn read(name: &[u8]) -> Result { validate_name(name); - let mut value = MaybeUninit { init: () }; + let mut value = MaybeUninit::::uninit(); let mut len = mem::size_of::(); cvt(tikv_jemalloc_sys::mallctl( name as *const _ as *const c_char, - &mut value.init as *mut _ as *mut _, + value.as_mut_ptr() as *mut _, &mut len, ptr::null_mut(), 0, ))?; assert_eq!(len, mem::size_of::()); - Ok(value.maybe_uninit) + Ok(value.assume_init()) } /// Uses the MIB `mib` as key to the _MALLCTL NAMESPACE_ and writes its `value`. @@ -158,18 +161,19 @@ pub unsafe fn write(name: &[u8], mut value: T) -> Result<()> { /// invalid `T`, for example, by passing `T=u8` for a key expecting `bool`. The /// sizes of `bool` and `u8` match, but `bool` cannot represent all values that /// `u8` can. -pub unsafe fn update_mib(mib: &[usize], mut value: T) -> Result { - let mut len = mem::size_of::(); +pub unsafe fn update_mib(mib: &[usize], mut value: T) -> Result { + let mut old_len = mem::size_of::(); + let mut old_value = MaybeUninit::::uninit(); cvt(tikv_jemalloc_sys::mallctlbymib( mib.as_ptr(), mib.len(), + old_value.as_mut_ptr() as *mut _, + &mut old_len, &mut value as *mut _ as *mut _, - &mut len, - &mut value as *mut _ as *mut _, - len, + mem::size_of::(), ))?; - assert_eq!(len, mem::size_of::()); - Ok(value) + assert_eq!(old_len, mem::size_of::()); + Ok(old_value.assume_init()) } /// Uses the null-terminated string `name` as key to the _MALLCTL NAMESPACE_ and @@ -184,16 +188,17 @@ pub unsafe fn update_mib(mib: &[usize], mut value: T) -> Result { pub unsafe fn update(name: &[u8], mut value: T) -> Result { validate_name(name); - let mut len = mem::size_of::(); + let mut old_len = mem::size_of::(); + let mut old_value = MaybeUninit::::uninit(); cvt(tikv_jemalloc_sys::mallctl( name as *const _ as *const c_char, + old_value.as_mut_ptr() as *mut _, + &mut old_len, &mut value as *mut _ as *mut _, - &mut len, - &mut value as *mut _ as *mut _, - len, + mem::size_of::(), ))?; - assert_eq!(len, mem::size_of::()); - Ok(value) + assert_eq!(old_len, mem::size_of::()); + Ok(old_value.assume_init()) } /// Uses the MIB `mib` as key to the _MALLCTL NAMESPACE_ and reads its value. @@ -376,11 +381,6 @@ fn validate_name(name: &[u8]) { ); } -union MaybeUninit { - init: (), - maybe_uninit: T, -} - #[cfg(test)] mod tests { use super::*;