> -----Original Message----- > From: John Ferlan [mailto:jferlan@xxxxxxxxxx] > Sent: Wednesday, September 5, 2018 10:49 PM > To: Wang, Huaqiang <huaqiang.wang@xxxxxxxxx>; libvir-list@xxxxxxxxxx > Cc: Feng, Shaohe <shaohe.feng@xxxxxxxxx>; Niu, Bing <bing.niu@xxxxxxxxx>; > Ding, Jian-feng <jian-feng.ding@xxxxxxxxx>; Zang, Rui <rui.zang@xxxxxxxxx> > Subject: Re: [PATCH 05/10] util: resctrl: refactoring some functions > > > > On 08/27/2018 07:23 AM, Wang Huaqiang wrote: > > Some code, in virresctrl.c, manupulating the file objects of resctrlfs > > could be reused for cache monitor interfaces. This patch refactor > > these functions for purpose of reusing code in later patch: > > > > virResctrlAllocDeterminePath > > virResctrlAllocCreate > > virResctrlAddPID > > > > Signed-off-by: Wang Huaqiang <huaqiang.wang@xxxxxxxxx> > > --- > > src/util/virresctrl.c | 126 > > +++++++++++++++++++++++++++++++++++++------------- > > 1 file changed, 93 insertions(+), 33 deletions(-) > > > > Yikes, 3 or more patches in one. > Will be split according to your suggestions. > > diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c index > > 2f6923a..b3bae6e 100644 > > --- a/src/util/virresctrl.c > > +++ b/src/util/virresctrl.c > > @@ -2082,25 +2082,94 @@ virResctrlAllocAssign(virResctrlInfoPtr > > resctrl, } > > > > > > -int > > -virResctrlAllocDeterminePath(virResctrlAllocPtr alloc, > > - const char *machinename) > > +static int > > +virResctrlDeterminePath(const char *id, > > + const char *root, > > Let's use @rootpath instead of @root > Will be fixed.. > > + const char *parentpath, > > + const char *prefix, > > + char **path) > > Take it slowly - round 1, convert virResctrlAllocDeterminePath into using > virResctrlDeterminePath, but don't add the @parentpath yet since it's not > "introduced" until later. > OK. Split into two steps/patches. > I'd prefer to see the same argument order as being printed too... > OK. Will pay attention to the order. > > { > > - if (!alloc->id) { > > + if (!id) { > > virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > > - _("Resctrl Allocation ID must be set before creation")); > > + _("Resctrl resource ID must be set before > > + creation")); > > return -1; > > } > > > > - if (!alloc->path && > > - virAsprintf(&alloc->path, "%s/%s-%s", > > - SYSFS_RESCTRL_PATH, machinename, alloc->id) < 0) > > + if (*path) { > > + virReportError(VIR_ERR_INTERNAL_ERROR, > > + _("Resctrl group (%s) already created, path=%s."), > > + id, *path); > > The indent is off here w/ @id - need another space. Still is this a programming > error or something else? Tough to tell since you're adding multiple things at one > time. Yes. Will add a space before @id. Will follow your suggestion mention below, this function will take @path as a return Value. No parameter of @path then, and no error message. > > > return -1; > > + } > > + > > + if (!parentpath && !root) { > > + if (virAsprintf(path, "%s/%s-%s", > > + SYSFS_RESCTRL_PATH, prefix, id) < 0) > > + return -1; > > and this is just the initial case... > > > + } else if (!parentpath) { > > + if (virAsprintf(path, "%s/%s/%s-%s", > > + SYSFS_RESCTRL_PATH, parentpath, prefix, id) < 0) > > + return -1; > > + } else { > > + if (virAsprintf(path, "%s/%s/%s-%s", > > + root, parentpath, prefix, id) < 0) > > + return -1; > > + } > > These are additional cases added later on, but used in this patch, so they need to > "wait" to be added until we see "where" they come from. Will be fixed. > > > > > return 0; > > Seems to me rather than passing &alloc->path, this function could return @path > and the caller then be able to "handle" that. OK. Follow the suggestion, take @path as a return value. > > For the "first pass" before @root and @parentpath are added, using: > > ignore_value(virAsprintf(&path, "%s/%s-%s", > rootpath, prefix, id)); > > return path; > OK. Take your suggestion and will make change. > > } > > > > > > +int > > +virResctrlAllocDeterminePath(virResctrlAllocPtr alloc, > > + const char *machinename) { > > + return virResctrlDeterminePath(alloc->id, NULL, NULL, > > + machinename, &alloc->path); > > Thus this becomes: > > if (!(alloc->path = virResctrlDeterminePath(SYSFS_RESCTRL_PATH, > machinename, > alloc->id))) > return -1; > > return 0; > Understand. Will be followed. > > +} > > + > > should be two blank lines between and this could use a comment describing > what it's doing and what it's assumptions are. > Two blank lines here for coding style consistence, ok. And add following comments to describe the functionality. /* * This helper creates the resctrl group by making the real directory in * resctrl file system. @path is the directory path. */ > > +static int > > +virResctrlCreateGroup(virResctrlInfoPtr resctrl, > > + char *path) > > s/char/const char/ > Will be fixed. > should be: > > virResctrlCreateGroupPath > I prefer the original name ' virResctrlCreateGroup' than 'virResctrlCreateGroupPath'. The main role of this function is to make a directory, and the directory is called 'resource group' in kernel's document. See document https://www.kernel.org/doc/Documentation/x86/intel_rdt_ui.txt If you still think 'virResctrlCreateGroupPath' is better, OK, let's change it. > > +{ > > + int ret = -1; > > + int lockfd = -1; > > + > > + if (!path) > > + return -1; > > This would cause some sort of unknown error, but it's a caller bug isn't it? That > is if @path is empty before calling in here, then we've missed some other > condition, so in this instance it doesn't quite make sense. > OK. I need to pay more attention on these places that could cause 'unknown error'. > > + > > + if (virResctrlInfoIsEmpty(resctrl)) { > > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", > > + _("Resource control is not supported on this host")); > > + return -1; > > + } > > Not quite sure what this has to do with creating the GroupPath. Does 'this' mean the invoking of ' virResctrlInfoIsEmpty'? virResctrlInfoIsEmpty return true ensures that the 'resctrl fs' is supported here. >Feels like some > that should be in the caller, but I guess that depends on future usage.... I see this > helper is called in the next patch by virResctrlAllocCreateMonitor which isn't > used until patch9 and only called once/if virResctrlAllocCreate is successful. > Awesome, your feeling is right. My design is, virResctrlAllocCreate creates an resource 'allocation', and virResctrlAllocCreateMonitor creates a resource 'monitor'. The 'monitor' belongs to some specific 'allocation'. If you want to create a 'monitor', an 'allocation' must be created already, and link the 'monitor' to the 'allocation'. So when virResctrlAllocCreateMonitor is called, the virResctrlAllocCreate must be called successfully. And the ' virResctrlInfoIsEmpty' is checked more than one times. Will move the call of virResctrlInfoIsEmpty into virResctrlAllocCreate. > So it doesn't seem that calling it once for each time > virResctrlAllocCreateMonitor is called is really necessary since @resctrl doesn't > change. > > In fact, going back to qemuProcessResctrlCreate it would seem that calling > virResctrlAllocCreate once for each vm->def->nresctrls would also be somewhat > inefficient since caps->host.resctrl (a/k/a @resctrl) doesn't change. But moving > it back there may mean needing to check if > vm->def->resctrls[i]->alloc is NULL... > > I think perhaps some more thought needs to be placed on "efficient" code paths > before adding the monitor code paths. Confused. Do you still talking about the mult-call over function virResctrlInfoIsEmpty? > > > + > > + if (STREQ(path, SYSFS_RESCTRL_PATH)) > > + return 0; > > This concept doesn't appear until the next patch, so we cannot introduce it yet. > OK. Will split the patch and make change accordingly. > > + > > + if (virFileExists(path)) { > > + virReportError(VIR_ERR_INTERNAL_ERROR, > > + _("Path '%s' for resctrl resource group exists"), path); > > + goto cleanup; > > + } > > + > > + lockfd = virResctrlLockWrite(); > > + if (lockfd < 0) > > + goto cleanup; > > This Lock/Unlock sequence should be in the caller... and the fact that the lock > should be taken documented as "expected" in the caller. > Will remove locker here and put locker in caller. > > + > > + if (virFileMakePath(path) < 0) { > > + virReportSystemError(errno, > > + _("Cannot create resctrl directory '%s'"), path); > > + goto cleanup; > > + } > > + > > + ret = 0; > > + cleanup: > > + virResctrlUnlock(lockfd); > > + return ret; > > In the short term, @ret probably isn't needed - return 0 or -1 directly. > Will be fixed. > > +} > > + > > + > > /* This checks if the directory for the alloc exists. If not it tries to create > > * it and apply appropriate alloc settings. */ int @@ -2116,21 > > +2185,11 @@ virResctrlAllocCreate(virResctrlInfoPtr resctrl, > > if (!alloc) > > return 0; > > > > - if (virResctrlInfoIsEmpty(resctrl)) { > > - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", > > - _("Resource control is not supported on this host")); > > - return -1; > > - } > > - > > if (virResctrlAllocDeterminePath(alloc, machinename) < 0) > > return -1; > > If we return from this and alloc->path == NULL, there's a coding error, so I see > no reason in virResctrlCreateGroupPath that we'd need to validate that (at least > yet). It's a static helper and should be called only when your expected conditions > are right. > Got. Will pay attention to places that will generate 'unkown error's. > > > > - if (virFileExists(alloc->path)) { > > - virReportError(VIR_ERR_INTERNAL_ERROR, > > - _("Path '%s' for resctrl allocation exists"), > > - alloc->path); > > - goto cleanup; > > - } > > + if (virResctrlCreateGroup(resctrl, alloc->path) < 0) > > + return -1; > > > > lockfd = virResctrlLockWrite(); > > if (lockfd < 0) > > The call to virResctrlCreateGroupPath should come after this rather than > Lock/Unlock when creating the directory and then Lock/Unlock again when > writing to the file. I think it's all one autonomous operation. OK. > > > @@ -2146,13 +2205,6 @@ virResctrlAllocCreate(virResctrlInfoPtr resctrl, > > if (virAsprintf(&schemata_path, "%s/schemata", alloc->path) < 0) > > goto cleanup; > > > > - if (virFileMakePath(alloc->path) < 0) { > > - virReportSystemError(errno, > > - _("Cannot create resctrl directory '%s'"), > > - alloc->path); > > - goto cleanup; > > - } > > - > > VIR_DEBUG("Writing resctrl schemata '%s' into '%s'", alloc_str, > schemata_path); > > if (virFileWriteStr(schemata_path, alloc_str, 0) < 0) { > > rmdir(alloc->path); > > @@ -2171,21 +2223,21 @@ virResctrlAllocCreate(virResctrlInfoPtr > > resctrl, } > > > > > > The next hunk is fine as long as it is a single patch. > > John > Thanks for review. Huaqiang > > -int > > -virResctrlAllocAddPID(virResctrlAllocPtr alloc, > > - pid_t pid) > > +static int > > +virResctrlAddPID(char *path, > > + pid_t pid) > > { > > char *tasks = NULL; > > char *pidstr = NULL; > > int ret = 0; > > > > - if (!alloc->path) { > > + if (!path) { > > virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > > - _("Cannot add pid to non-existing resctrl allocation")); > > + _("Cannot add pid to non-existing resctrl > > + group")); > > return -1; > > } > > > > - if (virAsprintf(&tasks, "%s/tasks", alloc->path) < 0) > > + if (virAsprintf(&tasks, "%s/tasks", path) < 0) > > return -1; > > > > if (virAsprintf(&pidstr, "%lld", (long long int) pid) < 0) @@ > > -2207,6 +2259,14 @@ virResctrlAllocAddPID(virResctrlAllocPtr alloc, > > > > > > int > > +virResctrlAllocAddPID(virResctrlAllocPtr alloc, > > + pid_t pid) > > +{ > > + return virResctrlAddPID(alloc->path, pid); } > > + > > + > > +int > > virResctrlAllocRemove(virResctrlAllocPtr alloc) { > > int ret = 0; > > -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
