Message-ID: <ZsFEpjvE9osKDb3b@boqun-archlinux>
Date: Sat, 17 Aug 2024 17:47:34 -0700
From: Boqun Feng <boqun.feng@...il.com>
To: Gary Guo <gary@...yguo.net>
Cc: Miguel Ojeda <ojeda@...nel.org>, Alex Gaynor <alex.gaynor@...il.com>,
	Wedson Almeida Filho <wedsonaf@...il.com>,
	Björn Roy Baron <bjorn3_gh@...tonmail.com>,
	Benno Lossin <benno.lossin@...ton.me>,
	Andreas Hindborg <a.hindborg@...sung.com>,
	Alice Ryhl <aliceryhl@...gle.com>,
	Dirk Behme <dirk.behme@...bosch.com>,
	rust-for-linux@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] kbuild: rust: auto generate rust helper exports

On Sat, Aug 17, 2024 at 05:51:32PM +0100, Gary Guo wrote:
> This removes the need for explicit exporting all symbols.
> 
> Generate helper exports similar to what's currently done for Rust
> crates. These helpers are exclusively called from within Rust code
> and therefore can be treated similar as other Rust symbols.
> 
> Signed-off-by: Gary Guo <gary@...yguo.net>

Reviewed-by: Boqun Feng <boqun.feng@...il.com>

This also passed my test script (x86, arm64 and riscv build and kunit
tests). 

Miguel, I think it makes more sense if we take this one and the
helpers.c split one as early as possible, given they are treewide
changes ;-)

Regards,
Boqun

> ---
> 
> Notes:
>     Changes in v2:
>     - Split from the helper inline RFC [1]
>     - Rebased on Andreas' split helper patch [2]
>     - Moved comments about helper export to Makefile
>     
>     [1]: https://lore.kernel.org/rust-for-linux/20240529202817.3641974-2-gary@garyguo.net/
>     [2]: https://lore.kernel.org/rust-for-linux/20240815103016.2771842-1-nmi@metaspace.dk/
> 
>  rust/Makefile            | 16 ++++++++++++++--
>  rust/exports.c           |  1 +
>  rust/helpers/README.md   | 12 ------------
>  rust/helpers/blk.c       |  2 --
>  rust/helpers/bug.c       |  1 -
>  rust/helpers/build_bug.c |  1 -
>  rust/helpers/err.c       |  3 ---
>  rust/helpers/kunit.c     |  1 -
>  rust/helpers/mutex.c     |  1 -
>  rust/helpers/page.c      |  5 -----
>  rust/helpers/refcount.c  |  3 ---
>  rust/helpers/signal.c    |  1 -
>  rust/helpers/slab.c      |  1 -
>  rust/helpers/spinlock.c  |  3 ---
>  rust/helpers/task.c      |  3 ---
>  rust/helpers/uaccess.c   |  2 --
>  rust/helpers/wait.c      |  1 -
>  rust/helpers/workqueue.c |  1 -
>  18 files changed, 15 insertions(+), 43 deletions(-)
> 
> diff --git a/rust/Makefile b/rust/Makefile
> index 967da95ba8b0..3171a9021494 100644
> --- a/rust/Makefile
> +++ b/rust/Makefile
> @@ -16,8 +16,8 @@ no-clean-files += libmacros.so
>  
>  always-$(CONFIG_RUST) += bindings/bindings_generated.rs bindings/bindings_helpers_generated.rs
>  obj-$(CONFIG_RUST) += alloc.o bindings.o kernel.o
> -always-$(CONFIG_RUST) += exports_alloc_generated.h exports_bindings_generated.h \
> -    exports_kernel_generated.h
> +always-$(CONFIG_RUST) += exports_alloc_generated.h exports_helpers_generated.h \
> +    exports_bindings_generated.h exports_kernel_generated.h
>  
>  always-$(CONFIG_RUST) += uapi/uapi_generated.rs
>  obj-$(CONFIG_RUST) += uapi.o
> @@ -313,6 +313,18 @@ $(obj)/exports_core_generated.h: $(obj)/core.o FORCE
>  $(obj)/exports_alloc_generated.h: $(obj)/alloc.o FORCE
>  	$(call if_changed,exports)
>  
> +# Even though Rust kernel modules should never use the bindings directly,
> +# symbols from bindings crate and the C helpers need to be exported because
> +# Rust generics and inlined functions may not get their code generated in the
> +# crate where they are defined. Other helpers, called from non-inline
> +# functions, may not be exported, in principle. However, in general, the Rust
> +# compiler does not guarantee codegen will be performed for a non-inline
> +# function either. Therefore, we export all symbols from helpers and bindings.
> +# In the future, this may be revisited to reduce the number of exports after
> +# the compiler is informed about the places codegen is required.
> +$(obj)/exports_helpers_generated.h: $(obj)/helpers.o FORCE
> +	$(call if_changed,exports)
> +
>  $(obj)/exports_bindings_generated.h: $(obj)/bindings.o FORCE
>  	$(call if_changed,exports)
>  
> diff --git a/rust/exports.c b/rust/exports.c
> index 3803c21d1403..e5695f3b45b7 100644
> --- a/rust/exports.c
> +++ b/rust/exports.c
> @@ -17,6 +17,7 @@
>  
>  #include "exports_core_generated.h"
>  #include "exports_alloc_generated.h"
> +#include "exports_helpers_generated.h"
>  #include "exports_bindings_generated.h"
>  #include "exports_kernel_generated.h"
>  
> diff --git a/rust/helpers/README.md b/rust/helpers/README.md
> index 940b24bb552d..f2be545ef946 100644
> --- a/rust/helpers/README.md
> +++ b/rust/helpers/README.md
> @@ -3,15 +3,3 @@
>  Non-trivial C macros cannot be used in Rust. Similarly, inlined C functions
>  cannot be called either. The files in this directory explicitly create functions
>  ("helpers") that wrap those so that they can be called from Rust.
> -
> -Even though Rust kernel modules should never use the bindings directly, some of
> -these helpers need to be exported because Rust generics and inlined functions
> -may not get their code generated in the crate where they are defined. Other
> -helpers, called from non-inline functions, may not be exported, in principle.
> -However, in general, the Rust compiler does not guarantee codegen will be
> -performed for a non-inline function either. Therefore, this file exports all the
> -helpers. In the future, this may be revisited to reduce the number of exports
> -after the compiler is informed about the places codegen is required.
> -
> -All symbols are exported as GPL-only to guarantee no GPL-only feature is
> -accidentally exposed.
> diff --git a/rust/helpers/blk.c b/rust/helpers/blk.c
> index d99c965eb59b..cc9f4e6a2d23 100644
> --- a/rust/helpers/blk.c
> +++ b/rust/helpers/blk.c
> @@ -7,10 +7,8 @@ void *rust_helper_blk_mq_rq_to_pdu(struct request *rq)
>  {
>  	return blk_mq_rq_to_pdu(rq);
>  }
> -EXPORT_SYMBOL_GPL(rust_helper_blk_mq_rq_to_pdu);
>  
>  struct request *rust_helper_blk_mq_rq_from_pdu(void *pdu)
>  {
>  	return blk_mq_rq_from_pdu(pdu);
>  }
> -EXPORT_SYMBOL_GPL(rust_helper_blk_mq_rq_from_pdu);
> diff --git a/rust/helpers/bug.c b/rust/helpers/bug.c
> index e2afbad23dcd..e2d13babc737 100644
> --- a/rust/helpers/bug.c
> +++ b/rust/helpers/bug.c
> @@ -6,4 +6,3 @@ __noreturn void rust_helper_BUG(void)
>  {
>  	BUG();
>  }
> -EXPORT_SYMBOL_GPL(rust_helper_BUG);
> diff --git a/rust/helpers/build_bug.c b/rust/helpers/build_bug.c
> index f3106f248485..e994f7b5928c 100644
> --- a/rust/helpers/build_bug.c
> +++ b/rust/helpers/build_bug.c
> @@ -7,4 +7,3 @@ const char *rust_helper_errname(int err)
>  {
>  	return errname(err);
>  }
> -EXPORT_SYMBOL_GPL(rust_helper_errname);
> diff --git a/rust/helpers/err.c b/rust/helpers/err.c
> index fba4e0be64f5..be3d45ef78a2 100644
> --- a/rust/helpers/err.c
> +++ b/rust/helpers/err.c
> @@ -7,16 +7,13 @@ __force void *rust_helper_ERR_PTR(long err)
>  {
>  	return ERR_PTR(err);
>  }
> -EXPORT_SYMBOL_GPL(rust_helper_ERR_PTR);
>  
>  bool rust_helper_IS_ERR(__force const void *ptr)
>  {
>  	return IS_ERR(ptr);
>  }
> -EXPORT_SYMBOL_GPL(rust_helper_IS_ERR);
>  
>  long rust_helper_PTR_ERR(__force const void *ptr)
>  {
>  	return PTR_ERR(ptr);
>  }
> -EXPORT_SYMBOL_GPL(rust_helper_PTR_ERR);
> diff --git a/rust/helpers/kunit.c b/rust/helpers/kunit.c
> index 905e4ff4424a..9d725067eb3b 100644
> --- a/rust/helpers/kunit.c
> +++ b/rust/helpers/kunit.c
> @@ -7,4 +7,3 @@ struct kunit *rust_helper_kunit_get_current_test(void)
>  {
>  	return kunit_get_current_test();
>  }
> -EXPORT_SYMBOL_GPL(rust_helper_kunit_get_current_test);
> diff --git a/rust/helpers/mutex.c b/rust/helpers/mutex.c
> index 29fd141c387d..200db7e6279f 100644
> --- a/rust/helpers/mutex.c
> +++ b/rust/helpers/mutex.c
> @@ -7,4 +7,3 @@ void rust_helper_mutex_lock(struct mutex *lock)
>  {
>  	mutex_lock(lock);
>  }
> -EXPORT_SYMBOL_GPL(rust_helper_mutex_lock);
> diff --git a/rust/helpers/page.c b/rust/helpers/page.c
> index b3280c80b283..b3f2b8fbf87f 100644
> --- a/rust/helpers/page.c
> +++ b/rust/helpers/page.c
> @@ -7,18 +7,13 @@ 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);
> -
> -
> diff --git a/rust/helpers/refcount.c b/rust/helpers/refcount.c
> index 13ab64805f77..f47afc148ec3 100644
> --- a/rust/helpers/refcount.c
> +++ b/rust/helpers/refcount.c
> @@ -7,16 +7,13 @@ refcount_t rust_helper_REFCOUNT_INIT(int n)
>  {
>  	return (refcount_t)REFCOUNT_INIT(n);
>  }
> -EXPORT_SYMBOL_GPL(rust_helper_REFCOUNT_INIT);
>  
>  void rust_helper_refcount_inc(refcount_t *r)
>  {
>  	refcount_inc(r);
>  }
> -EXPORT_SYMBOL_GPL(rust_helper_refcount_inc);
>  
>  bool rust_helper_refcount_dec_and_test(refcount_t *r)
>  {
>  	return refcount_dec_and_test(r);
>  }
> -EXPORT_SYMBOL_GPL(rust_helper_refcount_dec_and_test);
> diff --git a/rust/helpers/signal.c b/rust/helpers/signal.c
> index d44e8096b8a9..63c407f80c26 100644
> --- a/rust/helpers/signal.c
> +++ b/rust/helpers/signal.c
> @@ -7,4 +7,3 @@ int rust_helper_signal_pending(struct task_struct *t)
>  {
>  	return signal_pending(t);
>  }
> -EXPORT_SYMBOL_GPL(rust_helper_signal_pending);
> diff --git a/rust/helpers/slab.c b/rust/helpers/slab.c
> index 3e0a1a173d8a..f043e087f9d6 100644
> --- a/rust/helpers/slab.c
> +++ b/rust/helpers/slab.c
> @@ -7,4 +7,3 @@ rust_helper_krealloc(const void *objp, size_t new_size, gfp_t flags)
>  {
>  	return krealloc(objp, new_size, flags);
>  }
> -EXPORT_SYMBOL_GPL(rust_helper_krealloc);
> diff --git a/rust/helpers/spinlock.c b/rust/helpers/spinlock.c
> index 04fd8ddb4986..acc1376b833c 100644
> --- a/rust/helpers/spinlock.c
> +++ b/rust/helpers/spinlock.c
> @@ -12,16 +12,13 @@ void rust_helper___spin_lock_init(spinlock_t *lock, const char *name,
>  	spin_lock_init(lock);
>  #endif
>  }
> -EXPORT_SYMBOL_GPL(rust_helper___spin_lock_init);
>  
>  void rust_helper_spin_lock(spinlock_t *lock)
>  {
>  	spin_lock(lock);
>  }
> -EXPORT_SYMBOL_GPL(rust_helper_spin_lock);
>  
>  void rust_helper_spin_unlock(spinlock_t *lock)
>  {
>  	spin_unlock(lock);
>  }
> -EXPORT_SYMBOL_GPL(rust_helper_spin_unlock);
> diff --git a/rust/helpers/task.c b/rust/helpers/task.c
> index b176c347f0d4..7ac789232d11 100644
> --- a/rust/helpers/task.c
> +++ b/rust/helpers/task.c
> @@ -7,16 +7,13 @@ struct task_struct *rust_helper_get_current(void)
>  {
>  	return current;
>  }
> -EXPORT_SYMBOL_GPL(rust_helper_get_current);
>  
>  void rust_helper_get_task_struct(struct task_struct *t)
>  {
>  	get_task_struct(t);
>  }
> -EXPORT_SYMBOL_GPL(rust_helper_get_task_struct);
>  
>  void rust_helper_put_task_struct(struct task_struct *t)
>  {
>  	put_task_struct(t);
>  }
> -EXPORT_SYMBOL_GPL(rust_helper_put_task_struct);
> diff --git a/rust/helpers/uaccess.c b/rust/helpers/uaccess.c
> index 3d004ac1c180..f49076f813cd 100644
> --- a/rust/helpers/uaccess.c
> +++ b/rust/helpers/uaccess.c
> @@ -7,11 +7,9 @@ unsigned long rust_helper_copy_from_user(void *to, const void __user *from,
>  {
>  	return copy_from_user(to, from, n);
>  }
> -EXPORT_SYMBOL_GPL(rust_helper_copy_from_user);
>  
>  unsigned long rust_helper_copy_to_user(void __user *to, const void *from,
>  				       unsigned long n)
>  {
>  	return copy_to_user(to, from, n);
>  }
> -EXPORT_SYMBOL_GPL(rust_helper_copy_to_user);
> diff --git a/rust/helpers/wait.c b/rust/helpers/wait.c
> index bf361f40c7cb..c7336bbf2750 100644
> --- a/rust/helpers/wait.c
> +++ b/rust/helpers/wait.c
> @@ -7,4 +7,3 @@ void rust_helper_init_wait(struct wait_queue_entry *wq_entry)
>  {
>  	init_wait(wq_entry);
>  }
> -EXPORT_SYMBOL_GPL(rust_helper_init_wait);
> diff --git a/rust/helpers/workqueue.c b/rust/helpers/workqueue.c
> index 12e2ee66aa4f..f59427acc323 100644
> --- a/rust/helpers/workqueue.c
> +++ b/rust/helpers/workqueue.c
> @@ -13,4 +13,3 @@ void rust_helper_init_work_with_key(struct work_struct *work, work_func_t func,
>  	INIT_LIST_HEAD(&work->entry);
>  	work->func = func;
>  }
> -EXPORT_SYMBOL_GPL(rust_helper_init_work_with_key);
> 
> base-commit: 7c626ce4bae1ac14f60076d00eafe71af30450ba
> prerequisite-patch-id: 97916a5ce500a204e6e5b4c5726691f080d6fbcc
> -- 
> 2.44.1
> 

Powered by blists - more mailing lists