Skip to content

Add ScopeGuard and use it in Binder. #317

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jun 1, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 12 additions & 8 deletions drivers/android/transaction.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ use alloc::sync::Arc;
use core::sync::atomic::{AtomicBool, Ordering};
use kernel::{
io_buffer::IoBufferWriter, linked_list::Links, prelude::*, sync::Ref,
user_ptr::UserSlicePtrWriter,
user_ptr::UserSlicePtrWriter, ScopeGuard,
};

use crate::{
Expand Down Expand Up @@ -144,6 +144,10 @@ impl DeliverToRead for Transaction {
pub sender_pid: pid_t,
pub sender_euid: uid_t,
*/
let send_failed_reply = ScopeGuard::new(|| {
let reply = Either::Right(BR_FAILED_REPLY);
self.from.deliver_reply(reply, &self);
});
let mut tr = BinderTransactionData::default();

if let Some(nref) = &self.node_ref {
Expand Down Expand Up @@ -171,13 +175,13 @@ impl DeliverToRead for Transaction {
BR_TRANSACTION
};

// Write the transaction code and data to the user buffer. On failure we complete the
// transaction with an error.
if let Err(err) = writer.write(&code).and_then(|_| writer.write(&tr)) {
let reply = Either::Right(BR_FAILED_REPLY);
self.from.deliver_reply(reply, &self);
return Err(err);
}
// Write the transaction code and data to the user buffer.
writer.write(&code)?;
writer.write(&tr)?;

// Dismiss the completion of transaction with a failure. No failure paths are allowed from
// here on out.
send_failed_reply.dismiss();

// When this is not a reply and not an async transaction, update `current_transaction`. If
// it's a reply, `current_transaction` has already been updated appropriately.
Expand Down
2 changes: 1 addition & 1 deletion rust/kernel/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ pub mod user_ptr;
pub use build_error::build_error;

pub use crate::error::{Error, Result};
pub use crate::types::Mode;
pub use crate::types::{Mode, ScopeGuard};

/// Page size defined in terms of the `PAGE_SHIFT` macro from C.
///
Expand Down
63 changes: 63 additions & 0 deletions rust/kernel/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -91,3 +91,66 @@ impl<T: PointerWrapper + Deref> PointerWrapper for Pin<T> {
Pin::new_unchecked(T::from_pointer(p))
}
}

/// Runs a cleanup function/closure when dropped.
///
/// The [`ScopeGuard::dismiss`] function prevents the cleanup function from running.
Comment on lines +95 to +97
Copy link
Member

@ojeda ojeda May 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add two or three examples, e.g. a trivial one with a explicit scope for extra clarity, one with dismiss() to show the difference and one with e.g. the ? operator etc. in the middle (like in your use case in Binder).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added two examples.

///
/// # Examples
///
/// In the example below, we have multiple exit paths and we want to log regardless of which one is
/// taken:
/// ```
/// fn example1(arg: bool) {
/// let _log = ScopeGuard::new(|| pr_info!("example1 completed\n"));
///
/// if arg {
/// return;
/// }
///
/// // Do something...
/// }
/// ```
///
/// In the example below, we want to log the same message on all early exits but a different one on
/// the main exit path:
/// ```
/// fn example2(arg: bool) {
/// let log = ScopeGuard::new(|| pr_info!("example2 returned early\n"));
///
/// if arg {
/// return;
/// }
///
/// // (Other early returns...)
///
/// log.dismiss();
/// pr_info!("example2 no early return\n");
/// }
/// ```
pub struct ScopeGuard<T: FnOnce()> {
cleanup_func: Option<T>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be ManuallyDrop<T>? On dismiss we can call drop, and on drop we can call take. This avoids extra memory.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it could. But the size of the object is 1 byte, which I expect to be optimised out on optimised builds anyway.

I don't think adding ManuallyDrop and two extra unsafe calls is worth the loss is readability here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's indeed usually optimised out, but having a known-to-be-true if statement in the drop method still feels awkward.

(BTW it's align_of::<T> extra bytes)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's indeed usually optimised out, but having a known-to-be-true if statement in the drop method still feels awkward.

The if statement isn't always true, it is false when dismiss is called.

Copy link
Member

@nbdd0121 nbdd0121 May 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The if statement isn't always true, it is false when dismiss is called.

Haha, I know you're going to say this :) I only realized that after I send out that message. Obviously you can add a forget and it'll be always true :)

Option it's not zero-cost if used in async fn. We're unlikely to use async any time soon though, so I don't feel too strongly in favor of ManuallyDrop. I do prefer to rely as few on the optimiser as possible though.

Also, please add #[inline] for all of new, dismiss, drop. This helps in opt-level<=1.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also LTO probably means that we would need LLD somewhere in our pipeline? Anyway I still think we shouldn't require or assume people will use LTO.

Copy link
Member

@ojeda ojeda May 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By the way, please do not take me the wrong way when I complain about stuff like this. I usually push you guys a lot (@nbdd0121 and @bjorn3) because I know you are experts on this stuff.

The idea being that either 1) you tell me how I am completely stupid (and then I learn more about rustc/LLVM internals/limitations, which is a win for me; plus we collectively learn how to answer similar questions if they happen to be raised in e.g. the LKML); or 2) I convince you to improve Rust/LLVM in some areas and everybody wins ;-)

Copy link
Member

@ojeda ojeda May 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, Rust does not guess whether something is suitable for inlining at all; it's all up to LLVM. Therefore for non-generic functions, cross-crate inlining requires either #[inline] or LTO. It's like static inline function vs an extern function.

I was thinking LLVM could give back its cost heuristics after optimizing a function, and then the frontend decide if it wants to generate a Rust generic-like function or go with a normal one etc.

Yeah, it is likely not trivial if LLVM is not setup to do that, and most likely not worth it anyway since using LTO is simpler and a more general approach. But hey, it would improve things for all non-LTO-enabled projects and remove the needs for so many hints.

They may not care that much, but requiring a function call for &CStr to *const c_char conversion is not acceptable.

Yes, but what I am saying is the amount of hints we use depends on whether we say LTO is the way to go or not, and how much the overall % lost. The idea being that we could try to be better than C here and have the hints that actually matter, instead of systematically providing hints everywhere.

At the current time, the safest approach is indeed doing it manually like the C side does (providing hints, having functions in headers, using macros to hack it, etc.).

Copy link
Member

@ojeda ojeda May 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also LTO probably means that we would need LLD somewhere in our pipeline? Anyway I still think we shouldn't require or assume people will use LTO.

Yeah:

config HAS_LTO_CLANG
        ...
	depends on CC_IS_CLANG && CLANG_VERSION >= 110000 && LD_IS_LLD

It is still experimental, but since it is also a requirement for CFI, I bet it will become way more common soon enough.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also LTO probably means that we would need LLD somewhere in our pipeline? Anyway I still think we shouldn't require or assume people will use LTO.

Rustc's LTO support normally doesn't require LLD as rustc performs it itself. Due to the way rustc is used, it can't do LTO itself though and will need linker plugin LTO. This should work with linkers other than LLD though, but requires a linker plugin for the right LLVM version rustc uses.

I was thinking LLVM could give back its cost heuristics after optimizing a function, and then the frontend decide if it wants to generate a Rust generic-like function or go with a normal one etc.

By the time codegen happens, the frontend has already emitted the metadata file.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not even a nit, probably closer to a code style question. Why name the field if it's the only one in the struct? Presumably you could also do:

pub struct ScopeGuard<T: FnOnce()>(Option<T>);

Is this because an explicit name provides more information than .0 and makes the code more readable?

Copy link
Member

@ojeda ojeda May 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, we should decide which style to use.

One tiny advantage of non-tuple ones is avoiding tuple constructor shenanigans:

struct T(u8);

pub fn f() -> u8 {
    T as _
}

I would like to have a lint for that -- from a quick look there does not seem to be one.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sometimes this is an advantage, e.g. you can use the tuple struct name in functions such as map.

Obviously for non-public structs there's no difference though.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think a simple rule would be to avoid tuple ones unless the tuple constructor is intended to be used, e.g. for newtypes.

Copy link
Collaborator

@TheSven73 TheSven73 May 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ojeda I don't understand yet which shenanigans you mean. Maybe you could clarify?

But I also don't want to hijack @wedsonaf 's PR :)

Copy link
Member

@ojeda ojeda May 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is really shocking! This would be a great interview question to weed out people like me :)

I still don't understand where the 192_u8 comes from. See playground.

It is just the bottom bits of the address: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=a36bc1e4b100255e9f2c4e5b7786d5d1

Notice 20 as the last byte in the full address.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So you're saying that Rust leaks memory addresses just like that...? I'm not sure what to think about that.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From my point of view, this discussion shouldn't derail @wedsonaf 's PR.
LGTM.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So you're saying that Rust leaks memory addresses just like that...? I'm not sure what to think about that.

Well, it is memory-safe to do so on its own ;)

(More seriously: you are right, in the kernel we definitely want to avoid leaking addresses -- but that is a bit orthogonal to this).

From my point of view, this discussion shouldn't derail @wedsonaf 's PR. LGTM.

No worries!

(I will be logging off for the weekend myself in a few minutes anyway -- I guess Wedson will add the updated docs on Monday).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have a great week-end. Thanks for updating my understanding of Rust!

}

impl<T: FnOnce()> ScopeGuard<T> {
/// Creates a new cleanup object with the given cleanup function.
pub fn new(cleanup_func: T) -> Self {
Self {
cleanup_func: Some(cleanup_func),
}
}

/// Prevents the cleanup function from running.
pub fn dismiss(mut self) {
self.cleanup_func.take();
}
}

impl<T: FnOnce()> Drop for ScopeGuard<T> {
fn drop(&mut self) {
// Run the cleanup function if one is still present.
if let Some(cleanup) = self.cleanup_func.take() {
cleanup();
}
}
}