Hi Yann, 2016-08-21 4:14 GMT+02:00 Yann Ylavic <ylavic....@gmail.com>:
> On Thu, Aug 18, 2016 at 3:17 PM, Yann Ylavic <ylavic....@gmail.com> wrote: > > > > I was thinking of something like the attached patch, where we compute > > the exact/minimal timeout needed for the queues. > > > > It requires walking/locking the queues (looking at the first item > > only), but besides getting the exact poll() timeout which helps > > spurious wakeups, it also allows to avoid walking this queues after > > the poll() (previously each 0.1s, for maintenance) when not necessary. > > So overall I think it's a gain. > > Finally I came up with another way which requires no walking (hence > locking) in the listener outside queues/timers processing (patch > attached). > It looks good to me, not heavily tested/debugged still... > > The approach is, for both timeout queues and timers, to maintain the > next (lowest) expiry time of all entries (namely queues_next_expiry > and timers_next_expiry). > These global values are kept up to date in TO_QUEUE_APPEND (for > timeout queues) and event_get_timer_event (for timers, where the > skiplist is filled) by comparing the expiry of newly added entries > (already protected by their respective timeout/skiplist mutex). > This happens in the workers, and is cheap (locking was there already > to protect from concurrent workers), it just requires an > apr_pollset_wakeup to notice the listener whenever one of these > expiries is updated. > > So now, in the listener, we can read these values directly to compute > the min poll() timeout to use, and also for the timeout queues to > determine whether or not it's time to process them. > If an expiry is updated while poll()ing, it's woken up and the timeout > is also updated. > No locking needed here, I use volatile reads which is enough (no > atomic/ordered read required, no apr_atomic_read64 to handle an > apr_time_t anyway) since the listener is garanteed to be woken up when > needed. > > This also benefits the !listener_is_wakeable case (with a maximum > poll() timeout of 100ms), since we avoid the lock when no timer is > armed (we don't use timers in 2.4.x afaict), and also avoid spurious > timeout queues processing. > > The patch includes Luca's http://apaste.info/mke (base of > APR_POLLSET_WAKEABLE handling) and also tries to make existing > critical sections as short as possible (barely related, but while at > it :) > > Thoughts? > It looks really great, I like a lot the overall solution but I haven't reviewed (and understood) the whole set of changes yet. Other than pure code review, I would like to know how changes like this one have been tested/accepted/rejected in the past, since it would be really great not to loose momentum imho. Thanks a lot! Luca
