On Mon, Feb 05, 2018 at 02:11:24PM +0100, Peter Krempa wrote: > On Mon, Feb 05, 2018 at 12:55:11 +0000, Daniel Berrange wrote: > > On Mon, Feb 05, 2018 at 01:39:56PM +0100, Michal Privoznik wrote: > > > On 02/01/2018 08:51 AM, Nikolay Shirokovskiy wrote: > > > > > > > > > > > > On 29.01.2018 09:09, Michal Privoznik wrote: > > > >> On 12/20/2017 07:35 AM, Nikolay Shirokovskiy wrote: > > > >>> Host tcp4/tcp6 ports is a global resource thus we need to make > > > >>> port accounting also global or we have issues described in [1] when > > > >>> port allocator ranges of different instances are overlapped (which > > > >>> is by default for qemu for example). > > > >>> > > > >>> Let's have only one global port allocator object that take care > > > >>> of the entire ports range (0 - 65535) and introduce port range object > > > >>> for clients to specify desired auto allocation band. > > > >>> > > > >>> [1] https://www.redhat.com/archives/libvir-list/2017-December/msg00600.html > > > >>> --- > > > >>> src/bhyve/bhyve_driver.c | 4 +- > > > >>> src/bhyve/bhyve_utils.h | 2 +- > > > >>> src/libvirt_private.syms | 3 +- > > > >>> src/libxl/libxl_conf.c | 8 +-- > > > >>> src/libxl/libxl_conf.h | 8 +-- > > > >>> src/libxl/libxl_driver.c | 18 +++--- > > > >>> src/qemu/qemu_conf.h | 6 +- > > > >>> src/qemu/qemu_driver.c | 30 +++++----- > > > >>> src/util/virportallocator.c | 125 ++++++++++++++++++++++++++++------------- > > > >>> src/util/virportallocator.h | 20 ++++--- > > > >>> tests/bhyvexml2argvtest.c | 6 +- > > > >>> tests/libxlxml2domconfigtest.c | 8 +-- > > > >>> tests/virportallocatortest.c | 48 ++++++++++------ > > > >>> 13 files changed, 175 insertions(+), 111 deletions(-) > > > >>> > > > >> > > > >>> diff --git a/src/util/virportallocator.c b/src/util/virportallocator.c > > > >>> index fcd4f74..cd64356 100644 > > > >>> --- a/src/util/virportallocator.c > > > >>> +++ b/src/util/virportallocator.c > > > >>> @@ -35,10 +35,14 @@ > > > >>> > > > >>> #define VIR_FROM_THIS VIR_FROM_NONE > > > >>> > > > >>> +typedef struct _virPortAllocator virPortAllocator; > > > >>> +typedef virPortAllocator *virPortAllocatorPtr; > > > >>> struct _virPortAllocator { > > > >>> virObjectLockable parent; > > > >>> virBitmapPtr bitmap; > > > >>> +}; > > > >>> > > > >>> +struct _virPortRange { > > > >>> char *name; > > > >>> > > > >>> unsigned short start; > > > >>> @@ -48,6 +52,7 @@ struct _virPortAllocator { > > > >>> }; > > > >>> > > > >>> static virClassPtr virPortAllocatorClass; > > > >>> +static virPortAllocatorPtr virPortAllocatorInstance; > > > >> > > > >> I wonder if this is the way to go. I mean, this virPortAllocatorInstance > > > >> is going to be a global variable that will never be freed (even if we > > > >> wanted to). I mean, if virPortRange had a pointer to virPortAllocator > > > > > > > > Not sure why we need to free it. It is like global variables for classes, > > > > we don't need to free them yet. As to libxlxml2domconfigtest it can be > > > > fixed just like virportallocatortest by releasing all acquired ports. > > > > > > Well, okay. Disregard my suggestion. However, what we still need to > > > discuss is the driver separation work of Daniel's. Dan, how badly will > > > this hit you if I merged these? In another thread I suggested to turn > > > this into a separate deaemon (which might be overkill). > > > > The caching of the used ports in the bitmap is just an optimization, to > > avoid us having to retry the bind()+listen() on every port we've previously > > got in use. If we split the daemon, if multiple daemons all need port > > allocation tracking, they'll get separate virPortAllocator bitmap instances. > > Since one daemon won't see what other daemon has in use, it will mean that > > we must try to bind()+listen() on ports that the other daemon has in use. > > Thereafter we'll have cached that usage the bitmap. > > > > The main downside is that if one daemon releases a port, the other daemon > > won't see that release. This is only a significant problem if the 2 (or > > more) daemons are using the same port range. This would, however, be > > exactly the same when we have a per-QEMU instance daemon. The proposed > > change, however, does not make life worse than it already is in this > > respect. > > > > IOW, we'll probably have some trouble, but that's not a reason to reject > > this proposal. It is just one of many things we'll need to figure out > > wrt unique assignment. > > Well, you get slightly worse odds of having the same kind of race if you > have multiple instances of the port allocation approach in multiple > processes. > > Our problem is that when we bind()+listen() we still need to close that > port and have qemu open it again. This race window is still present but > will be worsened by multiple of these doing the same thing. > > When qemu will be able to accept the socket via FD passing then this > would be strictly an optimization, but until then it worsens the odds of > failure. I have patches to let QEMU accept a preopened FD for chardevs. VNC / SPICE are the other big ones we hit. I should make fixing those a higher priority. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :| -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
