[libvirt] [PATCH] Don't sleep in poll() if there is existing SASL decoded data
In the SASL codepath we typically read far more data off the wire than we immediately need. When using a connection from a single thread this isn't a problem, since only our reply will be pending (or an event we can handle directly). When using a connection from multiple threads though, we may read the data from replies from other threads. If those replies occur after our own reply, they'll not be processed. The other thread will then go into poll() and wait for its reply which has already been received and decoded. The solution is to set poll() timeout to 0 if there is pending SASL data. * src/remote/remote_driver.c: Don't sleep in poll() if SASL data exists --- src/remote/remote_driver.c | 16 +++++++++++++++- 1 files changed, 15 insertions(+), 1 deletions(-) diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index b52b780..452ab38 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -10222,6 +10222,14 @@ remoteIOEventLoop(virConnectPtr conn, #ifdef HAVE_PTHREAD_SIGMASK sigset_t oldmask, blockedsigs; #endif + int timeout = -1; + + /* If we have existing SASL decoded data we + * don't want to sleep in the poll(), just + * check if any other FDs are also ready + */ + if (priv->saslDecoded) + timeout = 0; fds[0].events = fds[0].revents = 0; fds[1].events = fds[1].revents = 0; @@ -10257,7 +10265,7 @@ remoteIOEventLoop(virConnectPtr conn, #endif repoll: - ret = poll(fds, ARRAY_CARDINALITY(fds), -1); + ret = poll(fds, ARRAY_CARDINALITY(fds), timeout); if (ret < 0 && errno == EAGAIN) goto repoll; @@ -10267,6 +10275,12 @@ remoteIOEventLoop(virConnectPtr conn, remoteDriverLock(priv); + /* If we have existing SASL decoded data, pretend + * the socket became readable so we consume it + */ + if (priv->saslDecoded) + fds[0].revents |= POLLIN; + if (fds[1].revents) { ssize_t s; DEBUG0("Woken up from poll by other thread"); -- 1.7.3.5
On 02/01/2011 09:09 AM, Daniel P. Berrange wrote:
In the SASL codepath we typically read far more data off the wire than we immediately need. When using a connection from a single thread this isn't a problem, since only our reply will be pending (or an event we can handle directly). When using a connection from multiple threads though, we may read the data from replies from other threads. If those replies occur after our own reply, they'll not be processed. The other thread will then go into poll() and wait for its reply which has already been received and decoded. The solution is to set poll() timeout to 0 if there is pending SASL data.
* src/remote/remote_driver.c: Don't sleep in poll() if SASL data exists --- src/remote/remote_driver.c | 16 +++++++++++++++- 1 files changed, 15 insertions(+), 1 deletions(-)
ACK. Will your refactoring to make things use socket wrappers that do SASL transparently under the hood be complete any time soon? But this patch is good in the meantime. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Tue, Feb 01, 2011 at 09:18:45AM -0700, Eric Blake wrote:
On 02/01/2011 09:09 AM, Daniel P. Berrange wrote:
In the SASL codepath we typically read far more data off the wire than we immediately need. When using a connection from a single thread this isn't a problem, since only our reply will be pending (or an event we can handle directly). When using a connection from multiple threads though, we may read the data from replies from other threads. If those replies occur after our own reply, they'll not be processed. The other thread will then go into poll() and wait for its reply which has already been received and decoded. The solution is to set poll() timeout to 0 if there is pending SASL data.
* src/remote/remote_driver.c: Don't sleep in poll() if SASL data exists --- src/remote/remote_driver.c | 16 +++++++++++++++- 1 files changed, 15 insertions(+), 1 deletions(-)
ACK.
Will your refactoring to make things use socket wrappers that do SASL transparently under the hood be complete any time soon? But this patch is good in the meantime.
Yeah that code is pending soon...it shares this same flaw though and this is urgent to fix because it makes the client lockup. Daniel
On Tue, Feb 01, 2011 at 04:20:16PM +0000, Daniel P. Berrange wrote:
On Tue, Feb 01, 2011 at 09:18:45AM -0700, Eric Blake wrote:
On 02/01/2011 09:09 AM, Daniel P. Berrange wrote:
In the SASL codepath we typically read far more data off the wire than we immediately need. When using a connection from a single thread this isn't a problem, since only our reply will be pending (or an event we can handle directly). When using a connection from multiple threads though, we may read the data from replies from other threads. If those replies occur after our own reply, they'll not be processed. The other thread will then go into poll() and wait for its reply which has already been received and decoded. The solution is to set poll() timeout to 0 if there is pending SASL data.
* src/remote/remote_driver.c: Don't sleep in poll() if SASL data exists --- src/remote/remote_driver.c | 16 +++++++++++++++- 1 files changed, 15 insertions(+), 1 deletions(-)
ACK.
Will your refactoring to make things use socket wrappers that do SASL transparently under the hood be complete any time soon? But this patch is good in the meantime.
Yeah that code is pending soon...it shares this same flaw though and this is urgent to fix because it makes the client lockup.
ACtually that patch broke when compiling without SASL because priv->saslDecoded is not defined in that case. I'm pushing a trivial fix patch for this, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
participants (3)
-
Daniel P. Berrange -
Daniel Veillard -
Eric Blake