[wedsonaf]

chrdev will need to be updated once we decide how we want it to share state. For now, it doesn't allow sharing.

Can you please give some context / explanation to the changes?

Can you please give some context / explanation to the changes?

Sure. Consider the example that I added: how would you implement it without my changes? The only way to do it would be with global variables, and even then it wouldn't be great because if we registered more than one device they'd all share the global state.

My change allows us to have per-device state: when we register a device, we provide some state; then each file gets a reference to the shared state on its open method (and does with it whatever it wants).

This is a very common pattern in the kernel, and each type of device saves this shared state in a different place. For example, miscdev uses file->private_data, while chrdev uses inode->i_cdev to store pointers to miscdev and cdev. These are usually embedded in an outer struct, so using container_of we can access the shared state.

In C, this is all untyped and one could easily mess this up. In Rust, the unsafe code is wrapped and it exposes a safe API.

In binder, we need this because each device is a "universe" -- processes that can access a device can all do IPC amongst themselves, totally independently of what's happening in another device (e.g., normally we have three instances: binder, hwbinder, and vndbinder).

@alex -- this is an example of the ugliness brought by Pin I mentioned in one of the meetings. A bunch of unsafe that wouldn't need to be unsafe if we had better support for pinning. Two aspects are relevant here:

1. Initialisation: CondVar::new and Mutex::new are unsafe because they require a call to init after the condvar/mutex are in their final pinned location.
2. Projection: the compiler can't (or won't) verify for us that we don't move out of the fields of a pinned structure, so we need unsafe. (Though admittedly I don't explicitly say that state is pinned, mostly because it doesn't give me anything.)

It would be nice if we could avoid this.

Right. When you say "we had better support for pinning", do you think this is something that Rust-for-Linux can improve itself, or do you think this requires better language level support?

Well, we could choose to ignore Pin and assume that everything that is heap-allocated is pinned, but my opinion is that this isn't desirable because Pin is actually useful. (We sort of discussed this in the meeting.)

So I think we need better language support.

@joshtriplett as our resident friend upstream, what's the best way for us to share these observations/desires with the team?

@RalfJung -- since you commented on something else related to this project, I thought I'd ask you if you have any thoughts about improving pinning support. As it is today, it forces us (unreasonably in my opinion) to use too much unsafe code.

Projection: the compiler can't (or won't) verify for us that we don't move out of the fields of a pinned structure, so we need unsafe. (Though admittedly I don't explicitly say that state is pinned, mostly because it doesn't give me anything.)

The pin-project crate has a proc macro to help with this.

I agree that writing code with pin projections currently requires more unsafe code than it should. I am not sure what the best way forward for that is; the pin-project crate mentioned by @bjorn3 is a great start showing how far one can go even without any language support. (I did not carefully audit it though.)

The "new and then init" pattern seems mostly orthogonal to pinning; that pattern inherently requires exposing the user to an unsafe pre-init state. If you want to expose that state safely, you need to give it a separate type.

since you commented on something else related to this project, I thought I'd ask you

Sure, feel free to. :) I don't have a ton of time these days, but I am super excited about Rust having a realistic chance of being used in the upstream Linux kernel, so if I can help in small ways I'm more than happy to do that.

I've had one pass through this, and I'm still working to make sure I fully understand it, to be honest.

One small thing though -- do the changes to make read/write return counts need to be a part of this? Could we split them out, just to reduce the review surface area.

I've had one pass through this, and I'm still working to make sure I fully understand it, to be honest.

Thanks for reviewing. I'm happy to try to clarify if there's anything that doesn't make sense.

One small thing though -- do the changes to make read/write return counts need to be a part of this? Could we split them out, just to reduce the review surface area.

It is needed by the example. I could split out the example if you think it'll help, although it would be counter to what reviewers have been asking -- they always [rightfully] ask for example usage.

Can we split the baby :-) The changes for read/write to return usize into their own PR, but keep the core changes to the example that demo the shared state here.

…

On Tue, Mar 30, 2021 at 7:22 PM Wedson Almeida Filho < ***@***.***> wrote: I've had one pass through this, and I'm still working to make sure I fully understand it, to be honest. Thanks for reviewing. I'm happy to try to clarify if there's anything that doesn't make sense. One small thing though -- do the changes to make read/write return counts need to be a part of this? Could we split them out, just to reduce the review surface area. It is needed by the example. I could split out the example if you think it'll help, although it would be counter to what reviewers have been asking -- they always [rightfully] ask for example usage. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#145 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAAGBHDZDVZ4ZW6GF3OILDTGJMLHANCNFSM4Z2JEZAQ> .

-- All that is necessary for evil to succeed is for good people to do nothing.

Can we split the baby :-) The changes for read/write to return usize into their own PR, but keep the core changes to the example that demo the shared state here.

Sure, give me a few minutes.

I'm splitting the last commit (or most of it) into its own PR to simplify things here.

Actually, I'm going to split it into more PRs.

Only 37ee4f8 and c8e1fdb are part of this PR now.

A couple bits that I noticed...

@ojeda @alex -- is there anything about this that is bothering you or doesn't sound right? I'm happy to discuss if that's the case.

I lost track of the fact that I owed you a re-review. Added to my TODO list for this evening.

is there anything about this that is bothering you or doesn't sound right?

Not at all! Just marked things as resolved (I usually let the PR sender to resolve them).

@ojeda did you want to take another pass at this before it's merged?

Let's move forward!