virtperf: Revert PERF_COUNT_HW_REF_CPU_CYCLES conditional

[libvirt] [PATCH] virtperf: Revert PERF_COUNT_HW_REF_CPU_CYCLES conditional

Posted by Olaf Hering 7 years, 1 month ago

All PERF_* names are enmus. Enums are ordinary code for CPP.
An ifdef does not trigger.
Fixes 1d29c889a ("Make use of PERF_COUNT_HW_REF_CPU_CYCLES conditional")

Signed-off-by: Olaf Hering <olaf@aepfle.de>
---
 src/util/virperf.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/src/util/virperf.c b/src/util/virperf.c
index e39cebbe0..33039544a 100644
--- a/src/util/virperf.c
+++ b/src/util/virperf.c
@@ -108,13 +108,8 @@ static struct virPerfEventAttr attrs[] = {
      .attrType = PERF_TYPE_HARDWARE,
      .attrConfig = PERF_COUNT_HW_STALLED_CYCLES_BACKEND},
     {.type = VIR_PERF_EVENT_REF_CPU_CYCLES,
-# ifdef PERF_COUNT_HW_REF_CPU_CYCLES
      .attrType = PERF_TYPE_HARDWARE,
      .attrConfig = PERF_COUNT_HW_REF_CPU_CYCLES
-# else
-     .attrType = 0,
-     .attrConfig = 0,
-# endif
     },
     {.type = VIR_PERF_EVENT_CPU_CLOCK,
      .attrType = PERF_TYPE_SOFTWARE,

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

Re: [libvirt] [PATCH] virtperf: Revert PERF_COUNT_HW_REF_CPU_CYCLES conditional

Posted by Martin Kletzander 7 years, 1 month ago

On Tue, Mar 14, 2017 at 08:55:30AM +0000, Olaf Hering wrote:
>All PERF_* names are enmus. Enums are ordinary code for CPP.
>An ifdef does not trigger.
>Fixes 1d29c889a ("Make use of PERF_COUNT_HW_REF_CPU_CYCLES conditional")
>

Well, it *reverts* that commit.  If that is desired, it should rather be
reverted with git-revert(1).  However without that commit we'll be back
to broken code that won't compile on some distros.  So unless I missed
something (if I did, it should be added to the commit message), there
should rather be a configure check that defines something similar to
other code, e.g. HAVE_PERF_CPU_CYCLES and then that should be used in
the conditional.

Anyway it looks like the code is wrong anyway, does it currently error
out for you with "unable to open host cpu perf event ..." even though it
should work?

@Daniel: It seems like you probably wanted to relax the following
condition in virPerfEventEnable():

  if (event_attr->attrType == 0 && (type == VIR_PERF_EVENT_CMT ||
                                    type == VIR_PERF_EVENT_MBMT ||
                                    type == VIR_PERF_EVENT_MBML)) {

am I right?

>Signed-off-by: Olaf Hering <olaf@aepfle.de>
>---
> src/util/virperf.c | 5 -----
> 1 file changed, 5 deletions(-)
>
>diff --git a/src/util/virperf.c b/src/util/virperf.c
>index e39cebbe0..33039544a 100644
>--- a/src/util/virperf.c
>+++ b/src/util/virperf.c
>@@ -108,13 +108,8 @@ static struct virPerfEventAttr attrs[] = {
>      .attrType = PERF_TYPE_HARDWARE,
>      .attrConfig = PERF_COUNT_HW_STALLED_CYCLES_BACKEND},
>     {.type = VIR_PERF_EVENT_REF_CPU_CYCLES,
>-# ifdef PERF_COUNT_HW_REF_CPU_CYCLES
>      .attrType = PERF_TYPE_HARDWARE,
>      .attrConfig = PERF_COUNT_HW_REF_CPU_CYCLES
>-# else
>-     .attrType = 0,
>-     .attrConfig = 0,
>-# endif
>     },
>     {.type = VIR_PERF_EVENT_CPU_CLOCK,
>      .attrType = PERF_TYPE_SOFTWARE,
>
>--
>libvir-list mailing list
>libvir-list@redhat.com
>https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] virtperf: Revert PERF_COUNT_HW_REF_CPU_CYCLES conditional

Posted by Olaf Hering 7 years, 1 month ago

Am Tue, 14 Mar 2017 14:05:37 +0100
schrieb Martin Kletzander <mkletzan@redhat.com>:

> However without that commit we'll be back to broken code that won't compile on some distros.  So unless I missed something (if I did, it should be added to the commit message), there should rather be a configure check that defines something similar to other code, e.g. HAVE_PERF_CPU_CYCLES and then that should be used in the conditional.

There are other places in that and other files which can not be compiled. The obvious fix is to just add a #ifndef x;#define x 0 to get libvirt going. The question is still unanswered if configure should just error out and refuse to go further, or if the few missing pieces should be supplied manually because the affected functionality is optional.

See attached patches for how I deal with it.

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

Re: [libvirt] [PATCH] virtperf: Revert PERF_COUNT_HW_REF_CPU_CYCLES conditional

Posted by Martin Kletzander 7 years, 1 month ago

On Tue, Mar 14, 2017 at 02:18:48PM +0100, Olaf Hering wrote:
>Am Tue, 14 Mar 2017 14:05:37 +0100
>schrieb Martin Kletzander <mkletzan@redhat.com>:
>
>> However without that commit we'll be back to broken code that won't compile on some distros.  So unless I missed something (if I did, it should be added to the commit message), there should rather be a configure check that defines something similar to other code, e.g. HAVE_PERF_CPU_CYCLES and then that should be used in the conditional.
>
>There are other places in that and other files which can not be compiled. The obvious fix is to just add a #ifndef x;#define x 0 to get libvirt going. The question is still unanswered if configure should just error out and refuse to go further, or if the few missing pieces should be supplied manually because the affected functionality is optional.
>

if it is optional, it should not error out, but disable that part of the
code (unless it would change the behaviour).

>See attached patches for how I deal with it.
>
>Olaf

>--- a/src/util/virfile.c
>+++ b/src/util/virfile.c
>@@ -3623,6 +3623,9 @@ virFileBindMountDevice(const char *src,
>     return 0;
> }
>
>+#ifndef MS_MOVE
>+#define MS_MOVE              8192
>+#endif
>

We need this for qemu's mount namespace.  Functions in this file using
this could error out if MS_MOVE doesn't exist.  The caller can then just
decide to disable namespaces then (it can have different return value
for this kind of error).  I really don't like redefining constants from
somewhere else just to make the code compile.

> int
> virFileMoveMount(const char *src,
>--- a/src/util/virprocess.c
>+++ b/src/util/virprocess.c
>@@ -1156,6 +1156,13 @@ virProcessRunInMountNamespace(pid_t pid,
>
>
> #if defined(HAVE_SYS_MOUNT_H) && defined(HAVE_UNSHARE)
>+#ifndef MS_REC
>+# define MS_REC          16384
>+#endif
>+
>+#ifndef MS_SLAVE
>+# define MS_SLAVE                (1<<19)
>+#endif

Same here.  I know you that this way you can compile it on older kernel
and then upgrade and it will magically work, but that's not very nice
solution.

> int
> virProcessSetupPrivateMountNS(void)
> {

>--- a/src/util/virperf.c
>+++ b/src/util/virperf.c
>@@ -76,6 +76,12 @@ struct virPerfEventAttr {
>     unsigned long long attrConfig;
> };
>
>+#ifndef PERF_ATTR_SIZE_VER2
>+#define PERF_COUNT_HW_STALLED_CYCLES_FRONTEND   7
>+#define PERF_COUNT_HW_STALLED_CYCLES_BACKEND    8
>+#define PERF_COUNT_HW_REF_CPU_CYCLES            9
>+#endif
>+

These, IMHO, should be set to 0s and the code should be fixed to handle
0s as unsupported (as hinted in my previous mail).

> static struct virPerfEventAttr attrs[] = {
>     {.type = VIR_PERF_EVENT_CMT, .attrType = 0, .attrConfig = 1},
>     {.type = VIR_PERF_EVENT_MBMT, .attrType = 0, .attrConfig = 2},
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list