[libvirt] [PATCHv2] netcf driver: use a single netcf handle for all connections
Doug Goldstein
cardoe at gentoo.org
Wed Sep 11 15:37:18 UTC 2013
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;
> +
> +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
More information about the libvir-list
mailing list