[libvirt] [PATCH 1/6] port allocator: make used port bitmap global

Nikolay Shirokovskiy nshirokovskiy at virtuozzo.com
Mon Feb 5 13:28:25 UTC 2018



On 05.02.2018 16:27, Michal Privoznik wrote:
> On 02/05/2018 02:21 PM, Daniel P. Berrangé wrote:
>> 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.
> 
> Okay, so it looks like this can be merged. I mean, v2 can be merged
> (which fixes other issues raised like tests failing, build fixes in some
> drivers, etc.). Nikolay, do you think you can send v2?
> 

Sure!




More information about the libvir-list mailing list