Add `ScopeGuard` and use it in Binder. #317

wedsonaf · 2021-05-29T01:24:35Z

This is in preparation for introducing additional error paths that
require sending an error reply to the transaction issuer, but return a
success result to the function caller.

rust/kernel/types.rs

nbdd0121 · 2021-05-29T01:32:47Z

rust/kernel/types.rs

+///
+/// The [`Cleaner::dismiss`] function prevents the cleanup function from running.
+pub struct Cleaner<T: FnOnce()> {
+    cleanup_func: Option<T>,


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

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.

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)

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.

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.

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.

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 ;-)

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.).

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.

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.

ojeda

LGTM, but see the doc request.

ojeda · 2021-05-29T04:29:58Z

rust/kernel/types.rs

+/// Runs a cleanup function/closure when dropped.
+///
+/// The [`ScopeGuard::dismiss`] function prevents the cleanup function from running.


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).

Added two examples.

ojeda · 2021-05-29T04:41:36Z

Also please put the rust: prefix in the first commit.

rust/kernel/types.rs

TheSven73 · 2021-05-29T13:06:36Z

rust/kernel/types.rs

+///
+/// The [`ScopeGuard::dismiss`] function prevents the cleanup function from running.
+pub struct ScopeGuard<T: FnOnce()> {
+    cleanup_func: Option<T>,


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?

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.

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.

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

@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 :)

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.

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

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

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).

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

nbdd0121 · 2021-05-29T14:18:14Z

LGTM as well apart from the name cleanup_func, would be better if it's drop_fn or defer_fn (or at least cleanup_fn).

TheSven73 · 2021-05-29T14:27:36Z

I'm not asking to change the PR, just thinking out loud about the changes I see here.

The way this new ScopeGuard abstraction is used in Binder bears all the hallmarks of a transaction. Would it improve conceptual readability in the Binder code if you create a specific "binder transaction" abstraction? I may of course be mistaken, first time I see the Binder code. A transaction abstraction may not be worth it.

Signed-off-by: Wedson Almeida Filho <[email protected]>

This is in preparation for introducing additional error paths that require sending an error reply to the transaction issuer, but return a success result to the function caller. Signed-off-by: Wedson Almeida Filho <[email protected]>

ksquirrel · 2021-06-01T15:35:31Z

Review of c96a4c5dfbfd:

✔️ Commit fdb5c04: Looks fine!
✔️ Commit c96a4c5: Looks fine!

wedsonaf · 2021-06-01T15:37:20Z

Also please put the rust: prefix in the first commit.

Done.

wedsonaf · 2021-06-01T15:42:15Z

The way this new ScopeGuard abstraction is used in Binder bears all the hallmarks of a transaction. Would it improve conceptual readability in the Binder code if you create a specific "binder transaction" abstraction? I may of course be mistaken, first time I see the Binder code. A transaction abstraction may not be worth it.

Binder has the idea of a transaction (borrowed from the C code), but it has a different meaning from the traditional "something that can be prepared and then committed or rolled-back". It's more of a 'message' between two entities with additional properties.

That said, I'm curious about your idea of a transaction abstraction -- I'm all for simplifying concepts :) Would you mind sharing more about what you had in mind?

TheSven73 · 2021-06-01T16:04:57Z

That said, I'm curious about your idea of a transaction abstraction -- I'm all for simplifying concepts :) Would you mind sharing more about what you had in mind?

One size definitely doesn't fit all, it depends on what you have to work with. But normally, a transaction abstraction is an object that:

buffers all operations
until .transact() gets called, then they get batch-executed
transact() either returns an error (then no operations have been executed) or returns ok (then all operations have been executed)
after the call to transact() it is no longer possible to access the object. This could work in Rust by making that function consume self.

TheSven73 · 2021-06-01T18:03:28Z

LGTM.
Not sure if you saw Gary's comment re. changing cleanup_func into cleanup_fn?

TheSven73 · 2021-06-03T16:49:32Z

@wedsonaf I just noticed that's exactly what you did in #311. I may be a bit slow but I do get there eventually :)

wedsonaf · 2021-06-08T10:28:33Z

@wedsonaf I just noticed that's exactly what you did in #311. I may be a bit slow but I do get there eventually :)

I'm glad you noticed it :)

In this particular case, I thought you had had some inspiration and found a way to represent some [more complicated] code as a transaction. But I guess you were just wondering in general, which is also perfectly ok.

TheSven73 · 2021-06-08T12:58:16Z

Yes I had nothing more to offer than a general remark - I've never read or familiarized myself with your Binder Rust code. And that's too bad, as it's the only practical, performance-sensitive, real-world application we have at the moment...

(I think what you did with Binder is great BTW)

We can see that "bswap32: Takes an unsigned 32-bit number in either big- or little-endian format and returns the equivalent number with the same bit width but opposite endianness" in BPF Instruction Set Specification, so it should clear the upper 32 bits in "case 32:" for both BPF_ALU and BPF_ALU64. [root@linux fedora]# echo 1 > /proc/sys/net/core/bpf_jit_enable [root@linux fedora]# modprobe test_bpf Before: test_bpf: #313 BSWAP 32: 0x0123456789abcdef -> 0xefcdab89 jited:1 ret 1460850314 != -271733879 (0x5712ce8a != 0xefcdab89)FAIL (1 times) test_bpf: #317 BSWAP 32: 0xfedcba9876543210 -> 0x10325476 jited:1 ret -1460850316 != 271733878 (0xa8ed3174 != 0x10325476)FAIL (1 times) After: test_bpf: #313 BSWAP 32: 0x0123456789abcdef -> 0xefcdab89 jited:1 4 PASS test_bpf: #317 BSWAP 32: 0xfedcba9876543210 -> 0x10325476 jited:1 4 PASS Fixes: 4ebf921 ("LoongArch: BPF: Support unconditional bswap instructions") Acked-by: Hengqi Chen <[email protected]> Signed-off-by: Tiezhu Yang <[email protected]> Signed-off-by: Huacai Chen <[email protected]>

In case when is64 == 1 in emit(A64_REV32(is64, dst, dst), ctx) the generated insn reverses byte order for both high and low 32-bit words, resuling in an incorrect swap as indicated by the jit test: [ 9757.262607] test_bpf: Rust-for-Linux#312 BSWAP 16: 0x0123456789abcdef -> 0xefcd jited:1 8 PASS [ 9757.264435] test_bpf: Rust-for-Linux#313 BSWAP 32: 0x0123456789abcdef -> 0xefcdab89 jited:1 ret 1460850314 != -271733879 (0x5712ce8a != 0xefcdab89)FAIL (1 times) [ 9757.266260] test_bpf: Rust-for-Linux#314 BSWAP 64: 0x0123456789abcdef -> 0x67452301 jited:1 8 PASS [ 9757.268000] test_bpf: Rust-for-Linux#315 BSWAP 64: 0x0123456789abcdef >> 32 -> 0xefcdab89 jited:1 8 PASS [ 9757.269686] test_bpf: Rust-for-Linux#316 BSWAP 16: 0xfedcba9876543210 -> 0x1032 jited:1 8 PASS [ 9757.271380] test_bpf: Rust-for-Linux#317 BSWAP 32: 0xfedcba9876543210 -> 0x10325476 jited:1 ret -1460850316 != 271733878 (0xa8ed3174 != 0x10325476)FAIL (1 times) [ 9757.273022] test_bpf: Rust-for-Linux#318 BSWAP 64: 0xfedcba9876543210 -> 0x98badcfe jited:1 7 PASS [ 9757.274721] test_bpf: Rust-for-Linux#319 BSWAP 64: 0xfedcba9876543210 >> 32 -> 0x10325476 jited:1 9 PASS Fix this by forcing 32bit variant of rev32. Fixes: 1104247 ("bpf, arm64: Support unconditional bswap") Signed-off-by: Artem Savkov <[email protected]> Tested-by: Puranjay Mohan <[email protected]> Acked-by: Puranjay Mohan <[email protected]> Acked-by: Xu Kuohai <[email protected]> Message-ID: <[email protected]> Signed-off-by: Alexei Starovoitov <[email protected]>

This comment has been minimized.

Sign in to view

nbdd0121 reviewed May 29, 2021

View reviewed changes

wedsonaf force-pushed the cleanup branch from 3acb195 to a5aa111 Compare May 29, 2021 02:15

This comment has been minimized.

Sign in to view

wedsonaf changed the title ~~Add Cleaner and use it in Binder.~~ Add ScopeGuard and use it in Binder. May 29, 2021

alex approved these changes May 29, 2021

View reviewed changes

ojeda reviewed May 29, 2021

View reviewed changes

TheSven73 reviewed May 29, 2021

View reviewed changes

rust/kernel/types.rs Show resolved Hide resolved

TheSven73 reviewed May 29, 2021

View reviewed changes

TheSven73 approved these changes May 29, 2021

View reviewed changes

wedsonaf added 2 commits June 1, 2021 16:30

rust: Add ScopeGuard to optionally run cleanup closures.

fdb5c04

Signed-off-by: Wedson Almeida Filho <[email protected]>

binder: Use ScopeGuard on transaction processing.

c96a4c5

This is in preparation for introducing additional error paths that require sending an error reply to the transaction issuer, but return a success result to the function caller. Signed-off-by: Wedson Almeida Filho <[email protected]>

wedsonaf force-pushed the cleanup branch from a5aa111 to c96a4c5 Compare June 1, 2021 15:35

TheSven73 approved these changes Jun 1, 2021

View reviewed changes

alex approved these changes Jun 1, 2021

View reviewed changes

ojeda merged commit ac536cc into Rust-for-Linux:rust Jun 1, 2021

nbdd0121 mentioned this pull request Jun 1, 2021

Add SPI Abstraction #264

Open

wedsonaf deleted the cleanup branch June 8, 2021 10:24

Add ScopeGuard and use it in Binder. #317

Add ScopeGuard and use it in Binder. #317

Conversation

wedsonaf commented May 29, 2021

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nbdd0121 May 29, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ojeda May 29, 2021 • edited Loading

Choose a reason for hiding this comment

ojeda May 29, 2021 • edited Loading

Choose a reason for hiding this comment

ojeda May 29, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

ojeda left a comment

Choose a reason for hiding this comment

ojeda May 29, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ojeda commented May 29, 2021

Choose a reason for hiding this comment

ojeda May 29, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheSven73 May 29, 2021 • edited Loading

Choose a reason for hiding this comment

ojeda May 29, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nbdd0121 commented May 29, 2021

TheSven73 commented May 29, 2021

ksquirrel commented Jun 1, 2021

wedsonaf commented Jun 1, 2021

wedsonaf commented Jun 1, 2021

TheSven73 commented Jun 1, 2021

TheSven73 commented Jun 1, 2021

TheSven73 commented Jun 3, 2021

wedsonaf commented Jun 8, 2021

TheSven73 commented Jun 8, 2021

Add `ScopeGuard` and use it in Binder. #317

Add `ScopeGuard` and use it in Binder. #317

nbdd0121 May 29, 2021 •

edited

Loading

ojeda May 29, 2021 •

edited

Loading

ojeda May 29, 2021 •

edited

Loading

ojeda May 29, 2021 •

edited

Loading

ojeda May 29, 2021 •

edited

Loading

ojeda May 29, 2021 •

edited

Loading

TheSven73 May 29, 2021 •

edited

Loading

ojeda May 29, 2021 •

edited

Loading