[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



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

[...]

Attachment: signature.asc
Description: Digital signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]