On Mon, Jan 29, 2024 at 6:59 PM Matthew Wilcox <willy@xxxxxxxxxxxxx> wrote: > > On Wed, Jan 24, 2024 at 11:20:23AM +0000, Alice Ryhl wrote: > > Adds a new struct called `Page` that wraps a pointer to `struct page`. > > This struct is assumed to hold ownership over the page, so that Rust > > code can allocate and manage pages directly. > > OK ... Thank you for taking your time to review my wrappers! > > This patch only adds support for pages of order zero, as that is all > > Rust Binder needs. However, it is written to make it easy to add support > > for higher-order pages in the future. To do that, you would add a const > > generic parameter to `Page` that specifies the order. Most of the > > methods do not need to be adjusted, as the logic for dealing with > > mapping multiple pages at once can be isolated to just the > > `with_pointer_into_page` method. Finally, the struct can be renamed to > > `Pages<ORDER>`, and the type alias `Page = Pages<0>` can be introduced. > > This description concerns me because it reads like you're not keeping > up with the current thinking in MM about what pages are and how we're > improving the type hierarchy. As in, we're creating one instead of > allowing the current mish-mash of absolutely everything to continue. That's very possible. I have a good understanding about how C binder interacts with pages, but I don't know too much about the abstractions that Binder is not using. > Are you the right person to ask about the operations that Binder does > with a page so we can figure out where it fits in the type hierarchy? I can definitely answer questions about that. If we can find another abstraction that is not too far away from what we are doing today, then I am open to looking into whether we can do that instead of the current approach. However, I want to avoid large deviations from C Binder, at least before I get Rust binder into the kernel tree. A short overview of what Binder does: * Every process will mmap a region of memory. This memory region will contain the data for all incoming transactions sent to that process. Only the kernel can modify these pages. * Binder has a data structure that keeps track of where the allocations in this mmap'd region are. It has functionality to find an open interval of a given size (so it's essentially an allocator). This is called the "range allocator". * The pages in this region are allocated lazily whenever the range allocator starts using the page. * When the range allocator stops using a page, it is marked as unused and put on an LRU list. * There's a custom shrinker that can free pages not currently used by any allocation. So when process A sends a transaction to process B using the appropriate ioctl, process A will go into the range allocator for process B and reserve a range for the transaction that A is sending. If any pages in the resulting range are missing, then new pages are allocated, and process A will vm_insert_page them into process B's mmap. Then, process A will map the page with kmap_local_page, and use copy_from_user to copy data *directly* from A's userspace into a page in process B's address space. Note that transactions don't have uniform sizes. Think of them as arbitrary buffers provided by userspace. They will generally not be larger than a few hundred bytes each, but larger transactions are possible. The mmap for a process is usually 4 MB. The biggest user of Page is here in the RFC: https://lore.kernel.org/rust-for-linux/20231101-rust-binder-v1-19-08ba9197f637@xxxxxxxxxx/ The range allocator is defined here: https://lore.kernel.org/rust-for-linux/20231101-rust-binder-v1-6-08ba9197f637@xxxxxxxxxx/ > > Rust Binder needs to manage pages directly as that is how transactions > > are delivered: Each process has an mmap'd region for incoming > > transactions. When an incoming transaction arrives, the Binder driver > > will choose a region in the mmap, allocate and map the relevant pages > > manually, and copy the incoming transaction directly into the page. This > > architecture allows the driver to copy transactions directly from the > > address space of one process to another, without an intermediate copy > > to a kernel buffer. > > Everything about this says "This is what a first year comp sci student > thinks will be fast". Oh well, the thinking here isn't your fault. Ultimately, I am just replicating what C Binder does. I had a long discussion with Liam Howlett at Plumbers where we discussed various alternatives to the hand-rolled stuff that Binder does. Liam thought that we may be able to replace the entire thing with maple trees. These are things that I definitely want to experiment with, but I am reluctant to try replacing the entire thing with a maple tree, at least until I get Rust Binder into the kernel tree. In general, there are many places in Binder where we are hand-rolling something that has an alternative elsewhere in the kernel, but replacing them is not always trivial. The hand-rolled versions often have Binder-specific optimizations that make it a regression to replace it with the general thing. > > @@ -127,6 +129,24 @@ int rust_helper_signal_pending(struct task_struct *t) > > } > > EXPORT_SYMBOL_GPL(rust_helper_signal_pending); > > > > +struct page *rust_helper_alloc_pages(gfp_t gfp_mask, unsigned int order) > > +{ > > + return alloc_pages(gfp_mask, order); > > +} > > +EXPORT_SYMBOL_GPL(rust_helper_alloc_pages); > > + > > +void *rust_helper_kmap_local_page(struct page *page) > > +{ > > + return kmap_local_page(page); > > +} > > +EXPORT_SYMBOL_GPL(rust_helper_kmap_local_page); > > + > > +void rust_helper_kunmap_local(const void *addr) > > +{ > > + kunmap_local(addr); > > +} > > +EXPORT_SYMBOL_GPL(rust_helper_kunmap_local); > > I remain opposed to all these fidgetty little helpers. Particularly > when they're noops on machines without HIGHMEM, which is ~all of them. I don't disagree with you, but there's not much I can do about them. I can wrap them in #ifdef HIGHMEM if they are no-ops or exported without HIGHMEM? > > +/// A bitwise shift for the page size. > > +pub const PAGE_SHIFT: usize = bindings::PAGE_SHIFT as usize; > > Does PAGE_SHIFT really need to be as large as 'usize'? If it's more > than 63 by the time I retire, I'll be shocked. If it's more than 127 > by the time I die, I'll be even more shocked. And it won't get to 255 > by the heat death of the universe. Rust usually requires that both operands to an integer operation are of the same integer type, requiring explicit conversions if that is not the case. That is why I am using usize here. However, it seems like Rust doesn't actually require that for the << operator, so I guess it doesn't matter much for this particular constant. > > +/// A bitwise mask for the page size. > > +pub const PAGE_MASK: usize = PAGE_SIZE - 1; > > Are you trying to get somebody killed? > > include/asm-generic/page.h:#define PAGE_MASK (~(PAGE_SIZE-1)) > > Defining PAGE_MASK to be the opposite set of bits in C and Rust is > going to bite us all day every day for a decade. Oops, that's embarrassing. Thank you for catching that. I'll make sure to change it. > > +impl Page { > > + /// Allocates a new set of contiguous pages. > > + pub fn new() -> Result<Self, AllocError> { > > + // SAFETY: These are the correct arguments to allocate a single page. > > + let page = unsafe { > > + bindings::alloc_pages( > > + bindings::GFP_KERNEL | bindings::__GFP_ZERO | bindings::__GFP_HIGHMEM, > > + 0, > > + ) > > + }; > > This feels too Binder-specific to be 'Page'. Pages are not necessarily > allocated with GFP_HIGHMEM, nor are they necessarily zeroed. Maybe you > want a BinderPage type? We can add a constructor that takes a flag argument, so this type doesn't necessarily have to be tied to those specific arguments. Alice
