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

[Alice Ryhl]

On Wed, Mar 04, 2026 at 08:29:12AM -0800, Boqun Feng wrote:
> 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)

Yeah but then I have to actually re-implement those methods and not just
call them. Seems not worth it.

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

Perf-wise it doesn't matter, but I'm concerned about memory usage.

Alice