[libvirt] [PATCHv2] netcf driver: use a single netcf handle for all connections

[Doug Goldstein]

On Wed, Sep 11, 2013 at 10:43 AM, Michal Privoznik <mprivozn at redhat.com> wrote:
> On 11.09.2013 17:37, Doug Goldstein wrote:
>> On Wed, Sep 11, 2013 at 10:06 AM, Laine Stump <laine at laine.org> wrote:
>>> This resolves: https://bugzilla.redhat.com/show_bug.cgi?id=983026
>>>
>>> The netcf interface driver previously had no state driver associated
>>> with it - as a connection was opened, it would create a new netcf
>>> instance just for that connection, and close it when it was
>>> finished. the problem with this is that each connection to libvirt
>>> used up a netlink socket, and there is a per process maximum of ~1000
>>> netlink sockets.
>>>
>>> The solution is to create a state driver to go along with the netcf
>>> driver. The state driver will opens a netcf instance, then all
>>> connections share that same netcf instance, thus only a single
>>> netlink socket will be used no matter how many connections are mde to
>>> libvirtd.
>>>
>>> This was rather simple to do - a new virObjectLockable class is
>>> created for the single driverState object, which is created in
>>> netcfStateInitialize and contains the single netcf handle; instead of
>>> creating a new object for each client connection, netcfInterfaceOpen
>>> now just increments the driverState object's reference count and puts
>>> a pointer to it into the connection's privateData. Similarly,
>>> netcfInterfaceClose() just un-refs the driverState object (as does
>>> netcfStateCleanup()), and virNetcfInterfaceDriverStateDispose()
>>> handles closing the netcf instance. Since all the functions already
>>> have locking around them, the static lock functions used by all
>>> functions just needed to be changed to call virObjectLock() and
>>> virObjectUnlock() instead of directly calling the virMutex* functions.
>>> ---
>>> Changes from V1:
>>>
>>> * make driverState a static.
>>>
>>> * switch to using a virObjectLockable for driverState, at
>>>   Eric's suggestion.
>>>
>>> * add a simple error message if ncf_init() fails.
>>>
>>> Again, I've tried this with a small number of simultaneous connections
>>> (including virt-manager), but I don't have a ready-made stress test.
>>>
>>>
>>>  src/interface/interface_backend_netcf.c | 173 +++++++++++++++++++++++---------
>>>  1 file changed, 125 insertions(+), 48 deletions(-)
>>>
>>> diff --git a/src/interface/interface_backend_netcf.c b/src/interface/interface_backend_netcf.c
>>> index f47669e..627c225 100644
>>> --- a/src/interface/interface_backend_netcf.c
>>> +++ b/src/interface/interface_backend_netcf.c
>>> @@ -41,19 +41,119 @@
>>>  /* Main driver state */
>>>  typedef struct
>>>  {
>>> -    virMutex lock;
>>> +    virObjectLockable parent;
>>>      struct netcf *netcf;
>>>  } virNetcfDriverState, *virNetcfDriverStatePtr;
>>>
>>> +static virClassPtr virNetcfDriverStateClass;
>>> +static void virNetcfDriverStateDispose(void *obj);
>>>
>>> -static void interfaceDriverLock(virNetcfDriverStatePtr driver)
>>> +static int
>>> +virNetcfDriverStateOnceInit(void)
>>> +{
>>> +    if (!(virNetcfDriverStateClass = virClassNew(virClassForObjectLockable(),
>>> +                                       "virNetcfDriverState",
>>> +                                       sizeof(virNetcfDriverState),
>>> +                                       virNetcfDriverStateDispose)))
>>> +        return -1;
>>> +    return 0;
>>
>> Bad spacing?
>>
>>> +}
>>> +
>>> +VIR_ONCE_GLOBAL_INIT(virNetcfDriverState)
>>> +
>>> +static virNetcfDriverStatePtr driverState = NULL;
>
>>
>> So my understanding of libvirt's object and locking code isn't too
>> good so I hope someone else will review. But I don't see
>> virNetcfDriverStateOnceInit() called anywhere and I don't see
>> virNetcfDriverStateInitialize() defined anywhere? Are those suppose to
>> be one in the same or is there some magical macro expansion going on
>> somewhere that I just don't know about.
>>
>
> The VIR_ONCE_GLOBAL_INIT(virDummy) macro creates a function called
> virDummyInitialize() which when called for the 1st time calls
> virDummyOnceInit(). Any subsequent call to virDummyInitialize() results
> in immediate return.
>
> Michal
>

Thanks Michal. ACK this patch. And ignore the spacing comment. Gmail
ate some white space.

-- 
Doug Goldstein