[libvirt] [PATCH 2/2] netcf driver: use a single netcf handle for all connections

[Eric Blake]

On 08/28/2013 11:39 AM, Laine Stump 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 open a netcf instance, then all
> connections will 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 - most of the functionality from
> netcfInterfaceOpen() was moved to netcfStateInitialize (which
> initializes a single global driverState), and netcfInterfaceOpen now
> just puts a pointer to driverState into the privateData. A similar
> change was mad eto netcfStateCleanup() vs netcfInterfaceClose(). Since
> all the functions already have locking around them, no other change
> was necessary (they now use the single global lock rather than a lock
> specific to their connection).

I'm torn on whether this patch qualifies as being safe enough to apply
after freeze.

> ---
>  src/interface/interface_backend_netcf.c | 157 ++++++++++++++++++++++++--------
>  1 file changed, 117 insertions(+), 40 deletions(-)
> 
> diff --git a/src/interface/interface_backend_netcf.c b/src/interface/interface_backend_netcf.c
> index f47669e..1b5c850 100644
> --- a/src/interface/interface_backend_netcf.c
> +++ b/src/interface/interface_backend_netcf.c
> @@ -43,8 +43,10 @@ typedef struct
>  {
>      virMutex lock;
>      struct netcf *netcf;
> +    size_t nConnections;
>  } virNetcfDriverState, *virNetcfDriverStatePtr;
>  
> +virNetcfDriverStatePtr driverState = NULL;

Any reason this is not static?

>  
>  static void interfaceDriverLock(virNetcfDriverStatePtr driver)
>  {
> @@ -56,6 +58,98 @@ static void interfaceDriverUnlock(virNetcfDriverStatePtr driver)
>      virMutexUnlock(&driver->lock);
>  }
>  
> +static int
> +netcfStateInitialize(bool privileged ATTRIBUTE_UNUSED,
> +                     virStateInhibitCallback callback ATTRIBUTE_UNUSED,
> +                     void *opaque ATTRIBUTE_UNUSED)
> +{
> +    if (VIR_ALLOC(driverState) < 0)
> +        goto alloc_error;
> +
> +    /* initialize non-0 stuff in driverState */
> +    if (virMutexInit(&driverState->lock) < 0)
> +    {
> +        /* what error to report? */
> +        goto mutex_error;

VIR_ERR_INTERNAL_ERROR is probably as good as any, since this is unlikely.

> +    }
> +
> +    /* open netcf */
> +    if (ncf_init(&driverState->netcf, NULL) != 0)
> +    {
> +        /* what error to report? */
> +        goto netcf_error;

Probably here as well.

> -static virDrvOpenStatus netcfInterfaceOpen(virConnectPtr conn,
> -                                           virConnectAuthPtr auth ATTRIBUTE_UNUSED,
> -                                           unsigned int flags)
> +static virDrvOpenStatus
> +netcfInterfaceOpen(virConnectPtr conn,
> +                   virConnectAuthPtr auth ATTRIBUTE_UNUSED,
> +                   unsigned int flags)
>  {

>  
>      conn->interfacePrivateData = driverState;
> +    interfaceDriverLock(driverState);
> +    driverState->nConnections++;
> +    interfaceDriverUnlock(driverState);

Is it worth switching things to use virObject with atomic refcounting
that doesn't require a lock?  But that can probably be a separate patch.


-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 621 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20130828/31b8ced4/attachment-0001.sig>