Re: [PATCH net-next v4 1/4] rust: core abstractions for network PHY drivers

[Benno Lossin <benno.lossin@proton.me>]

On 17.10.23 16:21, Greg KH wrote:
> On Tue, Oct 17, 2023 at 02:04:33PM +0000, Benno Lossin wrote:
>> On 17.10.23 14:38, Andrew Lunn wrote:
>>>>> Because set_speed() updates the member in phy_device and read()
>>>>> updates the object that phy_device points to?
>>>>
>>>> `set_speed` is entirely implemented on the Rust side and is not protected
>>>> by a lock.
>>>
>>> With the current driver, all entry points into the driver are called
>>> from the phylib core, and the core guarantees that the lock is
>>> taken. So it should not matter if its entirely implemented in the Rust
>>> side, somewhere up the call stack, the lock was taken.
>>
>> Sure that might be the case, I am trying to guard against this future
>> problem:
>>
>>       fn soft_reset(driver: &mut Driver) -> Result {
>>           let driver = driver
>>           thread::scope(|s| {
>>               let thread_a = s.spawn(|| {
>>                   for _ in 0..100_000_000 {
>>                       driver.set_speed(10);
>>                   }
>>               });
>>               let thread_b = s.spawn(|| {
>>                   for _ in 0..100_000_000 {
>>                       driver.set_speed(10);
>>                   }
>>               });
>>               thread_a.join();
>>               thread_b.join();
>>           });
>>           Ok(())
>>       }
>>
>> This code spawns two new threads both of which can call `set_speed`,
>> since it takes `&self`. But this leads to a data race, since those
>> accesses are not serialized. I know that this is a very contrived
>> example, but you never when this will become reality, so we should
>> do the right thing now and just use `&mut self`, since that is exactly
>> what it is for.
> 
> Kernel code is written for the use cases today, don't worry about
> tomorrow, you can fix the issue tomorrow if you change something that
> requires it.

The kind of coding style that (mis)-uses interior mutability is not
something that we can change over night. We should do it properly to
begin with.

> And what "race" are you getting here?  You don't have threads in the
> kernel :)

I chose threads, since I am a lot more familiar with that, but the
kernel also has workqueues which execute stuff concurrently (if I
remember correctly). We also have patches for bindings for the workqueue
so they are not that far away.

> Also, if two things are setting the speed, wonderful, you get some sort
> of value eventually, you have much bigger problems in your code as you
> shouldn't have been doing that in the first place.

This is not allowed in Rust, it is UB and will lead to bad things.

>> Not that we do not even have a way to create threads on the Rust side
>> at the moment.
> 
> Which is a good thing :)
> 
>> But we should already be thinking about any possible code pattern.
> 
> Again, no, deal with what we have today, kernel code is NOT
> future-proof, that's not how we write this stuff.

While I made my argument for future proofing, I think that we
should just be using the standard Rust stuff where it applies.

When you want to modify something, use `&mut T`, if not then use
`&T`. Only deviate from this if you have a good argument.

-- 
Cheers,
Benno