[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



On Wed, Sep 11, 2013 at 10:06 AM, Laine Stump <laine 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;
> +
> +static void
> +virNetcfDriverStateDispose(void *obj)
> +{
> +    virNetcfDriverStatePtr driver = obj;
> +
> +    if (driver->netcf)
> +        ncf_close(driver->netcf);
> +}
> +
> +static void
> +interfaceDriverLock(virNetcfDriverStatePtr driver)
> +{
> +    virObjectLock(driver);
> +}
> +
> +static void
> +interfaceDriverUnlock(virNetcfDriverStatePtr driver)
> +{
> +    virObjectUnlock(driver);
> +}
> +
> +static int
> +netcfStateInitialize(bool privileged ATTRIBUTE_UNUSED,
> +                     virStateInhibitCallback callback ATTRIBUTE_UNUSED,
> +                     void *opaque ATTRIBUTE_UNUSED)
> +{
> +    if (virNetcfDriverStateInitialize() < 0)
> +        return -1;
> +
> +    if (!(driverState = virObjectLockableNew(virNetcfDriverStateClass)))
> +        return -1;
> +
> +    /* open netcf */
> +    if (ncf_init(&driverState->netcf, NULL) != 0) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                       _("failed to initialize netcf"));
> +        virObjectUnref(driverState);
> +        driverState = NULL;
> +        return -1;
> +    }
> +    return 0;
> +}
> +
> +static int
> +netcfStateCleanup(void)
>  {
> -    virMutexLock(&driver->lock);
> +    if (!driverState) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                       _("Attempt to close netcf state driver already closed"));
> +        return -1;
> +    }
> +
> +    if (virObjectUnref(driverState)) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                       _("Attempt to close netcf state driver "
> +                         "with open connections"));
> +        return -1;
> +    }
> +    driverState = NULL;
> +    return 0;
>  }
>
> -static void interfaceDriverUnlock(virNetcfDriverStatePtr driver)
> +static int
> +netcfStateReload(void)
>  {
> -    virMutexUnlock(&driver->lock);
> +    int ret = -1;
> +
> +    if (!driverState)
> +        return 0;
> +
> +    interfaceDriverLock(driverState);
> +    ncf_close(driverState->netcf);
> +    if (ncf_init(&driverState->netcf, NULL) != 0)
> +    {
> +        /* this isn't a good situation, because we can't shut down the
> +         * driver as there may still be connections to it. If we set
> +         * the netcf handle to NULL, any subsequent calls to netcf
> +         * will just fail rather than causing a crash. Not ideal, but
> +         * livable (since this should never happen).
> +         */
> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                       _("failed to re-init netcf"));
> +        driverState->netcf = NULL;
> +        goto cleanup;
> +    }
> +
> +    ret = 0;
> +cleanup:
> +    interfaceDriverUnlock(driverState);
> +
> +    return ret;
>  }
>
>  /*
> @@ -148,61 +248,30 @@ static struct netcf_if *interfaceDriverGetNetcfIF(struct netcf *ncf, virInterfac
>      return iface;
>  }
>
> -static virDrvOpenStatus netcfInterfaceOpen(virConnectPtr conn,
> -                                           virConnectAuthPtr auth ATTRIBUTE_UNUSED,
> -                                           unsigned int flags)
> +static virDrvOpenStatus
> +netcfInterfaceOpen(virConnectPtr conn,
> +                   virConnectAuthPtr auth ATTRIBUTE_UNUSED,
> +                   unsigned int flags)
>  {
> -    virNetcfDriverStatePtr driverState;
> -
>      virCheckFlags(VIR_CONNECT_RO, VIR_DRV_OPEN_ERROR);
>
> -    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;
> -    }
> -
> -    /* open netcf */
> -    if (ncf_init(&driverState->netcf, NULL) != 0)
> -    {
> -        /* what error to report? */
> -        goto netcf_error;
> -    }
> +    if (!driverState)
> +        return VIR_DRV_OPEN_ERROR;
>
> +    virObjectRef(driverState);
>      conn->interfacePrivateData = driverState;
>      return VIR_DRV_OPEN_SUCCESS;
> -
> -netcf_error:
> -    if (driverState->netcf)
> -    {
> -        ncf_close(driverState->netcf);
> -    }
> -    virMutexDestroy(&driverState->lock);
> -mutex_error:
> -    VIR_FREE(driverState);
> -alloc_error:
> -    return VIR_DRV_OPEN_ERROR;
>  }
>
> -static int netcfInterfaceClose(virConnectPtr conn)
> +static int
> +netcfInterfaceClose(virConnectPtr conn)
>  {
>
>      if (conn->interfacePrivateData != NULL)
>      {
> -        virNetcfDriverStatePtr driver = conn->interfacePrivateData;
> -
> -        /* close netcf instance */
> -        ncf_close(driver->netcf);
> -        /* destroy lock */
> -        virMutexDestroy(&driver->lock);
> -        /* free driver state */
> -        VIR_FREE(driver);
> +        virObjectUnref(conn->interfacePrivateData);
> +        conn->interfacePrivateData = NULL;
>      }
> -    conn->interfacePrivateData = NULL;
>      return 0;
>  }
>
> @@ -1070,7 +1139,7 @@ static int netcfInterfaceChangeRollback(virConnectPtr conn, unsigned int flags)
>  #endif /* HAVE_NETCF_TRANSACTIONS */
>
>  static virInterfaceDriver interfaceDriver = {
> -    "netcf",
> +    .name = INTERFACE_DRIVER_NAME,
>      .interfaceOpen = netcfInterfaceOpen, /* 0.7.0 */
>      .interfaceClose = netcfInterfaceClose, /* 0.7.0 */
>      .connectNumOfInterfaces = netcfConnectNumOfInterfaces, /* 0.7.0 */
> @@ -1093,11 +1162,19 @@ static virInterfaceDriver interfaceDriver = {
>  #endif /* HAVE_NETCF_TRANSACTIONS */
>  };
>
> +static virStateDriver interfaceStateDriver = {
> +    .name = INTERFACE_DRIVER_NAME,
> +    .stateInitialize = netcfStateInitialize,
> +    .stateCleanup = netcfStateCleanup,
> +    .stateReload = netcfStateReload,
> +};
> +
>  int netcfIfaceRegister(void) {
>      if (virRegisterInterfaceDriver(&interfaceDriver) < 0) {
>          virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>                         _("failed to register netcf interface driver"));
>          return -1;
>      }
> +    virRegisterStateDriver(&interfaceStateDriver);
>      return 0;
>  }
> --
> 1.7.11.7

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.

-- 
Doug Goldstein


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]