On Wed, Oct 18, 2023 at 09:25:08AM -0300, Wedson Almeida Filho wrote:
> +void *rust_helper_kmap(struct page *page)
> +{
> + return kmap(page);
> +}
> +EXPORT_SYMBOL_GPL(rust_helper_kmap);
> +
> +void rust_helper_kunmap(struct page *page)
> +{
> + kunmap(page);
> +}
> +EXPORT_SYMBOL_GPL(rust_helper_kunmap);
I'm not thrilled by exposing kmap()/kunmap() to Rust code. The vast
majority of code really only needs kmap_local_*() / kunmap_local().
Can you elaborate on why you need the old kmap() in new Rust code?
> +void rust_helper_folio_set_error(struct folio *folio)
> +{
> + folio_set_error(folio);
> +}
> +EXPORT_SYMBOL_GPL(rust_helper_folio_set_error);
I'm trying to get rid of the error flag. Can you share the situations
in which you've needed the error flag? Or is it just copying existing
practices?
> + /// Returns the byte position of this folio in its file.
> + pub fn pos(&self) -> i64 {
> + // SAFETY: The folio is valid because the shared reference implies a non-zero refcount.
> + unsafe { bindings::folio_pos(self.0.get()) }
> + }
I think it's a mistake to make file positions an i64. I estimate 64
bits will not be enough by 2035-2040. We should probably have a numeric
type which is i64 on 32-bit and isize on other CPUs (I also project
64-bit pointers will be insufficient by 2035-2040 and so we will have
128-bit pointers around the same time, so we're not going to need i128
file offsets with i64 pointers).
> +/// A [`Folio`] that has a single reference to it.
> +pub struct UniqueFolio(pub(crate) ARef<Folio>);
How do we know it only has a single reference? Do you mean "has at
least one reference"? Or am I confusing Rust's notion of a reference
with Linux's notion of a reference?
> +impl UniqueFolio {
> + /// Maps the contents of a folio page into a slice.
> + pub fn map_page(&self, page_index: usize) -> Result<MapGuard<'_>> {
> + if page_index >= self.0.size() / bindings::PAGE_SIZE {
> + return Err(EDOM);
> + }
> +
> + // SAFETY: We just checked that the index is within bounds of the folio.
> + let page = unsafe { bindings::folio_page(self.0 .0.get(), page_index) };
> +
> + // SAFETY: `page` is valid because it was returned by `folio_page` above.
> + let ptr = unsafe { bindings::kmap(page) };
Surely this can be:
let ptr = unsafe { bindings::kmap_local_folio(folio, page_index * PAGE_SIZE) };
> + // SAFETY: We just mapped `ptr`, so it's valid for read.
> + let data = unsafe { core::slice::from_raw_parts(ptr.cast::<u8>(), bindings::PAGE_SIZE) };
Can we hide away the "if this isn't a HIGHMEM system, this maps to the
end of the folio, but if it is, it only maps to the end of the page"
problem here?
