Skip to content

Commit 8b3ced3

Browse files
BusyJayBusyJay
committed
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 <busyjaylee@gmail.com>
1 parent c103179 commit 8b3ced3

File tree

3 files changed

+91
-84
lines changed

3 files changed

+91
-84
lines changed

‎jemalloc-ctl/src/lib.rs‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -192,7 +192,8 @@ option! {
192192
/// `jemalloc` epoch.
193193
///
194194
/// Many of the statistics tracked by `jemalloc` are cached. The epoch
195-
/// controls when they are refreshed.
195+
/// controls when they are refreshed. Both `write` and `update` are the
196+
/// same as calling `advance()` and ignore any provided value.
196197
///
197198
/// # Example
198199
///
@@ -217,14 +218,14 @@ option! {
217218
}
218219

219220
impl epoch {
220-
/// Advances the epoch returning its old value - see [`epoch`].
221+
/// Advances the epoch returning its latest value - see [`epoch`].
221222
pub fn advance() -> crate::error::Result<u64> {
222223
Self::update(1)
223224
}
224225
}
225226

226227
impl epoch_mib {
227-
/// Advances the epoch returning its old value - see [`epoch`].
228+
/// Advances the epoch returning its latest value - see [`epoch`].
228229
pub fn advance(self) -> crate::error::Result<u64> {
229230
self.0.update(1)
230231
}

‎jemalloc-ctl/src/macros.rs‎

Lines changed: 62 additions & 56 deletions
Original file line numberDiff line numberDiff line change
@@ -60,31 +60,6 @@ macro_rules! r {
6060
self.0.read()
6161
}
6262
}
63-
64-
#[cfg(test)]
65-
#[test]
66-
#[allow(unused)]
67-
fn [<$id _read_test>]() {
68-
match stringify!($id) {
69-
"background_thread" |
70-
"max_background_threads"
71-
if cfg!(target_os = "macos") => return,
72-
_ => (),
73-
}
74-
75-
let a = $id::read().unwrap();
76-
77-
let mib = $id::mib().unwrap();
78-
let b = mib.read().unwrap();
79-
80-
#[cfg(feature = "use_std")]
81-
println!(
82-
concat!(
83-
stringify!($id),
84-
" (read): \"{}\" - \"{}\""),
85-
a, b
86-
);
87-
}
8863
}
8964
};
9065
}
@@ -108,31 +83,6 @@ macro_rules! w {
10883
self.0.write(value)
10984
}
11085
}
111-
112-
#[cfg(test)]
113-
#[test]
114-
fn [<$id _write_test>]() {
115-
match stringify!($id) {
116-
"background_thread" |
117-
"max_background_threads"
118-
if cfg!(target_os = "macos") => return,
119-
_ => (),
120-
}
121-
122-
let _ = $id::write($ret_ty::default()).unwrap();
123-
124-
let mib = $id::mib().unwrap();
125-
let _ = mib.write($ret_ty::default()).unwrap();
126-
127-
#[cfg(feature = "use_std")]
128-
println!(
129-
concat!(
130-
stringify!($id),
131-
" (write): \"{}\""),
132-
$ret_ty::default()
133-
);
134-
135-
}
13686
}
13787
};
13888
}
@@ -156,29 +106,83 @@ macro_rules! u {
156106
self.0.update(value)
157107
}
158108
}
109+
}
110+
};
111+
}
159112

113+
macro_rules! make_test {
114+
($id:ident, $ret_ty:ty, ()) => {};
115+
(max_background_threads, $ret_ty:ty, ($($ops:ident),+)) => {
116+
make_test!(max_background_threads, $ret_ty, |_| 1, $($ops),+);
117+
};
118+
(epoch, $ret_ty:ty, ($($ops:ident),+)) => {
119+
make_test!(epoch, $ret_ty, |k| k + 1, $($ops),+);
120+
};
121+
($id:ident, $ret_ty:ty, ($($ops:ident),+)) => {
122+
make_test!($id, $ret_ty, |_| Default::default(), $($ops),+);
123+
};
124+
($id:ident, $ret_ty:ty, $test_val:expr, r,w,u) => {
125+
paste::paste! {
160126
#[cfg(test)]
161127
#[test]
162-
#[allow(unused)]
163-
fn [<$id _update_test>]() {
128+
fn [<$id _read_write_update_test>]() {
164129
match stringify!($id) {
165130
"background_thread" |
166131
"max_background_threads"
167132
if cfg!(target_os = "macos") => return,
168133
_ => (),
169134
}
170135

171-
let a = $id::update($ret_ty::default()).unwrap();
136+
let a = $id::read().unwrap();
137+
let b = $test_val(a);
138+
let _ = $id::write(b).unwrap();
139+
let c = $id::read().unwrap();
140+
assert_eq!(b, c);
141+
let d = $id::update(a).unwrap();
142+
let e = $id::read().unwrap();
143+
if stringify!($id) == "epoch" {
144+
assert_eq!(d, e);
145+
assert_ne!(a, e);
146+
} else {
147+
assert_eq!(d, c);
148+
assert_eq!(a, e);
149+
}
172150

173151
let mib = $id::mib().unwrap();
174-
let b = mib.update($ret_ty::default()).unwrap();
152+
let f = mib.read().unwrap();
153+
assert_eq!(e, f);
154+
let g = $test_val(f);
155+
let _ = mib.write(g).unwrap();
156+
let h = mib.read().unwrap();
157+
assert_eq!(g, h);
158+
let i = mib.update(f).unwrap();
159+
let j = mib.read().unwrap();
160+
if stringify!($id) == "epoch" {
161+
assert_eq!(i, j);
162+
assert_ne!(f, j);
163+
} else {
164+
assert_eq!(i, h);
165+
assert_eq!(f, j);
166+
}
167+
}
168+
}
169+
};
170+
($id:ident, $ret_ty:ty, $test_val:expr, r) => {
171+
paste::paste! {
172+
#[cfg(test)]
173+
#[test]
174+
fn [<$id _read_test>]() {
175+
let a = $id::read().unwrap();
176+
let mib = $id::mib().unwrap();
177+
let b = mib.read().unwrap();
178+
assert_eq!(a, b);
175179

176180
#[cfg(feature = "use_std")]
177181
println!(
178182
concat!(
179183
stringify!($id),
180-
" (update): (\"{}\", \"{}\") - \"{}\""),
181-
a, b, $ret_ty::default()
184+
" (read): \"{}\" - \"{}\""),
185+
a, b
182186
);
183187
}
184188
}
@@ -202,6 +206,8 @@ macro_rules! option {
202206
$(
203207
$ops!($id => $ret_ty);
204208
)*
209+
210+
make_test!($id, $ret_ty, ($($ops),*));
205211
};
206212
// Non-string option:
207213
($id:ident[ str: $byte_string:expr, non_str: $mib_len:expr ] => $ret_ty:ty |

‎jemalloc-ctl/src/raw.rs‎

Lines changed: 25 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,10 @@
11
//! Raw `unsafe` access to the `malloctl` API.
22
33
use crate::error::{cvt, Result};
4-
use crate::{mem, ptr, slice};
4+
use crate::{
5+
mem::{self, MaybeUninit},
6+
ptr, slice,
7+
};
58
use libc::c_char;
69

710
/// Translates `name` to a `mib` (Management Information Base)
@@ -64,18 +67,18 @@ pub fn name_to_mib(name: &[u8], mib: &mut [usize]) -> Result<()> {
6467
/// sizes of `bool` and `u8` match, but `bool` cannot represent all values that
6568
/// `u8` can.
6669
pub unsafe fn read_mib<T: Copy>(mib: &[usize]) -> Result<T> {
67-
let mut value = MaybeUninit { init: () };
70+
let mut value = MaybeUninit::<T>::uninit();
6871
let mut len = mem::size_of::<T>();
6972
cvt(tikv_jemalloc_sys::mallctlbymib(
7073
mib.as_ptr(),
7174
mib.len(),
72-
&mut value.init as *mut _ as *mut _,
75+
value.as_mut_ptr() as *mut _,
7376
&mut len,
7477
ptr::null_mut(),
7578
0,
7679
))?;
7780
assert_eq!(len, mem::size_of::<T>());
78-
Ok(value.maybe_uninit)
81+
Ok(value.assume_init())
7982
}
8083

8184
/// Uses the null-terminated string `name` as key to the _MALLCTL NAMESPACE_ and
@@ -90,17 +93,17 @@ pub unsafe fn read_mib<T: Copy>(mib: &[usize]) -> Result<T> {
9093
pub unsafe fn read<T: Copy>(name: &[u8]) -> Result<T> {
9194
validate_name(name);
9295

93-
let mut value = MaybeUninit { init: () };
96+
let mut value = MaybeUninit::<T>::uninit();
9497
let mut len = mem::size_of::<T>();
9598
cvt(tikv_jemalloc_sys::mallctl(
9699
name as *const _ as *const c_char,
97-
&mut value.init as *mut _ as *mut _,
100+
value.as_mut_ptr() as *mut _,
98101
&mut len,
99102
ptr::null_mut(),
100103
0,
101104
))?;
102105
assert_eq!(len, mem::size_of::<T>());
103-
Ok(value.maybe_uninit)
106+
Ok(value.assume_init())
104107
}
105108

106109
/// Uses the MIB `mib` as key to the _MALLCTL NAMESPACE_ and writes its `value`.
@@ -158,18 +161,19 @@ pub unsafe fn write<T>(name: &[u8], mut value: T) -> Result<()> {
158161
/// invalid `T`, for example, by passing `T=u8` for a key expecting `bool`. The
159162
/// sizes of `bool` and `u8` match, but `bool` cannot represent all values that
160163
/// `u8` can.
161-
pub unsafe fn update_mib<T>(mib: &[usize], mut value: T) -> Result<T> {
162-
let mut len = mem::size_of::<T>();
164+
pub unsafe fn update_mib<T: Copy>(mib: &[usize], mut value: T) -> Result<T> {
165+
let mut old_len = mem::size_of::<T>();
166+
let mut old_value = MaybeUninit::<T>::uninit();
163167
cvt(tikv_jemalloc_sys::mallctlbymib(
164168
mib.as_ptr(),
165169
mib.len(),
170+
old_value.as_mut_ptr() as *mut _,
171+
&mut old_len,
166172
&mut value as *mut _ as *mut _,
167-
&mut len,
168-
&mut value as *mut _ as *mut _,
169-
len,
173+
mem::size_of::<T>(),
170174
))?;
171-
assert_eq!(len, mem::size_of::<T>());
172-
Ok(value)
175+
assert_eq!(old_len, mem::size_of::<T>());
176+
Ok(old_value.assume_init())
173177
}
174178

175179
/// Uses the null-terminated string `name` as key to the _MALLCTL NAMESPACE_ and
@@ -184,16 +188,17 @@ pub unsafe fn update_mib<T>(mib: &[usize], mut value: T) -> Result<T> {
184188
pub unsafe fn update<T>(name: &[u8], mut value: T) -> Result<T> {
185189
validate_name(name);
186190

187-
let mut len = mem::size_of::<T>();
191+
let mut old_len = mem::size_of::<T>();
192+
let mut old_value = MaybeUninit::<T>::uninit();
188193
cvt(tikv_jemalloc_sys::mallctl(
189194
name as *const _ as *const c_char,
195+
old_value.as_mut_ptr() as *mut _,
196+
&mut old_len,
190197
&mut value as *mut _ as *mut _,
191-
&mut len,
192-
&mut value as *mut _ as *mut _,
193-
len,
198+
mem::size_of::<T>(),
194199
))?;
195-
assert_eq!(len, mem::size_of::<T>());
196-
Ok(value)
200+
assert_eq!(old_len, mem::size_of::<T>());
201+
Ok(old_value.assume_init())
197202
}
198203

199204
/// Uses the MIB `mib` as key to the _MALLCTL NAMESPACE_ and reads its value.
@@ -376,11 +381,6 @@ fn validate_name(name: &[u8]) {
376381
);
377382
}
378383

379-
union MaybeUninit<T: Copy> {
380-
init: (),
381-
maybe_uninit: T,
382-
}
383-
384384
#[cfg(test)]
385385
mod tests {
386386
use super::*;

0 commit comments

Comments
 (0)