Re: [PATCH proxmox 11/26] procfs: add helpers for querying pressure stall information

["Lukas Wagner" <l.wagner@proxmox.com>]

Hey Arthur, thanks for the review!

On Mon Mar 16, 2026 at 2:25 PM CET, Arthur Bied-Charreton wrote:
> On Thu, Mar 12, 2026 at 02:52:12PM +0100, Lukas Wagner wrote:
>> This is put into a new crate, proxmox-procfs, since proxmox-sys is
>> already quite large and should be split in the future. The general idea
>> is that the contents of proxmox_sys::linux::procfs should be moved into
>> this new crate (potentially after some API cleanup) at some point.
>> 
>> Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
> I really like how you designed this, it reads very nicely! 
>
> 2 comments inline 
>> ---
>>  Cargo.toml                          |   2 +
>>  proxmox-procfs/Cargo.toml           |  18 ++
>>  proxmox-procfs/debian/changelog     |   5 +
>>  proxmox-procfs/debian/control       |  50 +++++
>>  proxmox-procfs/debian/copyright     |  18 ++
>>  proxmox-procfs/debian/debcargo.toml |   7 +
>>  proxmox-procfs/src/lib.rs           |   1 +
>>  proxmox-procfs/src/pressure.rs      | 334 ++++++++++++++++++++++++++++
>>  8 files changed, 435 insertions(+)
>>  create mode 100644 proxmox-procfs/Cargo.toml
>>  create mode 100644 proxmox-procfs/debian/changelog
>>  create mode 100644 proxmox-procfs/debian/control
>>  create mode 100644 proxmox-procfs/debian/copyright
>>  create mode 100644 proxmox-procfs/debian/debcargo.toml
>>  create mode 100644 proxmox-procfs/src/lib.rs
>>  create mode 100644 proxmox-procfs/src/pressure.rs
>> +
> [...]
>> +    fn read_pressure_line<R: BufRead>(
>> +        reader: &mut R,
>> +        buf: &mut Vec<u8>,
>> +    ) -> Result<(PressureRecordKind, PressureRecord), Error> {
>> +        // The buffer should be empty. It is only passed by the caller as a performance
>> +        // optimization
>> +        debug_assert!(buf.is_empty());
>> +
>> +        reader.read_until(b'\n', buf)?;
>> +        // SAFETY: In production, `reader` is expected to read from
>> +        // procfs/sysfs pressure files, which only ever should return ASCII strings.
>> +        let line = unsafe { std::str::from_utf8_unchecked(buf) };
> Major nit, but I wonder if using unsafe here is justified, given that this 
> assumption is not enforced at the type level (BufRead could return anything),
> and this is not really a hot code path

I saw the same unsafe block elsewhere in `proxmox-sys` which inspired me
to do the same - but it's probably worth to benchmark this and check if
it actually gives us a real-world performance benefit that is worth the
hassle. As you said, will never be a particularly hot code path, in PDM
(and soon PBS context) this will be called once every 10 seconds.

>> +
>> +        Self::read_record(line)
>> +    }
>> +
>> +
> [...]
>> +#[cfg(test)]
>> +mod test {
>> +    use super::*;
>> +
>> +    #[test]
>> +    fn test_read_psi() {
>> +        let s = "some avg10=1.42 avg60=2.09 avg300=1.42 total=40979658
>> +full avg10=0.08 avg60=0.18 avg300=0.13 total=22865313
>> +";
>> +
>> +        let mut reader = std::io::Cursor::new(s);
>> +        let stats = PressureData::read(&mut reader).unwrap();
>> +
>> +        assert_eq!(stats.some.total, 40979658);
>> +        assert!((stats.some.average_10 - 1.82).abs() < f64::EPSILON);
> This test is failing, looks like this was supposed to be 1.42

Argh, this was a last-minute clippy fix before I sent the series (I
forgot the .abs() initially), apparently I failed to run the tests again
after that.

Thanks! I will of course fix this for v2.

>> +        assert!((stats.some.average_60 - 2.09).abs() < f64::EPSILON);
>> +        assert!((stats.some.average_300 - 1.42).abs() < f64::EPSILON);
>> +
> [...]