[libvirt] [PATCH 16/21] resctrl: Add functions to work with resctrl allocations
Martin Kletzander
mkletzan at redhat.com
Wed Nov 15 22:01:35 UTC 2017
On Wed, Nov 15, 2017 at 01:17:56PM -0500, John Ferlan wrote:
>
>[...]
>
>>>
>>> Two blank lines (multiple instances in new functions)
>>>
>>
>> I tried keeping this one more organized, two lines between groups of
>> functions, one line between functions in the same group. But I can do
>> two everywhere, the fact that I don't fully agree is irrelevant
>> (unfortunately), but I'd rather get this in instead of arguing over
>> the amount of whitespace =D
>>
>
>I guess I'm just going by other reviews I've done and received where the
>2 lines was requested for anything "new"... Not everyone follows it and
>I'm sure I've missed a few along the way.
>
I may miss some when amending the commits as well, even though I'm going line by
line and adjusting it as I go. I'll just put 2 everywhere.
>>>> +static int virResctrlAllocOnceInit(void)
>>>
>>> static int
>>> virResctrlAllocOnceInit(void)
>>>
>>
>> Oh, missed this one.
>>
>>>> +{
>>>> + if (!(virResctrlAllocClass = virClassNew(virClassForObject(),
>>>> + "virResctrlAlloc",
>>>> + sizeof(virResctrlAlloc),
>>>> + virResctrlAllocDispose)))
>>>> + return -1;
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> +VIR_ONCE_GLOBAL_INIT(virResctrlAlloc)
>>>> +
>>>> +static virResctrlAllocPtr
>>>> +virResctrlAllocNew(void)
>>>> +{
>>>> + if (virResctrlAllocInitialize() < 0)
>>>> + return NULL;
>>>> +
>>>> + return virObjectNew(virResctrlAllocClass);
>>>> +}
>>>> +
>>>> +
>>>> +/* Common functions */
>>>> +static int
>>>> +virResctrlLockInternal(int op)
>>>> +{
>>>> + int fd = open(SYSFS_RESCTRL_PATH, O_DIRECTORY | O_CLOEXEC);
>>>> +
>>>> + if (fd < 0) {
>>>> + virReportSystemError(errno, "%s", _("Cannot open resctrlfs"));
>>>> + return -1;
>>>> + }
>>>> +
>>>> + if (flock(fd, op) < 0) {
>>>
>>> So only ever used on a local file system right? Linux file locking
>>> functions are confounding...
>>>
>>
>> Yes, only local filesystem.
>>
>>> Why not use virFile{Lock|Unlock}?
>>>
>>
>> Because that uses fcnlt(2) which is different lock which might not
>> interactl with flock(2), so all programs working with resctrlfs must use
>> the same type of lock. And resctrlfs should specifically use flock(2)
>> according to the docs:
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/x86/intel_rdt_ui.txt
>>
>
>
>[...]
>
>>
>>>> + if (fd == -1)
>>>> + return 0;
>>>> +
>>>> + /* The lock gets unlocked by closing the fd, which we need to do
>>>> anyway in
>>>> + * order to clean up properly */
>>>> + if (VIR_CLOSE(fd) < 0) {
>>>> + virReportSystemError(errno, "%s", _("Cannot close resctrlfs"));
>>>> +
>>>> + /* Trying to save the already broken */
>>>
>>> So if close unlocks too, then why the unlock?
>>>
>>
>> Only if the close failed, I figured we might as well try to safe the
>> already broken, right? I can remove it if you want.
>>
>
>oh right - no, it's fine here. "Eye" think I missed the on failure part!
>
>>>> + if (flock(fd, LOCK_UN) < 0)
>>>> + virReportSystemError(errno, "%s", _("Cannot unlock
>>>> resctrlfs"));
>>>> + return -1;
>>>> + }
>>>> +
>>>> + return ret;
>>>> +}
>>>> +
>>>> +
>
>[...]
>
>>>> +/* This checks if the directory for the alloc exists. If not it
>>>> tries to create
>>>> + * it and apply appropriate alloc settings. */
>>>> +int
>>>> +virResctrlAllocCreate(virResctrlInfoPtr r_info,
>>>> + virResctrlAllocPtr alloc,
>>>> + const char *drivername,
>>>> + const char *machinename)
>>>> +{
>>>> + char *alloc_path = NULL;
>>>> + char *schemata_path = NULL;
>>>> + bool dir_created = false;
>>>> + char *alloc_str = NULL;
>>>> + int ret = -1;
>>>> + int lockfd = -1;
>>>> +
>>>> + if (!alloc)
>>>> + return 0;
>>>> +
>>>> + if (!alloc->path &&
>>>> + virAsprintf(&alloc->path, "%s/%s-%s-%s",
>>>> + SYSFS_RESCTRL_PATH, drivername, machinename,
>>>> alloc->id) < 0)
>>>
>>> This is being created in /sys/fs... and theoretically nothing will
>>> change for @drivername and @machinename
>>>
>>>> + return -1;
>>>> +
>>>> + /* Check if this allocation was already created */
>>>> + if (virFileIsDir(alloc->path)) {
>>>> + VIR_FREE(alloc_path);
>>>
>>> dead code ;-) "alloc_path" is never allocated...
>>>
>>
>> s/alloc_path/alloc->path/ =)
>>
>>> Any concern over the guest being killed without running through
>>> virResctrlAllocRemove and the rmdir?
>>>
>>
>> Yes, where do you see that? The remove will be done in
>> qemuProcessStop(). I can remove this one puny directory here, but
>> proper cleanup needs to be done for all anyway.
>>
>
>No where in particular - there's so much code to wade through and
>attempt to remember. I do see the call for Stop - just trying to think
>if there was some other way for guest death that could cause problems.
>
in qemu the ProcessStop is called always precisely for the reason so that we
have one place to clean up stuff.
>The only other thought I had along the lines here was writing into
>/sys/fs - not just the privilege but the size/number of files being
I should check that the daemon is running as privileged.
>created in /sys, but perhaps other things distracted those (beer?)
>thoughts... I was going to ask if output such as this should be
>something like ->libDir which could be deleted "for free"...
>
It's not a normal filesystem, it behaves similarly to cgroups and it's theonly
kernel interface for this. I cannot create the files where I please.
>If not should the rmdir(alloc->path) be a virDeleteTree(alloc->path)
>since you're creating "schemata" and "tasks"
>
I'm not, when you create the directory the files are "just there", like cgroup
dirs. You cannot virFileDeleteTree because you'll get error on removing any
file.
>John
>
>[...]
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20171115/092a1d7e/attachment-0001.sig>
More information about the libvir-list
mailing list