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(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.