[libvirt] [PATCH 0/2] netcf: use only a single netcf instance for all connections
There is a good description in 2/2. I'll be out of reach for the next several days, so unable to push even if it gets ACKed before the freeze. I have done a rudimentary test and it seems to work, but if someone with a suitable setup wants to do a stress test with [many] connections, that would be very useful. Laine Stump (2): rename "struct interface_driver" to virNetcfDriverState netcf driver: use a single netcf handle for all connections src/interface/interface_backend_netcf.c | 209 ++++++++++++++++++++++---------- 1 file changed, 144 insertions(+), 65 deletions(-) -- 1.7.11.7
This better fits the modern naming scheme in libvirt, and anticipates an upcoming change where a single instance of this state will be maintained by a separate state driver, and every instance of the netcf driver will share the same state. --- src/interface/interface_backend_netcf.c | 52 +++++++++++++++++---------------- 1 file changed, 27 insertions(+), 25 deletions(-) diff --git a/src/interface/interface_backend_netcf.c b/src/interface/interface_backend_netcf.c index b92b0ce..f47669e 100644 --- a/src/interface/interface_backend_netcf.c +++ b/src/interface/interface_backend_netcf.c @@ -2,7 +2,7 @@ * interface_driver.c: backend driver methods to handle physical * interface configuration using the netcf library. * - * Copyright (C) 2006-2012 Red Hat, Inc. + * Copyright (C) 2006-2013 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -36,20 +36,22 @@ #define VIR_FROM_THIS VIR_FROM_INTERFACE +#define INTERFACE_DRIVER_NAME "netcf" + /* Main driver state */ -struct interface_driver +typedef struct { virMutex lock; struct netcf *netcf; -}; +} virNetcfDriverState, *virNetcfDriverStatePtr; -static void interfaceDriverLock(struct interface_driver *driver) +static void interfaceDriverLock(virNetcfDriverStatePtr driver) { virMutexLock(&driver->lock); } -static void interfaceDriverUnlock(struct interface_driver *driver) +static void interfaceDriverUnlock(virNetcfDriverStatePtr driver) { virMutexUnlock(&driver->lock); } @@ -150,7 +152,7 @@ static virDrvOpenStatus netcfInterfaceOpen(virConnectPtr conn, virConnectAuthPtr auth ATTRIBUTE_UNUSED, unsigned int flags) { - struct interface_driver *driverState; + virNetcfDriverStatePtr driverState; virCheckFlags(VIR_CONNECT_RO, VIR_DRV_OPEN_ERROR); @@ -191,7 +193,7 @@ static int netcfInterfaceClose(virConnectPtr conn) if (conn->interfacePrivateData != NULL) { - struct interface_driver *driver = conn->interfacePrivateData; + virNetcfDriverStatePtr driver = conn->interfacePrivateData; /* close netcf instance */ ncf_close(driver->netcf); @@ -208,7 +210,7 @@ static int netcfConnectNumOfInterfacesImpl(virConnectPtr conn, int status, virInterfaceObjListFilter filter) { - struct interface_driver *driver = conn->interfacePrivateData; + virNetcfDriverStatePtr driver = conn->interfacePrivateData; int count; int want = 0; int ret = -1; @@ -302,7 +304,7 @@ static int netcfConnectListInterfacesImpl(virConnectPtr conn, char **const names, int nnames, virInterfaceObjListFilter filter) { - struct interface_driver *driver = conn->interfacePrivateData; + virNetcfDriverStatePtr driver = conn->interfacePrivateData; int count = 0; int want = 0; int ret = -1; @@ -401,7 +403,7 @@ cleanup: static int netcfConnectNumOfInterfaces(virConnectPtr conn) { int count; - struct interface_driver *driver = conn->interfacePrivateData; + virNetcfDriverStatePtr driver = conn->interfacePrivateData; if (virConnectNumOfInterfacesEnsureACL(conn) < 0) return -1; @@ -416,7 +418,7 @@ static int netcfConnectNumOfInterfaces(virConnectPtr conn) static int netcfConnectListInterfaces(virConnectPtr conn, char **const names, int nnames) { - struct interface_driver *driver = conn->interfacePrivateData; + virNetcfDriverStatePtr driver = conn->interfacePrivateData; int count; if (virConnectListInterfacesEnsureACL(conn) < 0) @@ -435,7 +437,7 @@ static int netcfConnectListInterfaces(virConnectPtr conn, char **const names, in static int netcfConnectNumOfDefinedInterfaces(virConnectPtr conn) { int count; - struct interface_driver *driver = conn->interfacePrivateData; + virNetcfDriverStatePtr driver = conn->interfacePrivateData; if (virConnectNumOfDefinedInterfacesEnsureACL(conn) < 0) return -1; @@ -450,7 +452,7 @@ static int netcfConnectNumOfDefinedInterfaces(virConnectPtr conn) static int netcfConnectListDefinedInterfaces(virConnectPtr conn, char **const names, int nnames) { - struct interface_driver *driver = conn->interfacePrivateData; + virNetcfDriverStatePtr driver = conn->interfacePrivateData; int count; if (virConnectListDefinedInterfacesEnsureACL(conn) < 0) @@ -472,7 +474,7 @@ netcfConnectListAllInterfaces(virConnectPtr conn, virInterfacePtr **ifaces, unsigned int flags) { - struct interface_driver *driver = conn->interfacePrivateData; + virNetcfDriverStatePtr driver = conn->interfacePrivateData; int count; size_t i; struct netcf_if *iface = NULL; @@ -627,7 +629,7 @@ cleanup: static virInterfacePtr netcfInterfaceLookupByName(virConnectPtr conn, const char *name) { - struct interface_driver *driver = conn->interfacePrivateData; + virNetcfDriverStatePtr driver = conn->interfacePrivateData; struct netcf_if *iface; virInterfacePtr ret = NULL; virInterfaceDefPtr def = NULL; @@ -667,7 +669,7 @@ cleanup: static virInterfacePtr netcfInterfaceLookupByMACString(virConnectPtr conn, const char *macstr) { - struct interface_driver *driver = conn->interfacePrivateData; + virNetcfDriverStatePtr driver = conn->interfacePrivateData; struct netcf_if *iface; int niface; virInterfacePtr ret = NULL; @@ -716,7 +718,7 @@ cleanup: static char *netcfInterfaceGetXMLDesc(virInterfacePtr ifinfo, unsigned int flags) { - struct interface_driver *driver = ifinfo->conn->interfacePrivateData; + virNetcfDriverStatePtr driver = ifinfo->conn->interfacePrivateData; struct netcf_if *iface = NULL; char *xmlstr = NULL; virInterfaceDefPtr ifacedef = NULL; @@ -774,7 +776,7 @@ static virInterfacePtr netcfInterfaceDefineXML(virConnectPtr conn, const char *xml, unsigned int flags) { - struct interface_driver *driver = conn->interfacePrivateData; + virNetcfDriverStatePtr driver = conn->interfacePrivateData; struct netcf_if *iface = NULL; char *xmlstr = NULL; virInterfaceDefPtr ifacedef = NULL; @@ -821,7 +823,7 @@ cleanup: } static int netcfInterfaceUndefine(virInterfacePtr ifinfo) { - struct interface_driver *driver = ifinfo->conn->interfacePrivateData; + virNetcfDriverStatePtr driver = ifinfo->conn->interfacePrivateData; struct netcf_if *iface = NULL; virInterfaceDefPtr def = NULL; int ret = -1; @@ -862,7 +864,7 @@ cleanup: static int netcfInterfaceCreate(virInterfacePtr ifinfo, unsigned int flags) { - struct interface_driver *driver = ifinfo->conn->interfacePrivateData; + virNetcfDriverStatePtr driver = ifinfo->conn->interfacePrivateData; struct netcf_if *iface = NULL; virInterfaceDefPtr def = NULL; int ret = -1; @@ -905,7 +907,7 @@ cleanup: static int netcfInterfaceDestroy(virInterfacePtr ifinfo, unsigned int flags) { - struct interface_driver *driver = ifinfo->conn->interfacePrivateData; + virNetcfDriverStatePtr driver = ifinfo->conn->interfacePrivateData; struct netcf_if *iface = NULL; virInterfaceDefPtr def = NULL; int ret = -1; @@ -947,7 +949,7 @@ cleanup: static int netcfInterfaceIsActive(virInterfacePtr ifinfo) { - struct interface_driver *driver = ifinfo->conn->interfacePrivateData; + virNetcfDriverStatePtr driver = ifinfo->conn->interfacePrivateData; struct netcf_if *iface = NULL; unsigned int flags = 0; virInterfaceDefPtr def = NULL; @@ -990,7 +992,7 @@ cleanup: #ifdef HAVE_NETCF_TRANSACTIONS static int netcfInterfaceChangeBegin(virConnectPtr conn, unsigned int flags) { - struct interface_driver *driver = conn->interfacePrivateData; + virNetcfDriverStatePtr driver = conn->interfacePrivateData; int ret; virCheckFlags(0, -1); /* currently flags must be 0 */ @@ -1016,7 +1018,7 @@ static int netcfInterfaceChangeBegin(virConnectPtr conn, unsigned int flags) static int netcfInterfaceChangeCommit(virConnectPtr conn, unsigned int flags) { - struct interface_driver *driver = conn->interfacePrivateData; + virNetcfDriverStatePtr driver = conn->interfacePrivateData; int ret; virCheckFlags(0, -1); /* currently flags must be 0 */ @@ -1042,7 +1044,7 @@ static int netcfInterfaceChangeCommit(virConnectPtr conn, unsigned int flags) static int netcfInterfaceChangeRollback(virConnectPtr conn, unsigned int flags) { - struct interface_driver *driver = conn->interfacePrivateData; + virNetcfDriverStatePtr driver = conn->interfacePrivateData; int ret; virCheckFlags(0, -1); /* currently flags must be 0 */ -- 1.7.11.7
On 08/28/2013 11:39 AM, Laine Stump wrote:
This better fits the modern naming scheme in libvirt, and anticipates an upcoming change where a single instance of this state will be maintained by a separate state driver, and every instance of the netcf driver will share the same state. --- src/interface/interface_backend_netcf.c | 52 +++++++++++++++++---------------- 1 file changed, 27 insertions(+), 25 deletions(-)
This one is mechanical, and safe to apply even during freeze. ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
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). --- 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; 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; + } + + /* open netcf */ + if (ncf_init(&driverState->netcf, NULL) != 0) + { + /* what error to report? */ + goto netcf_error; + } + return 0; + +netcf_error: + if (driverState->netcf) + { + ncf_close(driverState->netcf); + } + virMutexDestroy(&driverState->lock); +mutex_error: + VIR_FREE(driverState); +alloc_error: + return -1; +} + +static int +netcfStateCleanup(void) +{ + if (!driverState) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Attempt to close netcf state driver already closed")); + return -1; + } + if (driverState->nConnections) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Attempt to close netcf state driver with %zu open connections"), + driverState->nConnections); + return -1; + } + + /* assume that we are single-thread here, so lock isn't needed */ + + /* close netcf instance */ + ncf_close(driverState->netcf); + /* destroy lock */ + virMutexDestroy(&driverState->lock); + /* free driver state */ + VIR_FREE(driverState); + return 0; +} + +static int +netcfStateReload(void) +{ + 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; +} + /* * Get a minimal virInterfaceDef containing enough metadata * for access control checks to be performed. Currently @@ -148,59 +242,34 @@ 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; conn->interfacePrivateData = driverState; + interfaceDriverLock(driverState); + driverState->nConnections++; + interfaceDriverUnlock(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); + interfaceDriverLock(driver); + driverState->nConnections--; + interfaceDriverUnlock(driver); } 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
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
participants (2)
-
Eric Blake -
Laine Stump