[libvirt] [PATCH 16/21] resctrl: Add functions to work with resctrl allocations

[John Ferlan]

[...]

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

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

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

If not should the rmdir(alloc->path) be a virDeleteTree(alloc->path)
since you're creating "schemata" and "tasks"

John

[...]