On Mon, 26 Feb 2018 23:01:29 +0100, Guus Sliepen wrote:> The problem is not the order of the events, the problem is that in the > Windows version of the event loop, we only handle one event in each loop > iteration. The select() loop handles all events that have accumulated so > far, so regardless of the order it handles them, it never starves fd. At > least, that was what I thought, until I double checked and found out > that we actually don't in tinc 1.1 (tinc 1.0 is correct though).Sure, but changing the order of the events changes which one will be in that first slot.> So, we have to find a proper fix for both the POSIX and Windows variants > of the event loop in 1.1. For POSIX, the issue is that we might > invalidate the loop iterator due to a callback. It should be easy to > detect that and only exit the loop early if that's really the case. I'll > do that.How would you detect that? By checking whether the next node has changed?> For Windows, there are two issues that have to be fixed. First is > that we only handle a single writable IO event. The second is that > WSAWaitForMultipleEvents() only returns one index into the array of > events, even though multiple events might have triggered. How hard would > it be to check for all triggered events?I don't think it is too hard. I was working in that direction initially but wasn't sure if a larger change would be welcome. I'll take a stab at it.> > With this change I can no longer reproduce the problem of nodes not > > responding to pings. > > This will merely move the problem to some other filedescriptor.Changing the order of the events changes the index of which event is returned by WSAWaitForMultipleEvents(). Moving the most recently accessed event to the end gives the others a chance to proceed. - todd
On Mon, Feb 26, 2018 at 03:18:15PM -0700, Todd C. Miller wrote:> > The problem is not the order of the events, the problem is that in the > > Windows version of the event loop, we only handle one event in each loop > > iteration. The select() loop handles all events that have accumulated so > > far, so regardless of the order it handles them, it never starves fd. At > > least, that was what I thought, until I double checked and found out > > that we actually don't in tinc 1.1 (tinc 1.0 is correct though). > > Sure, but changing the order of the events changes which one will > be in that first slot.Yes, but the order is still deterministic. Now another socket will always be the first in the events[] array, and if a lot of data is received on that one, it will starve the others.> > So, we have to find a proper fix for both the POSIX and Windows variants > > of the event loop in 1.1. For POSIX, the issue is that we might > > invalidate the loop iterator due to a callback. It should be easy to > > detect that and only exit the loop early if that's really the case. I'll > > do that. > > How would you detect that? By checking whether the next node has > changed?I was thinking by setting a flag whenever we call io_del(), and checking that flag in the for-loop in event_loop().> Changing the order of the events changes the index of which event > is returned by WSAWaitForMultipleEvents(). Moving the most recently > accessed event to the end gives the others a chance to proceed.But it doesn't order them by most recently accessed. It's a deterministic order that doesn't change except when io_add() or io_del() is called. Or put in another way: splay_each(io_t, io, &io_tree) goes through the nodes of the io_tree in the order determined by io_compare(). -- Met vriendelijke groet / with kind regards, Guus Sliepen <guus at tinc-vpn.org> -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 833 bytes Desc: not available URL: <http://www.tinc-vpn.org/pipermail/tinc-devel/attachments/20180227/ead5bcd5/attachment.sig>
On Tue, 27 Feb 2018 08:12:53 +0100, Guus Sliepen wrote:> I was thinking by setting a flag whenever we call io_del(), and checking > that flag in the for-loop in event_loop().So basically you would store a generation number in the tree that gets incremented by a call to io_del()? Or should the flag/number be changed each time the order of the tree changes?> > Changing the order of the events changes the index of which event > > is returned by WSAWaitForMultipleEvents(). Moving the most recently > > accessed event to the end gives the others a chance to proceed. > > But it doesn't order them by most recently accessed. It's a > deterministic order that doesn't change except when io_add() or io_del() > is called. Or put in another way: splay_each(io_t, io, &io_tree) goes > through the nodes of the io_tree in the order determined by > io_compare().Unlike the POSIX event code, the Windows version calls splay_search() to map the event to an io_t. The call to splay_search() will splay the tree which changes the order the next time we go through the loop. At least that's my take on it. It makes more sense to make the POSIX and Windows versions be as close as possible so I've abandoned this approach. - todd