Re: [PATCH v4 02/20] tools: Add control group manager

[Alyssa Ross <hi@alyssa.is>]

[-- Attachment #1: Type: text/plain, Size: 4859 bytes --]

Demi Marie Obenour <demiobenour@gmail.com> writes:

> On 7/27/26 08:10, Alyssa Ross wrote:
>> Demi Marie Obenour <demiobenour@gmail.com> writes:
>> 
>>> On 7/22/26 12:01, Alyssa Ross wrote:
>>>> Demi Marie Obenour <demiobenour@gmail.com> writes:
>> 
>>>>> +fn remove_recursively(fd: OwnedFd, remaining_depth: usize) -> Result<(), Errno> {
>>>>> +    if remaining_depth < 1 {
>>>>> +        panic!("control groups too deeply nested");
>>>>> +    }
>>>>> +    let mut d = rustix::fs::Dir::new(fd).expect("cannot start iterating");
>>>>> +    while let Some(element) = d.next() {
>>>>> +        let element = element.expect("Iterating through a cgroup directory failed?");
>>>>> +        if element.file_type() != rustix::fs::FileType::Directory {
>>>>> +            continue;
>>>>> +        }
>>>>> +
>>>>> +        let remaining_depth = remaining_depth - 1;
>>>>> +        let d: &rustix::fs::Dir = &d;
>>>>> +        let dirfd = d.fd().unwrap();
>>>>> +        let path = element.file_name();
>>>>> +        remove_all(remaining_depth, dirfd, path)?;
>> 
>> I'd inline these variables into the function call the extent possible.
>> Otherwise I have to follow a lot of shuffling around.
>
> Will fix in v5.
>
>>>>> +    }
>>>>> +    drop(d);
>>>>> +    Ok(())
>>>>> +}
>>>>> +
>>>>> +fn remove_all(
>>>>> +    remaining_depth: usize,
>>>>> +    dirfd: BorrowedFd<'_>,
>>>>> +    path: &std::ffi::CStr,
>>>>> +) -> Result<(), Errno> {
>>>>> +    if path == c"." || path == c".." {
>>>>> +        return Ok(());
>>>>> +    }
>> 
>> It's a bit weird that calling remove_all on . or .. does not fail.
>> Maybe would be clearer to move this check to the call site?
>
> Will fix in v5, making the code simpler.
>
>>>>> +    if rustix::fs::unlinkat(dirfd, path, AtFlags::REMOVEDIR).is_ok() {
>>>>> +        return Ok(());
>>>>> +    }
>> 
>> We could drop this, right?  A few extra syscalls, but less to wrap my
>> head around.
>
> Correct.
>
>>>>> +    let fd = rustix::fs::openat2(
>>>>> +        dirfd,
>>>>> +        path,
>>>>> +        OFlags::CLOEXEC | OFlags::NOFOLLOW | OFlags::RDONLY | OFlags::DIRECTORY,
>>>>> +        Mode::empty(),
>>>>> +        ResolveFlags::NO_SYMLINKS | ResolveFlags::BENEATH | ResolveFlags::NO_XDEV,
>>>>> +    )?;
>>>>> +    remove_recursively(fd, remaining_depth)?;
>>>>> +    rustix::fs::unlinkat(dirfd, path, AtFlags::REMOVEDIR)?;
>>>>> +    Ok(())
>>>>> +}
>>>>
>>>> Could we save a lot of code by calling std::fs::remove_dir_all with a
>>>> /proc/self/fd path?  It's already documented to ignore symlinks.
>>>
>>> I tried, but that tries to delete files too, and that isn't allowed.
>> 
>> Ah, alright.  That could use an explanatory comment.
>> 
>> How can we be confident we have enough stack for this?  Is there a way
>> it could be done non-recursively, with state on the heap?
>
> In Spectrum, the depth is currently limited to a small constant.

Right, but that's really papering over the problem, and our stack is
also limited to a small constant size.  We could avoid the problem
entirely by instead running paths to operate on through a VecDeque
rather than recursing, and then we wouldn't need any arbitrary limit.

>>>> Last time I suggested a clearer way of doing this, but it has instead
>>>> got even less clear.
>>>>
>>>> (I'm not sure why we'd care if there's a newline specifically, as
>>>> opposed to any other control character.)
>>>
>>> If cgroups v1 is in use, the file can contain multiple lines, one for
>>> each cgroup the program is in.  I also am not sure if starting with
>>> "0::/" is an invariant in that case.
>>>
>>> Using this program with cgroups v1 mounted is user error and will
>>> never happen on Spectrum, but if this tool is used outside of Spectrum,
>>> it could happen.
>> 
>> I see.  It seems like with cgroups v1, it _could_ start with 0::/, but
>> probably wouldn't.  I think it may not be possible to tell from this
>> file whether cgroups v1 is in use.
>> 
>> So I suppose it depends what you want to happen if cgroups v1 is in use.
>> If it looks enough like cgroups v2, do you continue, or do you
>> explicitly check for cgroups v1?  If the latter (sounds more sensible to
>> me), you need to explicitly check for cgroups v1 somehow I think.  Can
>> cgroups v1 and v2 be in use at the same time?  If so, checking might be
>> complicated, but if not, you can just check what type of filesystem is
>> mounted at /sys/fs/cgroup, or see if it has a
>> /sys/fs/cgroup/cgroup.controllers file.
>
> In the case of Spectrum, I think it's okay to just panic if cgroups v2
> isn't mounted or isn't working properly.  It's a bug in either
> cgroup-setup or the kernel, almost certainly the former.

Yeah I agree it's completely fine for new stuff to requrie cgroups v2.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 227 bytes --]