On Wed, Sep 11, 2013 at 10:06 AM, Laine Stump <laine@xxxxxxxxx> 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 -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list