10:06 a.m.
The attached patch (against libvirt-java) implements Java bindings for
libvirt domain events. This version provides a libvirt EventImpl
running in its own Java Thread, and provides per-Connect synchronization
that makes using the bindings thread-safe. (Note the Domain, Network,
StoragePool, and StorageVol methods also synchronize on their Connect
object, as required by libvirt. I have similar changes for
NodeDevice.java that need to be made when that code is checked in.)
This version of the patch also implements and uses an enum class
(DomainEvent.Type), as suggested by Tóth István.
IMPORTANT: THIS PATCH WILL BREAK THINGS UNLESS THE NEXT [PATCH 2/2] IS
APPLIED TO libvirt FIRST. Also, libvirt must be compiled WITH_PTHREADS
for Java events to work.
Dave
10:34 a.m.
On Tue, Nov 18, 2008 at 11:06:10AM -0500, David Lively wrote:
The attached patch (against libvirt-java) implements Java bindings
for
libvirt domain events. This version provides a libvirt EventImpl
running in its own Java Thread, and provides per-Connect synchronization
that makes using the bindings thread-safe. (Note the Domain, Network,
StoragePool, and StorageVol methods also synchronize on their Connect
object, as required by libvirt. I have similar changes for
NodeDevice.java that need to be made when that code is checked in.)
This version of the patch also implements and uses an enum class
(DomainEvent.Type), as suggested by Tóth István.
IMPORTANT: THIS PATCH WILL BREAK THINGS UNLESS THE NEXT [PATCH 2/2] IS
APPLIED TO libvirt FIRST. Also, libvirt must be compiled WITH_PTHREADS
for Java events to work.
my main concern is to keep Java bindings working on Windows. Does our
Win32 build includes pthreads ? I would love to get some positive
feedback on that point (or even better that the java bindings do indeed
work on Win32 with that patch).
Daniel
--
Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/
daniel(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
10:51 a.m.
On Tue, Nov 18, 2008 at 11:06:10AM -0500, David Lively wrote:
The attached patch (against libvirt-java) implements Java bindings
for
libvirt domain events. This version provides a libvirt EventImpl
running in its own Java Thread, and provides per-Connect synchronization
that makes using the bindings thread-safe. (Note the Domain, Network,
StoragePool, and StorageVol methods also synchronize on their Connect
object, as required by libvirt. I have similar changes for
NodeDevice.java that need to be made when that code is checked in.)
I don't particularly like the event loop code because it is adding a huge
pile of non-portable JNI code that won't work on Windows, which lacks
pipe() and poll(). Java already provides a portable pure Java API for
building a poll() like event loop in form of NIO.
http://www.xhaus.com/alan/python/jynio/select.html
The addition of the 'synchronized' annotatioons is something we need
regardless of the rest of the patch, since our API contract dictates
that only a single thread is allowed to use a single virConnectPtr
at once.
This version of the patch also implements and uses an enum class
(DomainEvent.Type), as suggested by Tóth István.
IMPORTANT: THIS PATCH WILL BREAK THINGS UNLESS THE NEXT [PATCH 2/2] IS
APPLIED TO libvirt FIRST. Also, libvirt must be compiled WITH_PTHREADS
for Java events to work.
Daniel
--
|: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
12:12 p.m.
On Tue, 2008-11-18 at 16:51 +0000, Daniel P. Berrange wrote:
On Tue, Nov 18, 2008 at 11:06:10AM -0500, David Lively wrote:
> The attached patch (against libvirt-java) implements Java bindings for
> libvirt domain events. This version provides a libvirt EventImpl
> running in its own Java Thread, and provides per-Connect synchronization
> that makes using the bindings thread-safe. (Note the Domain, Network,
> StoragePool, and StorageVol methods also synchronize on their Connect
> object, as required by libvirt. I have similar changes for
> NodeDevice.java that need to be made when that code is checked in.)
I don't particularly like the event loop code because it is adding a huge
pile of non-portable JNI code that won't work on Windows, which lacks
pipe() and poll(). Java already provides a portable pure Java API for
building a poll() like event loop in form of NIO.
http://www.xhaus.com/alan/python/jynio/select.html
Yeah, Daniel V and I had briefly considered this, and rejected it on the
basis of "it's complicated" and (more importantly) some negative
feedback I hear from our Java folks on the java.nio Select mechanism.
But I agree the java.nio Select mechanism should greatly decrease the
amount of JNI code in the Java EventImpl. I need to look over the docs
again, but I think it's "just" a matter of implementing a
SelectableChannel on top of a fd. (That JNI code will presumably be
very different in Win32 and Unix, but it should be a relatively small
amount of JNI code in comparison to my current impl.)
So I'll look over the java.nio Select documentation and start thinking
about a more portable approach ... (and also talk more with our Java
folks about their Select gripes).
Dave
The addition of the 'synchronized' annotatioons is something
we need
regardless of the rest of the patch, since our API contract dictates
that only a single thread is allowed to use a single virConnectPtr
at once.
> This version of the patch also implements and uses an enum class
> (DomainEvent.Type), as suggested by Tóth István.
>
> IMPORTANT: THIS PATCH WILL BREAK THINGS UNLESS THE NEXT [PATCH 2/2] IS
> APPLIED TO libvirt FIRST. Also, libvirt must be compiled WITH_PTHREADS
> for Java events to work.
Daniel
1:34 a.m.
On Tue, Nov 18, 2008 at 01:12:42PM -0500, David Lively wrote:
On Tue, 2008-11-18 at 16:51 +0000, Daniel P. Berrange wrote:
> On Tue, Nov 18, 2008 at 11:06:10AM -0500, David Lively wrote:
> > The attached patch (against libvirt-java) implements Java bindings for
> > libvirt domain events. This version provides a libvirt EventImpl
> > running in its own Java Thread, and provides per-Connect synchronization
> > that makes using the bindings thread-safe. (Note the Domain, Network,
> > StoragePool, and StorageVol methods also synchronize on their Connect
> > object, as required by libvirt. I have similar changes for
> > NodeDevice.java that need to be made when that code is checked in.)
>
> I don't particularly like the event loop code because it is adding a huge
> pile of non-portable JNI code that won't work on Windows, which lacks
> pipe() and poll(). Java already provides a portable pure Java API for
> building a poll() like event loop in form of NIO.
>
> http://www.xhaus.com/alan/python/jynio/select.html
Yeah, Daniel V and I had briefly considered this, and rejected it on the
basis of "it's complicated" and (more importantly) some negative
feedback I hear from our Java folks on the java.nio Select mechanism.
But I agree the java.nio Select mechanism should greatly decrease the
amount of JNI code in the Java EventImpl. I need to look over the docs
again, but I think it's "just" a matter of implementing a
SelectableChannel on top of a fd. (That JNI code will presumably be
very different in Win32 and Unix, but it should be a relatively small
amount of JNI code in comparison to my current impl.)
So I'll look over the java.nio Select documentation and start thinking
about a more portable approach ... (and also talk more with our Java
folks about their Select gripes).
I guess it's better to invest a bit more time and come up with a
solution relying as much as possible on Java threading, I/O and
synchronization. After all we should capitalize as much as possible
on the portability work done in the Java engine, and limit the
C part of the bindings to the strict minimum JNI (as much as possible).
On one hand we want the bindings to be the easiest possible to use
and avoid threading limitation imposed to the client code, on the other
hand limit the C part on those issue, of course that means growing
the java side of the bindings, but it really should be easier to
maintain and port than equivalent C code, even if NIO is not the
nicest Java API :-\
Daniel
--
Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/
daniel(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
4:48 a.m.
On Wed, Nov 19, 2008 at 08:34:41AM +0100, Daniel Veillard wrote:
On Tue, Nov 18, 2008 at 01:12:42PM -0500, David Lively wrote:
> On Tue, 2008-11-18 at 16:51 +0000, Daniel P. Berrange wrote:
> > On Tue, Nov 18, 2008 at 11:06:10AM -0500, David Lively wrote:
> > > The attached patch (against libvirt-java) implements Java bindings for
> > > libvirt domain events. This version provides a libvirt EventImpl
> > > running in its own Java Thread, and provides per-Connect synchronization
> > > that makes using the bindings thread-safe. (Note the Domain, Network,
> > > StoragePool, and StorageVol methods also synchronize on their Connect
> > > object, as required by libvirt. I have similar changes for
> > > NodeDevice.java that need to be made when that code is checked in.)
> >
> > I don't particularly like the event loop code because it is adding a huge
> > pile of non-portable JNI code that won't work on Windows, which lacks
> > pipe() and poll(). Java already provides a portable pure Java API for
> > building a poll() like event loop in form of NIO.
> >
> > http://www.xhaus.com/alan/python/jynio/select.html
>
> Yeah, Daniel V and I had briefly considered this, and rejected it on the
> basis of "it's complicated" and (more importantly) some negative
> feedback I hear from our Java folks on the java.nio Select mechanism.
>
> But I agree the java.nio Select mechanism should greatly decrease the
> amount of JNI code in the Java EventImpl. I need to look over the docs
> again, but I think it's "just" a matter of implementing a
> SelectableChannel on top of a fd. (That JNI code will presumably be
> very different in Win32 and Unix, but it should be a relatively small
> amount of JNI code in comparison to my current impl.)
>
> So I'll look over the java.nio Select documentation and start thinking
> about a more portable approach ... (and also talk more with our Java
> folks about their Select gripes).
I guess it's better to invest a bit more time and come up with a
solution relying as much as possible on Java threading, I/O and
synchronization. After all we should capitalize as much as possible
on the portability work done in the Java engine, and limit the
C part of the bindings to the strict minimum JNI (as much as possible).
On one hand we want the bindings to be the easiest possible to use
and avoid threading limitation imposed to the client code, on the other
hand limit the C part on those issue, of course that means growing
the java side of the bindings, but it really should be easier to
maintain and port than equivalent C code, even if NIO is not the
nicest Java API :-\
Also, while I remember any event loop implementation in Java should
be an optional add-on class, not a mandatory part of the Java libvirt
bindings. Applications may well already have an event loop they wish
to use - for example a java desktop application will have an event
loop provided by GTK or QT. So all that would be require is a Java
binding to the libvirt-glib module, so you cn register libvirt with
Glib from Java.
Daniel
--
|: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
9:35 a.m.
On Wed, 2008-11-19 at 10:48 +0000, Daniel P. Berrange wrote:
On Wed, Nov 19, 2008 at 08:34:41AM +0100, Daniel Veillard wrote:
> On Tue, Nov 18, 2008 at 01:12:42PM -0500, David Lively wrote:
> > On Tue, 2008-11-18 at 16:51 +0000, Daniel P. Berrange wrote:
> > > On Tue, Nov 18, 2008 at 11:06:10AM -0500, David Lively wrote:
> > > > The attached patch (against libvirt-java) implements Java bindings
for
> > > > libvirt domain events. This version provides a libvirt EventImpl
> > > > running in its own Java Thread, and provides per-Connect
synchronization
> > > > that makes using the bindings thread-safe. (Note the Domain,
Network,
> > > > StoragePool, and StorageVol methods also synchronize on their
Connect
> > > > object, as required by libvirt. I have similar changes for
> > > > NodeDevice.java that need to be made when that code is checked in.)
> > >
> > > I don't particularly like the event loop code because it is adding a
huge
> > > pile of non-portable JNI code that won't work on Windows, which lacks
> > > pipe() and poll(). Java already provides a portable pure Java API for
> > > building a poll() like event loop in form of NIO.
> > >
> > > http://www.xhaus.com/alan/python/jynio/select.html
> >
> > Yeah, Daniel V and I had briefly considered this, and rejected it on the
> > basis of "it's complicated" and (more importantly) some negative
> > feedback I hear from our Java folks on the java.nio Select mechanism.
> >
> > But I agree the java.nio Select mechanism should greatly decrease the
> > amount of JNI code in the Java EventImpl. I need to look over the docs
> > again, but I think it's "just" a matter of implementing a
> > SelectableChannel on top of a fd. (That JNI code will presumably be
> > very different in Win32 and Unix, but it should be a relatively small
> > amount of JNI code in comparison to my current impl.)
> >
> > So I'll look over the java.nio Select documentation and start thinking
> > about a more portable approach ... (and also talk more with our Java
> > folks about their Select gripes).
>
> I guess it's better to invest a bit more time and come up with a
> solution relying as much as possible on Java threading, I/O and
> synchronization. After all we should capitalize as much as possible
> on the portability work done in the Java engine, and limit the
> C part of the bindings to the strict minimum JNI (as much as possible).
Agreed -- BUT if we don't make libvirt allow asynchronous EventImpl
callbacks (the second patch in this series (plus requiring pthreads,
which sounds like a Big Deal for WIN32)), we'll have to stop the
EventImpl callbacks from happening:
(a) at the same time *ANY* connection (in the same process) is in use
(because EventImpl callbacks aren't necessarily associated with any
particular connection, though they might be)
(b) at the same time another EventImpl callback is happening
The patch already synchronizes operations using virConnect objects with
each other. To avoid making illegal EventImpl callbacks from Java for
the current libvirt, I have to lock every Connect object known to Java
and hold off creating new connections (via open & friends) around an
EventImpl callback. This sounds rather appalling to me, but it's
starting to sound like the only practical route in the short term
(unless it turns out we can rely on pthreads in WIN32 ...).
Alternatively, I could just use one global lock for everything, but that
essentially single-threads all libvirt operations, which seems *really*
undesirable.
> On one hand we want the bindings to be the easiest possible to
use
> and avoid threading limitation imposed to the client code, on the other
> hand limit the C part on those issue, of course that means growing
> the java side of the bindings, but it really should be easier to
> maintain and port than equivalent C code, even if NIO is not the
> nicest Java API :-\
Yeah, I suspect so. I'm starting to implement this path ...
Also, while I remember any event loop implementation in Java should
be an optional add-on class, not a mandatory part of the Java libvirt
bindings. Applications may well already have an event loop they wish
to use - for example a java desktop application will have an event
loop provided by GTK or QT. So all that would be require is a Java
binding to the libvirt-glib module, so you cn register libvirt with
Glib from Java.
Gotcha. I've just installed glib-java-devel, so I'll make sure this
easily integrates with a glib event loop as exposed in glib-java.
Dave
9:57 a.m.
On Wed, Nov 19, 2008 at 10:35:45AM -0500, David Lively wrote:
On Wed, 2008-11-19 at 10:48 +0000, Daniel P. Berrange wrote:
> Also, while I remember any event loop implementation in Java should
> be an optional add-on class, not a mandatory part of the Java libvirt
> bindings. Applications may well already have an event loop they wish
> to use - for example a java desktop application will have an event
> loop provided by GTK or QT. So all that would be require is a Java
> binding to the libvirt-glib module, so you cn register libvirt with
> Glib from Java.
Gotcha. I've just installed glib-java-devel, so I'll make sure this
easily integrates with a glib event loop as exposed in glib-java.
The current libvirt-glib integration code is here
http://libvirt.org/hg/libvirt-glib/
Though I'll need to update it once I comitt the virFreeCallback changes
shortly...
Daniel
--
|: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
10:22 a.m.
On Wed, 2008-11-19 at 10:35 -0500, David Lively wrote:
The patch already synchronizes operations using virConnect objects
with
each other. To avoid making illegal EventImpl callbacks from Java for
the current libvirt, I have to lock every Connect object known to Java
and hold off creating new connections (via open & friends) around an
EventImpl callback. This sounds rather appalling to me, but it's
starting to sound like the only practical route in the short term
(unless it turns out we can rely on pthreads in WIN32 ...).
Hmmm ... maybe the less appalling :-) route is practical. Currently, we
require only the Windows equivalent of a simple pthread mutex. We just
need to support declaration, initialization, lock, unlock, and
destruction, something like the following (thanks to Tom Hazel for
pointing me to the Windows Mutex stuff):
#if (defined _WIN32 || defined __WIN32__)
#define PTHREAD_MUTEX_T(v) HANDLE v
#define pthread_mutex_init(lk,p) ((*(lk)) = CreateMutex(0, FALSE, 0))
#define pthread_mutex_destroy(lk) CloseHandle(*(lk))
#define pthread_mutex_lock(lk) WaitForSingleObject(*(lk), INFINITE)
#define pthread_mutex_unlock(lk) ReleaseMutex(*(lk))
#define pthread_sigmask(h, s, o) sigprocmask((h), (s), (o))
#endif
I'm not a Windows guy, so maybe I'm missing something. But this doesn't
seem like a Big Deal ...
Dave
7:49 a.m.
On Wed, Nov 19, 2008 at 11:22:31AM -0500, David Lively wrote:
On Wed, 2008-11-19 at 10:35 -0500, David Lively wrote:
> The patch already synchronizes operations using virConnect objects with
> each other. To avoid making illegal EventImpl callbacks from Java for
> the current libvirt, I have to lock every Connect object known to Java
> and hold off creating new connections (via open & friends) around an
> EventImpl callback. This sounds rather appalling to me, but it's
> starting to sound like the only practical route in the short term
> (unless it turns out we can rely on pthreads in WIN32 ...).
Hmmm ... maybe the less appalling :-) route is practical. Currently, we
require only the Windows equivalent of a simple pthread mutex. We just
need to support declaration, initialization, lock, unlock, and
destruction, something like the following (thanks to Tom Hazel for
pointing me to the Windows Mutex stuff):
#if (defined _WIN32 || defined __WIN32__)
#define PTHREAD_MUTEX_T(v) HANDLE v
#define pthread_mutex_init(lk,p) ((*(lk)) = CreateMutex(0, FALSE, 0))
#define pthread_mutex_destroy(lk) CloseHandle(*(lk))
#define pthread_mutex_lock(lk) WaitForSingleObject(*(lk), INFINITE)
#define pthread_mutex_unlock(lk) ReleaseMutex(*(lk))
#define pthread_sigmask(h, s, o) sigprocmask((h), (s), (o))
#endif
I'm not a Windows guy, so maybe I'm missing something. But this doesn't
seem like a Big Deal ...
note that libxml2 that we rely on has a fully ported mutex basic API
in libxml/threads.h
/*
* xmlMutex are a simple mutual exception locks.
*/
typedef struct _xmlMutex xmlMutex;
typedef xmlMutex *xmlMutexPtr;
/*
* xmlRMutex are reentrant mutual exception locks.
*/
typedef struct _xmlRMutex xmlRMutex;
typedef xmlRMutex *xmlRMutexPtr;
XMLPUBFUN xmlMutexPtr XMLCALL
xmlNewMutex (void);
XMLPUBFUN void XMLCALL
xmlMutexLock (xmlMutexPtr tok);
XMLPUBFUN void XMLCALL
xmlMutexUnlock (xmlMutexPtr tok);
XMLPUBFUN void XMLCALL
xmlFreeMutex (xmlMutexPtr tok);
XMLPUBFUN xmlRMutexPtr XMLCALL
xmlNewRMutex (void);
XMLPUBFUN void XMLCALL
xmlRMutexLock (xmlRMutexPtr tok);
XMLPUBFUN void XMLCALL
xmlRMutexUnlock (xmlRMutexPtr tok);
XMLPUBFUN void XMLCALL
xmlFreeRMutex (xmlRMutexPtr tok);
in case you really want to do the exclusive locking at the C level
while still being portable.
Still it's probably better to try to implement most of this at the
Java level, at least IMHO,
Daniel
--
Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/
daniel(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
2:27 p.m.
On Fri, 2008-11-21 at 14:49 +0100, Daniel Veillard wrote:
On Wed, Nov 19, 2008 at 11:22:31AM -0500, David Lively wrote:
> On Wed, 2008-11-19 at 10:35 -0500, David Lively wrote:
> > The patch already synchronizes operations using virConnect objects with
> > each other. To avoid making illegal EventImpl callbacks from Java for
> > the current libvirt, I have to lock every Connect object known to Java
> > and hold off creating new connections (via open & friends) around an
> > EventImpl callback. This sounds rather appalling to me, but it's
> > starting to sound like the only practical route in the short term
> > (unless it turns out we can rely on pthreads in WIN32 ...).
>
note that libxml2 that we rely on has a fully ported mutex basic
API
in libxml/threads.h
....
in case you really want to do the exclusive locking at the C level
while still being portable.
Thanks for the pointer. This could be used. I'd assume we'd want to
change the mutex usage in datatypes.c as well, for consistency.
Still it's probably better to try to implement most of this at
the
Java level, at least IMHO,
Just to be clear, I've been planning on keeping the per-Connect Java
synchronization from the original patch. (And I think you guys have
bought into that much, so far.)
So here we're discussing the concurrency requirements imposed upon the
callbacks made by an asynchronous external (non-libvirtd) EventImpl.
Currently, we must make sure that no other connection is in use (and no
other EventImpl callback is in progress).
I'm arguing this is far too restrictive since it basically means locking
ALL connections before an event can be recognized. So if one connection
is performing some long-running operation (say something
storage-related), no events can be delivered to *any* connection until
the operation completes (and then we'd better hope another long-running
operation hasn't been started on another connection in the mean time).
Keep in mind I'm not proposing we make all libvirt C public interfaces
threadsafe. We only need to make sure the paths reachable from external
event impls (in the remote and xen-inotify drivers) are threadsafe. I
believe the patch I submitted (2/2 in this series) does this for the
remote driver, albeit via the PTHREADS_MUTEX macros that currently have
no Win32 impl. (And I'll do the same for the xen-inotify driver once
it's in. I haven't looked into it yet ...)
I'll happily change the existing mutex usage in datatypes.c to the
libxml one. And I'll use the same mutex impl in the remote and
xen-inotify drivers. But I'd rather not bother until you guys agree
this is desirable ...
Thanks,
Dave
P.S. I realize this isn't really a practical problem right now. It's
more of a future limit on scalability. But because it's part of a new
interface (virEventRegisterImpl), it seems worth it to try and specify
this nicely now (as opposed to relaxing the concurrency requirements in
a later version, which is a kind of API change).
2:45 p.m.
On Fri, Nov 21, 2008 at 03:27:49PM -0500, David Lively wrote:
On Fri, 2008-11-21 at 14:49 +0100, Daniel Veillard wrote:
> On Wed, Nov 19, 2008 at 11:22:31AM -0500, David Lively wrote:
> note that libxml2 that we rely on has a fully ported mutex basic API
> in libxml/threads.h
>
....
>
> in case you really want to do the exclusive locking at the C level
> while still being portable.
Thanks for the pointer. This could be used. I'd assume we'd want to
change the mutex usage in datatypes.c as well, for consistency.
> Still it's probably better to try to implement most of this at the
> Java level, at least IMHO,
Just to be clear, I've been planning on keeping the per-Connect Java
synchronization from the original patch. (And I think you guys have
bought into that much, so far.)
So here we're discussing the concurrency requirements imposed upon the
callbacks made by an asynchronous external (non-libvirtd) EventImpl.
Currently, we must make sure that no other connection is in use (and no
other EventImpl callback is in progress).
To me this is a killer argument for virConnectPtr being made threadsafe.
We're creating problems for ourselves by trying to hack around it.
I'm arguing this is far too restrictive since it basically means
locking
ALL connections before an event can be recognized. So if one connection
is performing some long-running operation (say something
storage-related), no events can be delivered to *any* connection until
the operation completes (and then we'd better hope another long-running
operation hasn't been started on another connection in the mean time).
Keep in mind I'm not proposing we make all libvirt C public interfaces
threadsafe.
I am :-)
The common "design pattern" is java is heavily multi-threaded and
doing a half-way house solution which makes only some scenrios in
the API thread-safe is still going to leave the Java libvirt API
limited in comparison with native Java APIs & desirable application
use cases.
We already have no choice but to make the QEMU, LXC, UML and OpenVZ
drivers fully threadsafe in order to allow the libvirtd daemon to be
threaded for sake of scability, and I have more or less completed
this work already. Making the remaining Xen driver thread-safe too
is trivial by comparison, since it has very little state. The main
remaining issue is the global & per-connection error variables which
are not thread safe. If we added a 3rd thread-local variable which
was set in parallel with these existing ones we could have a properly
threadsafe API, and simply say don't access the global error objects
if using the API in a multi-thread context.
This isn't just a problem for Java either - we jump through some
horrible hoops in virt-manger python code because of the thread
restrictions in the our public API.
The only alternative to a fully threaded API is a the idea of adding
a virConnectClone() operation which gives you a duplicate handled on
to the existing connection for use in a separate thread. The more I
consider this though, the less useful it seems - we'd still have the
same issues with the global error object, and we'd have todo internal
synchronization which is just as complex as the fully threadsafe
work.
We only need to make sure the paths reachable from external
event impls (in the remote and xen-inotify drivers) are threadsafe. I
believe the patch I submitted (2/2 in this series) does this for the
remote driver, albeit via the PTHREADS_MUTEX macros that currently have
no Win32 impl. (And I'll do the same for the xen-inotify driver once
it's in. I haven't looked into it yet ...)
I'll happily change the existing mutex usage in datatypes.c to the
libxml one. And I'll use the same mutex impl in the remote and
xen-inotify drivers. But I'd rather not bother until you guys agree
this is desirable ...
The datatypes.c code used to use the libxml mutex definitions but
I removed them because it was not a complete solution. We need to have
more than just the mutex datatypes & libxml does not provide wrappers
for all the thread APIs & data types we require, so we have no choice
but to use proper pthread types, and a pthreads API library on Win32)
Rich Jones has already packaged up such a library for Win32 in Fedora:
http://annexia.org/tmp/mingw/fedora-10/src/SRPMS/mingw32-pthreads-2.8.0-2...
P.S. I realize this isn't really a practical problem right now.
It's
more of a future limit on scalability. But because it's part of a new
interface (virEventRegisterImpl), it seems worth it to try and specify
this nicely now (as opposed to relaxing the concurrency requirements in
a later version, which is a kind of API change).
Daniel
--
|: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
10:56 a.m.
Now that I'm actually wading around in the Java java.nio.channels Select
swamp (getting this implemented), this is looking like it has exactly
the same portability problems as my original code.
This approach requires providing our own Selector (and SelectorProvider,
corresponding handle/timeout Channels, and SelectionKeys), with our own
native implementation of something that can provide select/poll
functionality (because there's no way to get that functionality via Java
for arbitrary unix fds; can't even construct java FileDescriptors from
them in native code because FileDescriptors are declared final). And we
also must implement Selector.wakeup() that can interrupt a select
operation in another thread (requiring something like the pipe() in my
original code - since the select operation will itself be in native
code, so can't rely on Java thread interruption).
So in the end this looks more like a java.nio.channels.Selector-style
interface wrapped around a libvirt EventImpl (which might as well be the
one copied from qemud). Certainly that exports a more standard
interface for integration with a foreign event loop, but at this point,
that looks like the only benefit. For now, I'll continue, assuming the
more standard interface is worth the extra code. The Java side of this
is basically done, with "only" the JNI code to write.
Just wanted to set expectations ...
Dave
On Wed, 2008-11-19 at 08:34 +0100, Daniel Veillard wrote:
On Tue, Nov 18, 2008 at 01:12:42PM -0500, David Lively wrote:
> On Tue, 2008-11-18 at 16:51 +0000, Daniel P. Berrange wrote:
> > On Tue, Nov 18, 2008 at 11:06:10AM -0500, David Lively wrote:
> > > The attached patch (against libvirt-java) implements Java bindings for
> > > libvirt domain events. This version provides a libvirt EventImpl
> > > running in its own Java Thread, and provides per-Connect synchronization
> > > that makes using the bindings thread-safe. (Note the Domain, Network,
> > > StoragePool, and StorageVol methods also synchronize on their Connect
> > > object, as required by libvirt. I have similar changes for
> > > NodeDevice.java that need to be made when that code is checked in.)
> >
> > I don't particularly like the event loop code because it is adding a huge
> > pile of non-portable JNI code that won't work on Windows, which lacks
> > pipe() and poll(). Java already provides a portable pure Java API for
> > building a poll() like event loop in form of NIO.
> >
> > http://www.xhaus.com/alan/python/jynio/select.html
>
> Yeah, Daniel V and I had briefly considered this, and rejected it on the
> basis of "it's complicated" and (more importantly) some negative
> feedback I hear from our Java folks on the java.nio Select mechanism.
>
> But I agree the java.nio Select mechanism should greatly decrease the
> amount of JNI code in the Java EventImpl. I need to look over the docs
> again, but I think it's "just" a matter of implementing a
> SelectableChannel on top of a fd. (That JNI code will presumably be
> very different in Win32 and Unix, but it should be a relatively small
> amount of JNI code in comparison to my current impl.)
>
> So I'll look over the java.nio Select documentation and start thinking
> about a more portable approach ... (and also talk more with our Java
> folks about their Select gripes).
I guess it's better to invest a bit more time and come up with a
solution relying as much as possible on Java threading, I/O and
synchronization. After all we should capitalize as much as possible
on the portability work done in the Java engine, and limit the
C part of the bindings to the strict minimum JNI (as much as possible).
On one hand we want the bindings to be the easiest possible to use
and avoid threading limitation imposed to the client code, on the other
hand limit the C part on those issue, of course that means growing
the java side of the bindings, but it really should be easier to
maintain and port than equivalent C code, even if NIO is not the
nicest Java API :-\
Daniel
5833
days inactive
5847
days old
12 comments
3 participants
participants (3)
-
Daniel P. Berrange -
Daniel Veillard -
David Lively