rust: add this_module macro

[PATCH] rust: add this_module macro

Posted by Martin Rodriguez Reboredo 2 years, 7 months ago

Adds a Rust equivalent to the handy THIS_MODULE macro from C.

Signed-off-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com>
---
 rust/kernel/lib.rs | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index e0b0e953907d..afb6b0390426 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -80,6 +80,18 @@ impl ThisModule {
     }
 }
 
+/// Returns the current module.
+#[macro_export]
+macro_rules! this_module {
+    () => {
+        if cfg!(MODULE) {
+            Some(unsafe { $crate::ThisModule::from_ptr(&mut $crate::bindings::__this_module) })
+        } else {
+            None
+        }
+    };
+}
+
 #[cfg(not(any(testlib, test)))]
 #[panic_handler]
 fn panic(info: &core::panic::PanicInfo<'_>) -> ! {
-- 
2.39.1

Re: [PATCH] rust: add this_module macro

Posted by Greg KH 2 years, 7 months ago

On Tue, Jan 31, 2023 at 10:08:41AM -0300, Martin Rodriguez Reboredo wrote:
> Adds a Rust equivalent to the handy THIS_MODULE macro from C.
> 
> Signed-off-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com>
> ---
>  rust/kernel/lib.rs | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> index e0b0e953907d..afb6b0390426 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -80,6 +80,18 @@ impl ThisModule {
>      }
>  }
>  
> +/// Returns the current module.
> +#[macro_export]
> +macro_rules! this_module {
> +    () => {
> +        if cfg!(MODULE) {
> +            Some(unsafe { $crate::ThisModule::from_ptr(&mut $crate::bindings::__this_module) })
> +        } else {
> +            None
> +        }
> +    };
> +}

While this is handy, what exactly will it be used for?  The C
wrappers/shim/whatever should probably handle this for you already when
you save this pointer into a structure right?

Surely you aren't trying to increment your own module's reference count,
right?  That just doesn't work :)

thanks,

greg k-h

Re: [PATCH] rust: add this_module macro

Posted by Martin Rodriguez Reboredo 2 years, 7 months ago

On Tue, Jan 31, 2023 at 02:42:08PM +0100, Greg KH wrote:
>On Tue, Jan 31, 2023 at 10:08:41AM -0300, Martin Rodriguez Reboredo wrote:
>> Adds a Rust equivalent to the handy THIS_MODULE macro from C.
>> 
>> Signed-off-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com>
>> ---
>>  rust/kernel/lib.rs | 12 ++++++++++++
>>  1 file changed, 12 insertions(+)
>> 
>> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
>> index e0b0e953907d..afb6b0390426 100644
>> --- a/rust/kernel/lib.rs
>> +++ b/rust/kernel/lib.rs
>> @@ -80,6 +80,18 @@ impl ThisModule {
>>      }
>>  }
>>  
>> +/// Returns the current module.
>> +#[macro_export]
>> +macro_rules! this_module {
>> +    () => {
>> +        if cfg!(MODULE) {
>> +            Some(unsafe { $crate::ThisModule::from_ptr(&mut $crate::bindings::__this_module) })
>> +        } else {
>> +            None
>> +        }
>> +    };
>> +}
>
>While this is handy, what exactly will it be used for?  The C
>wrappers/shim/whatever should probably handle this for you already when
>you save this pointer into a structure right?
>
>Surely you aren't trying to increment your own module's reference count,
>right?  That just doesn't work :)
>
>thanks,
>
>greg k-h

This was meant for setting the owner field of a file_operations struct
or the cra_owner field of crypto_alg and many other structs.

I know that increfing a module without a good reason is dead dumb, so
I'm not trying to send things in a downwards spiral. @@@

And yes, I should have mentioned that in the commit message, but I let
slip that detail.

Re: [PATCH] rust: add this_module macro

Posted by Greg KH 2 years, 7 months ago

On Tue, Jan 31, 2023 at 12:07:45PM -0300, Martin Rodriguez Reboredo wrote:
> On Tue, Jan 31, 2023 at 02:42:08PM +0100, Greg KH wrote:
> >On Tue, Jan 31, 2023 at 10:08:41AM -0300, Martin Rodriguez Reboredo wrote:
> >> Adds a Rust equivalent to the handy THIS_MODULE macro from C.
> >> 
> >> Signed-off-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com>
> >> ---
> >>  rust/kernel/lib.rs | 12 ++++++++++++
> >>  1 file changed, 12 insertions(+)
> >> 
> >> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> >> index e0b0e953907d..afb6b0390426 100644
> >> --- a/rust/kernel/lib.rs
> >> +++ b/rust/kernel/lib.rs
> >> @@ -80,6 +80,18 @@ impl ThisModule {
> >>      }
> >>  }
> >>  
> >> +/// Returns the current module.
> >> +#[macro_export]
> >> +macro_rules! this_module {
> >> +    () => {
> >> +        if cfg!(MODULE) {
> >> +            Some(unsafe { $crate::ThisModule::from_ptr(&mut $crate::bindings::__this_module) })
> >> +        } else {
> >> +            None
> >> +        }
> >> +    };
> >> +}
> >
> >While this is handy, what exactly will it be used for?  The C
> >wrappers/shim/whatever should probably handle this for you already when
> >you save this pointer into a structure right?
> >
> >Surely you aren't trying to increment your own module's reference count,
> >right?  That just doesn't work :)
> >
> >thanks,
> >
> >greg k-h
> 
> This was meant for setting the owner field of a file_operations struct
> or the cra_owner field of crypto_alg and many other structs.

But shouldn't the macro kernel::declare_file_operations() do this for
you automagically?  You should never have to manually say "this module!"
to any structure or function call if we do things right.

Yes, many "old school" structures in the kernel do this, but we have
learned from the 1990's, see the fun wrappers around simple things like
usb_register_driver(); as an example of how the driver author themselves
should never see a module pointer anywhere.

> I know that increfing a module without a good reason is dead dumb, so
> I'm not trying to send things in a downwards spiral. @@@

That's good, but let's not add housekeeping requirements when we do not
have to do so if at all possible please.

thanks,

greg k-h

Re: [PATCH] rust: add this_module macro

Posted by Martin Rodriguez Reboredo 2 years, 7 months ago

On Tue, Jan 31, 2023 at 04:15:51PM +0100, Greg KH wrote:
>On Tue, Jan 31, 2023 at 12:07:45PM -0300, Martin Rodriguez Reboredo wrote:
>> On Tue, Jan 31, 2023 at 02:42:08PM +0100, Greg KH wrote:
>> >On Tue, Jan 31, 2023 at 10:08:41AM -0300, Martin Rodriguez Reboredo wrote:
>> >> Adds a Rust equivalent to the handy THIS_MODULE macro from C.
>> >> 
>> >> Signed-off-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com>
>> >> ---
>> >>  rust/kernel/lib.rs | 12 ++++++++++++
>> >>  1 file changed, 12 insertions(+)
>> >> 
>> >> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
>> >> index e0b0e953907d..afb6b0390426 100644
>> >> --- a/rust/kernel/lib.rs
>> >> +++ b/rust/kernel/lib.rs
>> >> @@ -80,6 +80,18 @@ impl ThisModule {
>> >>      }
>> >>  }
>> >>  
>> >> +/// Returns the current module.
>> >> +#[macro_export]
>> >> +macro_rules! this_module {
>> >> +    () => {
>> >> +        if cfg!(MODULE) {
>> >> +            Some(unsafe { $crate::ThisModule::from_ptr(&mut $crate::bindings::__this_module) })
>> >> +        } else {
>> >> +            None
>> >> +        }
>> >> +    };
>> >> +}
>> >
>> >While this is handy, what exactly will it be used for?  The C
>> >wrappers/shim/whatever should probably handle this for you already when
>> >you save this pointer into a structure right?
>> >
>> >Surely you aren't trying to increment your own module's reference count,
>> >right?  That just doesn't work :)
>> >
>> >thanks,
>> >
>> >greg k-h
>> 
>> This was meant for setting the owner field of a file_operations struct
>> or the cra_owner field of crypto_alg and many other structs.
>
>But shouldn't the macro kernel::declare_file_operations() do this for
>you automagically?  You should never have to manually say "this module!"
>to any structure or function call if we do things right.
>
>Yes, many "old school" structures in the kernel do this, but we have
>learned from the 1990's, see the fun wrappers around simple things like
>usb_register_driver(); as an example of how the driver author themselves
>should never see a module pointer anywhere.
>
>> I know that increfing a module without a good reason is dead dumb, so
>> I'm not trying to send things in a downwards spiral. @@@
>
>That's good, but let's not add housekeeping requirements when we do not
>have to do so if at all possible please.
>
>thanks,
>
>greg k-h

*kicks can*, at least I can take some ideas out of this, anyways, thanks
for your reviews.

Re: [PATCH] rust: add this_module macro

Posted by Greg KH 2 years, 7 months ago

On Tue, Jan 31, 2023 at 01:07:28PM -0300, Martin Rodriguez Reboredo wrote:
> On Tue, Jan 31, 2023 at 04:15:51PM +0100, Greg KH wrote:
> >On Tue, Jan 31, 2023 at 12:07:45PM -0300, Martin Rodriguez Reboredo wrote:
> >> On Tue, Jan 31, 2023 at 02:42:08PM +0100, Greg KH wrote:
> >> >On Tue, Jan 31, 2023 at 10:08:41AM -0300, Martin Rodriguez Reboredo wrote:
> >> >> Adds a Rust equivalent to the handy THIS_MODULE macro from C.
> >> >> 
> >> >> Signed-off-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com>
> >> >> ---
> >> >>  rust/kernel/lib.rs | 12 ++++++++++++
> >> >>  1 file changed, 12 insertions(+)
> >> >> 
> >> >> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> >> >> index e0b0e953907d..afb6b0390426 100644
> >> >> --- a/rust/kernel/lib.rs
> >> >> +++ b/rust/kernel/lib.rs
> >> >> @@ -80,6 +80,18 @@ impl ThisModule {
> >> >>      }
> >> >>  }
> >> >>  
> >> >> +/// Returns the current module.
> >> >> +#[macro_export]
> >> >> +macro_rules! this_module {
> >> >> +    () => {
> >> >> +        if cfg!(MODULE) {
> >> >> +            Some(unsafe { $crate::ThisModule::from_ptr(&mut $crate::bindings::__this_module) })
> >> >> +        } else {
> >> >> +            None
> >> >> +        }
> >> >> +    };
> >> >> +}
> >> >
> >> >While this is handy, what exactly will it be used for?  The C
> >> >wrappers/shim/whatever should probably handle this for you already when
> >> >you save this pointer into a structure right?
> >> >
> >> >Surely you aren't trying to increment your own module's reference count,
> >> >right?  That just doesn't work :)
> >> >
> >> >thanks,
> >> >
> >> >greg k-h
> >> 
> >> This was meant for setting the owner field of a file_operations struct
> >> or the cra_owner field of crypto_alg and many other structs.
> >
> >But shouldn't the macro kernel::declare_file_operations() do this for
> >you automagically?  You should never have to manually say "this module!"
> >to any structure or function call if we do things right.
> >
> >Yes, many "old school" structures in the kernel do this, but we have
> >learned from the 1990's, see the fun wrappers around simple things like
> >usb_register_driver(); as an example of how the driver author themselves
> >should never see a module pointer anywhere.
> >
> >> I know that increfing a module without a good reason is dead dumb, so
> >> I'm not trying to send things in a downwards spiral. @@@
> >
> >That's good, but let's not add housekeeping requirements when we do not
> >have to do so if at all possible please.
> >
> >thanks,
> >
> >greg k-h
> 
> *kicks can*, at least I can take some ideas out of this, anyways, thanks
> for your reviews.

I don't mean to reject this out-of-hand, it's just that without a real
user, it's impossible to review this and say "this is ok" instead of
"perhaps you should do it this other way"?

Right now the rust framework is just that, a framework.  Perhaps we
should not be adding anything else to it until there is a real user of
it?  Otherwise this will keep coming up again and again.

Treat this like any other kernel feature/addition, you can't add apis
until you submit a user for it at the same time.  That's one way we have
been able to evolve and maintain the kernel source tree for so long.
Without an api user, we have no way to know how it's being used or if
it's even being used at all.

thanks,

greg k-h

Re: [PATCH] rust: add this_module macro

Posted by Miguel Ojeda 2 years, 7 months ago

On Tue, Jan 31, 2023 at 5:59 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> Right now the rust framework is just that, a framework.  Perhaps we
> should not be adding anything else to it until there is a real user of
> it?  Otherwise this will keep coming up again and again.
>
> Treat this like any other kernel feature/addition, you can't add apis
> until you submit a user for it at the same time.  That's one way we have
> been able to evolve and maintain the kernel source tree for so long.
> Without an api user, we have no way to know how it's being used or if
> it's even being used at all.

Agreed. For this patch, it came independently, so I cannot speak about
its users. For other patch series we have sent, the users are
out-of-tree, but they want to come in-tree as soon as possible. We are
coordinating with them to prioritize the submission of the things they
will depend on.

Since the goal is to submit things piece by piece so that they can be
properly reviewed, what we have been doing to ameliorate things is to
provide enough details, examples and documentation for each function,
type, etc. so that it is hopefully clear how they will be used. If
there are some cases where it may not be clear, we can also link to
the upcoming users.

Cheers,
Miguel