Re: [PATCHv2 1/4] util: Introduce monitor capability interface

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

 



Hi Jano,

Recognized there was already a commit for a  fixing: 7bff646d71aa90ed8727ef99be29d6d2ab5d8f06.
And now I got your idea.

Thanks
Huaqiang

> -----Original Message-----
> From: Wang, Huaqiang
> Sent: Tuesday, October 9, 2018 5:55 PM
> To: 'Ján Tomko' <jtomko@xxxxxxxxxx>; John Ferlan <jferlan@xxxxxxxxxx>
> Cc: libvir-list@xxxxxxxxxx; Feng, Shaohe <shaohe.feng@xxxxxxxxx>; Niu, Bing
> <bing.niu@xxxxxxxxx>; Ding, Jian-feng <jian-feng.ding@xxxxxxxxx>; Zang, Rui
> <rui.zang@xxxxxxxxx>
> Subject: RE:  [PATCHv2 1/4] util: Introduce monitor capability interface
> 
> 
> 
> > -----Original Message-----
> > From: Ján Tomko [mailto:jtomko@xxxxxxxxxx]
> > Sent: Friday, October 5, 2018 10:42 PM
> > To: John Ferlan <jferlan@xxxxxxxxxx>
> > Cc: Wang, Huaqiang <huaqiang.wang@xxxxxxxxx>; libvir-list@xxxxxxxxxx;
> > Feng, Shaohe <shaohe.feng@xxxxxxxxx>; Niu, Bing <bing.niu@xxxxxxxxx>;
> > Ding, Jian- feng <jian-feng.ding@xxxxxxxxx>; Zang, Rui
> > <rui.zang@xxxxxxxxx>
> > Subject: Re:  [PATCHv2 1/4] util: Introduce monitor
> > capability interface
> >
> > On Tue, Sep 18, 2018 at 03:38:44PM -0400, John Ferlan wrote:
> > >On 09/14/2018 09:30 PM, Wang Huaqiang wrote:
> > >> This patch introduces the resource monitor and creates the
> > >> interface for getting host capability of resource monitor from the
> > >> system resource control file system.
> > >>
> > >> The resource monitor take the role of RDT monitoring group, could
> > >> be
> > >
> > >*takes...
> > >
> > >s/, could/ and could/
> > >
> > >> used to monitor the resource consumption information, such as the
> > >> last level cache occupancy and the utilization of memory bandwidth.
> > >>
> > >> Signed-off-by: Wang Huaqiang <huaqiang.wang@xxxxxxxxx>
> > >> ---
> > >>  src/util/virresctrl.c | 124
> > >> ++++++++++++++++++++++++++++++++++++++++++++++++++
> > >>  1 file changed, 124 insertions(+)
> > >>
> >
> > [...]
> >
> > >> +
> > >> +    rv = virFileReadValueUint(&info_monitor->max_monitor,
> > >> +                              SYSFS_RESCTRL_PATH "/info/L3_MON/num_rmids");
> > >> +    if (rv == -2) {
> > >> +        /* The file doesn't exist, so it's unusable for us, probably resource
> > >> +         * monitor unsupported */
> > >> +        VIR_INFO("The path '" SYSFS_RESCTRL_PATH
> > "/info/L3_MON/num_rmids' "
> > >> +                 "does not exist");
> > >
> > >Add virResetLastError()
> > >
> > >[avoids having this error in Last and something else failing and
> > >spewing the error]
> > >
> >
> > The return value of -2 means no error was set, so there is nothing to do here.
> 
> A return value of -2 means no error, rather than executing remaining part of
> function, it requires to return to the caller without reporting any error.
> 
> Here, a return value of -2 means "/info/L3_MON/num_rmids" is not exists, this
> could happen if CMT is not supported by host. This is a valid scenario and does
> not mean an error, and this function should not report any error to its caller, and
> the caller, which is virResctrlGetInfo, will continue to run its remaining
> statements normally.
> 
> >
> > Also, virResetLastError is meant to be used before starting an API.
> > It only resets the thread-local error object (which can only contain
> > one error), it cannot possibly unlog an error that was logged earlier.
> > In that case, creating a Quiet version of the function is the proper solution.
> 
> Do you mean the message reported by 'VIR_INFO' should be removed and also
> not adding the 'virResetLastError' line?
> 
> It might be more consistent if we keep the 'VIR_INFO' lines, because the similar
> message has been emitted in checking the memory bandwidth information and
> cache allocation information. But if you insist that this message should not be
> shown to user or developer, I could accept that and make change to it.
> 
> @@ -704,12 +704,7 @@ virResctrlGetMonitorInfo(virResctrlInfoPtr resctrl)
>      rv = virFileReadValueUint(&info_monitor->max_monitor,
>                                SYSFS_RESCTRL_PATH "/info/L3_MON/num_rmids");
>      if (rv == -2) {
>         /* The file doesn't exist, so it's unusable for us, probably resource
>          * monitor unsupported */
>         VIR_INFO("The file '" SYSFS_RESCTRL_PATH "/info/L3_MON/num_rmids' "
>                  "does not exist");
>          ret = 0;
> -        virResetLastError();
>          goto cleanup;
>      } else if (rv < 0) {
>          /* Other failures are fatal, so just quit */
> 
> >
> > Jano
> >
> > >> +        ret = 0;
> > >> +        goto cleanup;

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux