Bug 87344 - Handle EWOULDBLOCK in pseudo-TCP code
Summary: Handle EWOULDBLOCK in pseudo-TCP code
Status: RESOLVED FIXED
Alias: None
Product: nice
Classification: Unclassified
Component: General (show other bugs)
Version: unspecified
Hardware: Other All
: medium normal
Assignee: Olivier Crête
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-12-15 23:45 UTC by Philip Withnall
Modified: 2014-12-20 15:33 UTC (History)
0 users

See Also:
i915 platform:
i915 features:


Attachments
agent: Handle EWOULDBLOCK when transmitting pseudo-TCP segments (3.43 KB, text/plain)
2014-12-15 23:45 UTC, Philip Withnall
Details
agent: Handle EWOULDBLOCK when transmitting pseudo-TCP segments (updated) (2.07 KB, patch)
2014-12-17 08:32 UTC, Philip Withnall
Details | Splinter Review

Description Philip Withnall 2014-12-15 23:45:00 UTC
Created attachment 110884 [details]
agent: Handle EWOULDBLOCK when transmitting pseudo-TCP segments

As reported on the mailing list[1], the pseudo-TCP code currently aborts the connection if it receives EWOULDBLOCK when transmitting. This is not great.

Patch attached to fix this. The patch is also not great, but definitely better than letting the issue persist.

Also available in git form[2].

Thanks to Radosław Kołodziejczyk for identifying and debugging this.

[1]: http://lists.freedesktop.org/archives/nice/2014-November/001016.html
[2]: http://cgit.collabora.com/git/user/pwith/libnice.git/log/?h=pseudotcp-ewouldblock
Comment 1 Philip Withnall 2014-12-16 00:02:03 UTC
Discussed this with Olivier on IRC, and we decided that a better approach to handling EWOULDBLOCK here would be to drop the packet, and return WR_SUCCESS anyway. The pseudo-TCP code will then recover (eventually) just as if the packet were dropped on the network.

This has the advantage of implicitly throttling the sender, which is what you want when receiving EWOULDBLOCK, since it’s the sender which is overloaded at that point. My patch had the opposite effect: increasing the sender’s CPU load exactly when they can’t handle it.

I’ll produce an updated patch tomorrow.
Comment 2 Philip Withnall 2014-12-17 08:32:31 UTC
Created attachment 110939 [details] [review]
agent: Handle EWOULDBLOCK when transmitting pseudo-TCP segments (updated)

Updated patch. git branch also updated.
Comment 3 Olivier Crête 2014-12-20 00:59:45 UTC
++ on the patch !
Comment 4 Philip Withnall 2014-12-20 15:33:25 UTC
Merged, thanks.

commit 8ecd6b89a815856b52d5931dd3a6d90fa9e85757
Author: Philip Withnall <philip@tecnocode.co.uk>
Date:   Mon Dec 15 23:35:13 2014 +0000

    agent: Handle EWOULDBLOCK when transmitting pseudo-TCP segments
    
    The pseudo-TCP code previously didn’t handle EAGAIN or EWOULDBLOCK
    errors from the low-level NiceSocket code. This caused pseudo-TCP
    connections to be dropped if the transmitting socket ever filled up,
    which could cause problems on high bandwidth connections.
    
    Fix the issue by effectively dropping the packet on EWOULDBLOCK. This
    will eventually get picked up by the pseudo-TCP recovery mechanism,
    retransmitting the packet and throttling the sender. This should
    hopefully reduce the system resource usage which caused EWOULDBLOCK in
    the first place.
    
    Spotted and debugged by Radosław Kołodziejczyk
    <radek.kolodziejczyk@gmail.com>.
    
    https://bugs.freedesktop.org/show_bug.cgi?id=87344

 agent/agent.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.