Re: [PATCH v2 1/2] rust: poll: make PollCondVar upgradable

[Boqun Feng <boqun@kernel.org>]

On Wed, Mar 04, 2026 at 07:59:59AM +0000, Alice Ryhl wrote:
[...]
> > > +        // If a normal waiter registers in parallel with us, then either:
> > > +        // * We took the lock first. In that case, the waiter sees the above cmpxchg.
> > > +        // * They took the lock first. In that case, we wake them up below.
> > > +        drop(lock.lock());
> > > +        self.simple.notify_all();
> > 
> > Hmm.. what if the waiter gets its `&CondVar` before `upgrade()` and use
> > that directly?
> > 
> > 	<waiter>				<in upgrade()>
> > 	let poll_cv: &UpgradePollCondVar = ...;
> > 	let cv = poll_cv.deref();
> > 						cmpxchg();
> > 						drop(lock.lock());
> > 						self.simple.notify_all();
> > 	let mut guard = lock.lock();
> > 	cv.wait(&mut guard);
> > 
> > we still miss the wake-up, right?
> > 
> > It's creative, but I particularly hate we use an empty lock critical
> > section to synchronize ;-)
> 
> I guess instead of exposing Deref, I can just implement `wait` directly
> on `UpgradePollCondVar`. Then this API misuse is not possible.
> 

If we do that,then we can avoid the `drop(lock.lock())` as well, if we
do:

    impl UpgradePollCondVar {
        pub fn wait(...) {
	    prepare_to_wait_exclusive(); // <- this will take lock in
                                         // simple.wait_queue_head. So
                                         // either upgrade() comes
                                         // first, or they observe the
                                         // wait being queued.
            let cv_ptr = self.active.load(Relaxed);
	    if !ptr_eq(cv_ptr, &self.simple) { // We have moved from
	                                       // simple, so need to
                                               // need to wake up and
                                               // redo the wait.
	        finish_wait();
	    } else {
	        guard.do_unlock(|| { schedule_timeout(); });
		finish_wait();
	    }
	}
    }

(CondVar::notify*() will take the wait_queue_head lock as well)

> > Do you think the complexity of a dynamic upgrading is worthwhile, or we
> > should just use the box-allocated PollCondVar unconditionally?
> > 
> > I think if the current users won't benefit from the dynamic upgrading
> > then we can avoid the complexity. We can always add it back later.
> > Thoughts?
> 
> I do actually think it's worthwhile to consider:
> 
> I started an Android device running this. It created 3961 instances of
> `UpgradePollCondVar` during the hour it ran, but only 5 were upgraded.
> 

That makes sense, thank you for providing the data! But still I think we
should be more informative about the performance difference between
dynamic upgrading vs. unconditionally box-allocated PollCondVar, because
I would assume when a `UpgradePollCondVar` is created, other allocations
also happen as well (e.g. when creating a Arc<binder::Thread>), so the
extra cost of the allocation may be unnoticeable.

Regards,
Boqun


> Alice