Received-SPF: pass (google.com: domain of linux-kernel+bounces-125733-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Date: Sat, 30 Mar 2024 13:30:17 +0000
To: Wedson Almeida Filho <wedsonaf@gmail.com>, rust-for-linux@vger.kernel.org
From: Benno Lossin <benno.lossin@proton.me>
Cc: Miguel Ojeda <ojeda@kernel.org>, Alex Gaynor <alex.gaynor@gmail.com>, Boqun Feng <boqun.feng@gmail.com>, Gary Guo <gary@garyguo.net>, =?utf-8?Q?Bj=C3=B6rn_Roy_Baron?= <bjorn3_gh@protonmail.com>, Andreas Hindborg <a.hindborg@samsung.com>, Alice Ryhl <aliceryhl@google.com>, linux-kernel@vger.kernel.org, Wedson Almeida Filho <walmeida@microsoft.com>
Subject: Re: [PATCH v3 07/10] rust: alloc: update `VecExt` to take allocation flags
Message-ID: <59c5bda5-6116-42f1-bf8e-47eba02e2ecb@proton.me>
In-Reply-To: <20240328013603.206764-8-wedsonaf@gmail.com>
References: <20240328013603.206764-1-wedsonaf@gmail.com> <20240328013603.206764-8-wedsonaf@gmail.com>
Feedback-ID: 71780778:user:proton
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable

On 28.03.24 02:36, Wedson Almeida Filho wrote:
> From: Wedson Almeida Filho <walmeida@microsoft.com>
>=20
> We also rename the methods by removing the `try_` prefix since the names
> are available due to our usage of the `no_global_oom_handling` config
> when building the `alloc` crate.
>=20
> Reviewed-by: Boqun Feng <boqun.feng@gmail.com>
> Signed-off-by: Wedson Almeida Filho <walmeida@microsoft.com>
> ---
>   rust/kernel/alloc/vec_ext.rs | 158 +++++++++++++++++++++++++++++++----
>   rust/kernel/error.rs         |  11 +--
>   rust/kernel/lib.rs           |   1 -
>   rust/kernel/str.rs           |   6 +-
>   rust/kernel/types.rs         |   4 +-
>   samples/rust/rust_minimal.rs |   6 +-
>   6 files changed, 152 insertions(+), 34 deletions(-)

Reviewed-by: Benno Lossin <benno.lossin@proton.me>

[...]

>   impl<T> VecExt<T> for Vec<T> {
> -    fn try_with_capacity(capacity: usize) -> Result<Self, TryReserveErro=
r> {
> +    fn with_capacity(capacity: usize, flags: Flags) -> Result<Self, Allo=
cError> {
>           let mut v =3D Vec::new();
> -        v.try_reserve(capacity)?;
> +        <Self as VecExt<_>>::reserve(&mut v, capacity, flags)?;
>           Ok(v)
>       }
>=20
> -    fn try_push(&mut self, v: T) -> Result<(), TryReserveError> {
> -        if let Err(retry) =3D self.push_within_capacity(v) {
> -            self.try_reserve(1)?;
> -            let _ =3D self.push_within_capacity(retry);
> -        }
> +    fn push(&mut self, v: T, flags: Flags) -> Result<(), AllocError> {
> +        <Self as VecExt<_>>::reserve(self, 1, flags)?;
> +        let s =3D self.spare_capacity_mut();
> +        s[0].write(v);

I haven't checked the codegen, but this could result in an additional
bound check. We don't need to care about this now, but at some point we
should do a performance test. There are probably other things that I
have missed.

--=20
Cheers,
Benno

> +
> +        // SAFETY: We just initialised the first spare entry, so it is s=
afe to increase the length
> +        // by 1. We also know that the new length is <=3D capacity becau=
se of the previous call to
> +        // `reserve` above.
> +        unsafe { self.set_len(self.len() + 1) };
>           Ok(())
>       }

[...]