Hi Jason:
On Tue, Jul 26, 2022 at 12:17:02PM +0200, Jason A. Donenfeld wrote:
Yea, I actually didn't consider this a bug, but just "nothing starts until everything else is totally done" semantics. Not wanting those semantics is understandable. I haven't looked in detail at how you've
The issue is when you switch from one hwrng to another, this could cause a user-space reader to fail during the switch-over. Granted it's not a big deal but the fix isn't that onerous.
done that safely without locking issues, but if you've tested it, then I trust it works. When I was playing around with different things, I encountered a number of locking issues, depending on what the last locker was - a user space thread, the rng kthread, or something opening up a reader afresh. So just be sure the various combinations still work.
Yes this is subtle and I don't plan on pushing this into mainline right away.
Specifically, Instead of doing all of this task interruption stuff and keeping track of readers and using RCU and all that fragile code, we can instead just use wait_completion_interruptible_timeout() inside of hwrng_msleep(), using a condition variable that's private to the hwrng. Then, when it's time for all the readers to quit, we just set the condition! Ta-da, no need for keeping track of who's reading, the difference between a kthread and a normal thread, and a variety of other concerns.
Yes, using a higher-level abstraction than wake_up_process/schedule is always preferred.
Unimportant nit: could call __rng_dying() instead so these don't diverge by accident.
Good idea, I will do this in the next repost.
@@ -269,6 +298,9 @@ static ssize_t rng_dev_read(struct file *filp, char __user *buf, } } out:
- if (synch)
synchronize_rcu();The synch usage no longer makes sense to me here. In my version, synchronize_rcu() would almost never be called. It's only purpose was to prevent rng_dev_read() from returning if the critical section inside of hwrng_unregister() was in flight. But now it looks like it will be called everytime there are no RNGs left? Or maybe I understood how this works. Either way, please don't feel that you have to write back an explanation; just make sure it has those same sorts of semantics when testing.
The purpose is still the same, prevent the current thread from going away while the RCU critical-section in hwrng_unregister is ongoing.
With my code the synch is triggered if we obtained an rng, then potentially went to sleep (wait == true), and upon finishing we found that the rng is undergoing unregistration (rng_dying == true).
If we switch to completions then this issue goes away because we will be using standard wait queues instead of our own current_waiting_reader.
Here you made a change whose utility I don't understand. My original hunk was:
if (kthread_should_stop())break;schedule_timeout_interruptible(HZ * 10);Which I think is a bit cleaner, as schedule_timeout_interruptible sets the state to INTERRUPTIBLE and such.
Valentin has already explained this. This is the standard paradigm for calling schedule_timeout when you need to ensure that a specific condition wakes up the thread. But as you suggested using a higher- level mechanism such as completions will render this unnecessary.
Thanks,