Re: [libvirt] [v7 00/10] Support cache tune in libvirt

On Mon, Feb 20, 2017 at 04:32:19PM +0800, Eli Qiao wrote: > > > -- > Best regards > Eli > > 天涯无处不重逢 > a leaf duckweed belongs to the sea, where not to meet in life > > Sent with Sparrow (http://www.sparrowmailapp.com/?sig) > > > On Sunday, 19 February 2017 at 10:51 PM, Marcelo Tosatti wrote: > > > > > Hi Eli Qiao, > > > > Question about removing resctrlfs directories with empty "tasks" file. > > > > > > /* This domain is not created by libvirt, so we don't care > > * about the tasks, add a fake one to prevent > > * virResCtrlRefresh > > * remove it from sysfs */ > > virResCtrlAddTask(p, 1); > > virResCtrlAppendDomain(p); > > > > /* Refresh all domains', remove the domains which has no task ids. > > * This will be used after VM pause, restart, destroy etc. > > */ > > static int > > virResCtrlRefresh(void) > > > > Why are you doing this (removing directories which have no tasks in > > them) ? The code should only read information from other > > CAT reservations, not write to them. > > > > When a VM get shutdown, their pids will disappear from resctrl’s directory, we can check pids to see if VM get KIlled or not. > > The reason why I would like check if tasks file empty is that: there maybe cases that libvirtd not tracking VM’s status, and the VM gone, left a empty directory with no tasks assigned, and cause resource leak. > > I think I can refine this later, keep a pointer of ResDomain in VM’s private data and use that to destroy VM’s resctrl directory. > > but this won’t work for exception, still resource leak happened. Function qemuProcessStop() will *always* be called to cleanup after VMs, all exceptions should be taken into account, so don't worry, just do proper cleanup.

> > This is not for cleanup purposes, since on VM shutdown the resctrlfs > > directories are properly removed: > > > > /* Remove the Domain from sysfs, this should only success no pids in > > * tasks > > * of a partition. > > */ > > static > > int virResCtrlRemoveDomain(const char *name) > > { > > char *path = NULL; > > int rc = 0; > > > > if ((rc = virAsprintf(&path, "%s/%s", RESCTRL_DIR, name)) < 0) > > return rc; > > rc = rmdir(path); > > VIR_FREE(path); > > return rc; > > } > > > > Should not write to other directories 'tasks' file. > > Other Apps can have a lock to resctrl before it write tasks to prevent libvirtd remove empty task directory.