> -----Original Message-----
> From: John Ferlan [mailto:jfer...@redhat.com]
> Sent: Saturday, September 8, 2018 1:41 AM
> To: Wang, Huaqiang <huaqiang.w...@intel.com>; libvir-list@redhat.com
> Cc: Feng, Shaohe <shaohe.f...@intel.com>; Niu, Bing <bing....@intel.com>;
> Ding, Jian-feng <jian-feng.d...@intel.com>; Zang, Rui <rui.z...@intel.com>
> Subject: Re: [libvirt] [PATCH 05/10] util: resctrl: refactoring some functions
>
>
>
> On 09/07/2018 06:52 AM, Wang, Huaqiang wrote:
> >
> >
>
> [...]
>
> >>> +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.
> >
>
> I don't really care... I also don't follow the kernel naming rules.
>
Let's use the name virResctrlCreateGroupPath.
> >>> +{
> >>> + 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.
> >
>
> Yes and I know why it was called, but the rest of the text explained why I
> thought with overkill thrown in for good measure.
>
> >> 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?
> >
>
>
> In the long run it's perhaps a "cheap call", but using a "static int"
> means you can control who calls it and whether they have checked
> virResctrlInfoIsEmpty prior to calling thus this can assume it has.
> Having multiple functions calling IsEmpty is overkill.
>
Thanks for clarification. I'll check code and make sure virResctrlInfoIsEmpty
is not necessarily called in next version patches.
Thanks for review.
Huaqinag
> John
>
> [...]
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
