2022-08-06 10:50:31

by Konstantin Shelekhin

[permalink] [raw]
Subject: Re: [PATCH v9 12/27] rust: add `kernel` crate

> +unsafe impl GlobalAlloc for KernelAllocator {
> + unsafe fn alloc(&self, layout: Layout) -> *mut u8 {
> + // `krealloc()` is used instead of `kmalloc()` because the latter is
> + // an inline function and cannot be bound to as a result.
> + unsafe { bindings::krealloc(ptr::null(), layout.size(), bindings::GFP_KERNEL) as *mut u8 }
> + }
> +
> + unsafe fn dealloc(&self, ptr: *mut u8, _layout: Layout) {
> + unsafe {
> + bindings::kfree(ptr as *const core::ffi::c_void);
> + }
> + }
> +}

I sense possible problems here. It's common for a kernel code to pass
flags during memory allocations.

For example:

struct bio *bio;

for (...) {
bio = bio_alloc_bioset(bdev, nr_vecs, opf, GFP_NOIO, bs);
if (!bio)
return -ENOMEM;
}

Without GFP_NOIO we can run into a deadlock, because the kernel will try
give us free memory by flushing the dirty pages and we need the memory
to actually do it and boom, deadlock.

Or we can be allocating some structs under spinlock (yeah, that happens too):

struct efc_vport *vport;

spin_lock_irqsave(...);
vport = kzalloc(sizeof(*vport), GFP_ATOMIC);
if (!vport) {
spin_unlock_irqrestore(...);
return NULL;
}
spin_unlock(...);

Same can (and probably will) happen to e.g. Vec elements. So some form
of flags passing should be supported in try_* variants:

let mut vec = Vec::try_new(GFP_ATOMIC)?;

vec.try_push(GFP_ATOMIC, 1)?;
vec.try_push(GFP_ATOMIC, 2)?;
vec.try_push(GFP_ATOMIC, 3)?;


2022-08-06 11:54:22

by Miguel Ojeda

[permalink] [raw]
Subject: Re: [PATCH v9 12/27] rust: add `kernel` crate

On Sat, Aug 6, 2022 at 12:25 PM Konstantin Shelekhin
<[email protected]> wrote:
>
> I sense possible problems here. It's common for a kernel code to pass
> flags during memory allocations.

Yes, of course. We will support this, but how exactly it will look
like, to what extent upstream Rust's `alloc` could support our use
cases, etc. has been on discussion for a long time.

For instance, see https://github.com/Rust-for-Linux/linux/pull/815 for
a potential extension trait approach with no allocator carried on the
type that Andreas wrote after a discussion in the last informal call:

let a = Box::try_new_atomic(101)?;

Cheers,
Miguel

2022-08-06 12:17:52

by Konstantin Shelekhin

[permalink] [raw]
Subject: Re: [PATCH v9 12/27] rust: add `kernel` crate

On Sat, Aug 06, 2022 at 01:22:52PM +0200, Miguel Ojeda wrote:
> > I sense possible problems here. It's common for a kernel code to pass
> > flags during memory allocations.
>
> Yes, of course. We will support this, but how exactly it will look
> like, to what extent upstream Rust's `alloc` could support our use
> cases, etc. has been on discussion for a long time.
>
> For instance, see https://github.com/Rust-for-Linux/linux/pull/815 for
> a potential extension trait approach with no allocator carried on the
> type that Andreas wrote after a discussion in the last informal call:
>
> let a = Box::try_new_atomic(101)?;

IMO it's just easier to always pass flags like this:

let a = Box::try_new(GFP_KERNEL | GFP_DMA, 101)?;

But if allocate_with_flags() will be somehow present in the API that's
just what we need.

P.S. Thanks for a quick reply!

2022-08-06 15:22:24

by Matthew Wilcox

[permalink] [raw]
Subject: Re: [PATCH v9 12/27] rust: add `kernel` crate

On Sat, Aug 06, 2022 at 01:22:52PM +0200, Miguel Ojeda wrote:
> On Sat, Aug 6, 2022 at 12:25 PM Konstantin Shelekhin
> <[email protected]> wrote:
> >
> > I sense possible problems here. It's common for a kernel code to pass
> > flags during memory allocations.
>
> Yes, of course. We will support this, but how exactly it will look
> like, to what extent upstream Rust's `alloc` could support our use
> cases, etc. has been on discussion for a long time.
>
> For instance, see https://github.com/Rust-for-Linux/linux/pull/815 for
> a potential extension trait approach with no allocator carried on the
> type that Andreas wrote after a discussion in the last informal call:
>
> let a = Box::try_new_atomic(101)?;

Something I've been wondering about for a while is ...

struct task_struct {
...
+ gfp_t gfp_flags;
...
};

We've already done some work towards this with the scoped allocation
API for NOIO and NOFS, but having spin_lock() turn current->gfp_flags
into GFP_ATOMIC might not be the worst idea in the world.