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

On 11.09.2013 17:37, Doug Goldstein wrote: > On Wed, Sep 11, 2013 at 10:06 AM, Laine Stump <laine(a)laine.org&gt; 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; > > 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. > The VIR_ONCE_GLOBAL_INIT(virDummy) macro creates a function called virDummyInitialize() which when called for the 1st time calls virDummyOnceInit(). Any subsequent call to virDummyInitialize() results in immediate return. Michal