* [PATCH v2 0/4] Add dtrace support on macOS
@ 2020-07-17 9:35 Roman Bolshakov
2020-07-17 9:35 ` [PATCH v2 1/4] scripts/tracetool: Fix dtrace generation for macOS Roman Bolshakov
` (5 more replies)
0 siblings, 6 replies; 21+ messages in thread
From: Roman Bolshakov @ 2020-07-17 9:35 UTC (permalink / raw)
To: qemu-devel
Cc: Roman Bolshakov, Daniel P. Berrangé,
Cameron Esfahani, Stefan Hajnoczi
Hi,
This is a small series that enables dtrace tracing backend on macOS.
Whether or not it should go to 5.1 is up to discretion of tracing
maintainers.
Thanks,
Roman
Changes since v1:
- Fixed a typo ANSI C to C99, wrt to _Bool in the first patch.
- Prevented a few [-Wpointer-sign] warnings by converting int8_t * to
signed char * in static probe definitions.
- Moved COLO packet dump under #ifdef DEBUG_COLO_PACKETS (Daniel).
- Separated tracepoints in net/filter-rewriter.c to use matching
is-enabled probe (Daniel).
Roman Bolshakov (4):
scripts/tracetool: Fix dtrace generation for macOS
scripts/tracetool: Use void pointer for vcpu
build: Don't make object files for dtrace on macOS
net/colo: Match is-enabled probe to tracepoint
Makefile.objs | 2 ++
net/colo-compare.c | 42 ++++++++++++++++++-----------------
net/filter-rewriter.c | 10 +++++++--
net/trace-events | 2 --
scripts/tracetool/format/d.py | 15 ++++++++++++-
scripts/tracetool/vcpu.py | 2 +-
6 files changed, 47 insertions(+), 26 deletions(-)
--
2.26.1
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v2 1/4] scripts/tracetool: Fix dtrace generation for macOS
2020-07-17 9:35 [PATCH v2 0/4] Add dtrace support on macOS Roman Bolshakov
@ 2020-07-17 9:35 ` Roman Bolshakov
2020-07-19 13:52 ` Philippe Mathieu-Daudé
2020-07-21 12:33 ` Stefan Hajnoczi
2020-07-17 9:35 ` [PATCH v2 2/4] scripts/tracetool: Use void pointer for vcpu Roman Bolshakov
` (4 subsequent siblings)
5 siblings, 2 replies; 21+ messages in thread
From: Roman Bolshakov @ 2020-07-17 9:35 UTC (permalink / raw)
To: qemu-devel
Cc: Roman Bolshakov, Daniel P. Berrangé,
Cameron Esfahani, Stefan Hajnoczi
dtrace USDT is fully supported since OS X 10.6. There are a few
peculiarities compared to other dtrace flavors.
1. It doesn't accept empty files.
2. It doesn't recognize bool type but accepts C99 _Bool.
3. It converts int8_t * in probe points to char * in
header files and introduces [-Wpointer-sign] warning.
Cc: Cameron Esfahani <dirty@apple.com>
Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com>
---
scripts/tracetool/format/d.py | 15 ++++++++++++++-
1 file changed, 14 insertions(+), 1 deletion(-)
diff --git a/scripts/tracetool/format/d.py b/scripts/tracetool/format/d.py
index 0afb5f3f47..353722f89c 100644
--- a/scripts/tracetool/format/d.py
+++ b/scripts/tracetool/format/d.py
@@ -13,6 +13,7 @@ __email__ = "stefanha@redhat.com"
from tracetool import out
+from sys import platform
# Reserved keywords from
@@ -34,7 +35,8 @@ def generate(events, backend, group):
# SystemTap's dtrace(1) warns about empty "provider qemu {}" but is happy
# with an empty file. Avoid the warning.
- if not events:
+ # But dtrace on macOS can't deal with empty files.
+ if not events and platform != "darwin":
return
out('/* This file is autogenerated by tracetool, do not edit. */'
@@ -44,6 +46,17 @@ def generate(events, backend, group):
for e in events:
args = []
for type_, name in e.args:
+ if platform == "darwin":
+ # macOS dtrace accepts only C99 _Bool
+ if type_ == 'bool':
+ type_ = '_Bool'
+ if type_ == 'bool *':
+ type_ = '_Bool *'
+ # It converts int8_t * in probe points to char * in header
+ # files and introduces [-Wpointer-sign] warning.
+ # Avoid it by changing probe type to signed char * beforehand.
+ if type_ == 'int8_t *':
+ type_ = 'signed char *'
if name in RESERVED_WORDS:
name += '_'
args.append(type_ + ' ' + name)
--
2.26.1
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v2 2/4] scripts/tracetool: Use void pointer for vcpu
2020-07-17 9:35 [PATCH v2 0/4] Add dtrace support on macOS Roman Bolshakov
2020-07-17 9:35 ` [PATCH v2 1/4] scripts/tracetool: Fix dtrace generation for macOS Roman Bolshakov
@ 2020-07-17 9:35 ` Roman Bolshakov
2020-07-21 12:34 ` Stefan Hajnoczi
2020-07-17 9:35 ` [PATCH v2 3/4] build: Don't make object files for dtrace on macOS Roman Bolshakov
` (3 subsequent siblings)
5 siblings, 1 reply; 21+ messages in thread
From: Roman Bolshakov @ 2020-07-17 9:35 UTC (permalink / raw)
To: qemu-devel
Cc: Roman Bolshakov, Daniel P. Berrangé,
Cameron Esfahani, Stefan Hajnoczi
dtrace on macOS complains that CPUState * is used for a few probes:
dtrace: failed to compile script trace-dtrace-root.dtrace: line 130: syntax error near "CPUState"
A comment in scripts/tracetool/__init__.py mentions that:
We only want to allow standard C types or fixed sized
integer types. We don't want QEMU specific types
as we can't assume trace backends can resolve all the
typedefs
Fixes: 3d211d9f4dbee ("trace: Add 'vcpu' event property to trace guest vCPU")
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Cc: Cameron Esfahani <dirty@apple.com>
Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com>
---
scripts/tracetool/vcpu.py | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/scripts/tracetool/vcpu.py b/scripts/tracetool/vcpu.py
index b54e4f4e7a..868b4cb04c 100644
--- a/scripts/tracetool/vcpu.py
+++ b/scripts/tracetool/vcpu.py
@@ -24,7 +24,7 @@ def transform_event(event):
assert "tcg-trans" not in event.properties
assert "tcg-exec" not in event.properties
- event.args = Arguments([("CPUState *", "__cpu"), event.args])
+ event.args = Arguments([("void *", "__cpu"), event.args])
if "tcg" in event.properties:
fmt = "\"cpu=%p \""
event.fmt = [fmt + event.fmt[0],
--
2.26.1
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v2 3/4] build: Don't make object files for dtrace on macOS
2020-07-17 9:35 [PATCH v2 0/4] Add dtrace support on macOS Roman Bolshakov
2020-07-17 9:35 ` [PATCH v2 1/4] scripts/tracetool: Fix dtrace generation for macOS Roman Bolshakov
2020-07-17 9:35 ` [PATCH v2 2/4] scripts/tracetool: Use void pointer for vcpu Roman Bolshakov
@ 2020-07-17 9:35 ` Roman Bolshakov
2020-07-21 12:34 ` Stefan Hajnoczi
2020-07-24 17:00 ` Roman Bolshakov
2020-07-17 9:35 ` [PATCH v2 4/4] net/colo: Match is-enabled probe to tracepoint Roman Bolshakov
` (2 subsequent siblings)
5 siblings, 2 replies; 21+ messages in thread
From: Roman Bolshakov @ 2020-07-17 9:35 UTC (permalink / raw)
To: qemu-devel
Cc: Roman Bolshakov, Daniel P. Berrangé,
Cameron Esfahani, Stefan Hajnoczi
dtrace on macOS uses unresolved symbols with a special prefix to define
probes [1], only headers should be generated for USDT (dtrace(1)). But
it doesn't support backwards compatible no-op -G flag [2] and implicit
build rules fail.
1. https://markmail.org/message/6grq2ygr5nwdwsnb
2. https://markmail.org/message/5xrxt2w5m42nojkz
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Cc: Cameron Esfahani <dirty@apple.com>
Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com>
---
Makefile.objs | 2 ++
1 file changed, 2 insertions(+)
diff --git a/Makefile.objs b/Makefile.objs
index d22b3b45d7..982f15ba30 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -211,5 +211,7 @@ trace-events-files = $(SRC_PATH)/trace-events $(trace-events-subdirs:%=$(SRC_PAT
trace-obj-y = trace-root.o
trace-obj-y += $(trace-events-subdirs:%=%/trace.o)
trace-obj-$(CONFIG_TRACE_UST) += trace-ust-all.o
+ifneq ($(CONFIG_DARWIN),y)
trace-obj-$(CONFIG_TRACE_DTRACE) += trace-dtrace-root.o
trace-obj-$(CONFIG_TRACE_DTRACE) += $(trace-events-subdirs:%=%/trace-dtrace.o)
+endif
--
2.26.1
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v2 4/4] net/colo: Match is-enabled probe to tracepoint
2020-07-17 9:35 [PATCH v2 0/4] Add dtrace support on macOS Roman Bolshakov
` (2 preceding siblings ...)
2020-07-17 9:35 ` [PATCH v2 3/4] build: Don't make object files for dtrace on macOS Roman Bolshakov
@ 2020-07-17 9:35 ` Roman Bolshakov
2020-07-18 17:58 ` Zhang, Chen
2020-07-21 12:36 ` [PATCH v2 0/4] Add dtrace support on macOS Stefan Hajnoczi
2020-08-07 13:00 ` Stefan Hajnoczi
5 siblings, 1 reply; 21+ messages in thread
From: Roman Bolshakov @ 2020-07-17 9:35 UTC (permalink / raw)
To: qemu-devel
Cc: Daniel P. Berrangé,
Li Zhijian, Jason Wang, Cameron Esfahani, Zhang Chen,
Roman Bolshakov, Stefan Hajnoczi, Philippe Mathieu-Daudé
Build of QEMU with dtrace fails on macOS:
LINK x86_64-softmmu/qemu-system-x86_64
error: probe colo_compare_miscompare doesn't exist
error: Could not register probes
ld: error creating dtrace DOF section for architecture x86_64
The reason of the error is explained by Adam Leventhal [1]:
Note that is-enabled probes don't have the stability magic so I'm not
sure how things would work if only is-enabled probes were used.
net/colo code uses is-enabled probes to determine if other probes should
be used but colo_compare_miscompare itself is not used explicitly.
Linker doesn't include the symbol and build fails.
The issue can be resolved if is-enabled probe matches the actual trace
point that is used inside the test. Packet dump toggle is replaced with
a compile-time conditional definition.
1. http://markmail.org/message/6grq2ygr5nwdwsnb
Fixes: f4b618360e ("colo-compare: add TCP, UDP, ICMP packet comparison")
Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
Cc: Cameron Esfahani <dirty@apple.com>
Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com>
---
net/colo-compare.c | 42 ++++++++++++++++++++++--------------------
net/filter-rewriter.c | 10 ++++++++--
net/trace-events | 2 --
3 files changed, 30 insertions(+), 24 deletions(-)
diff --git a/net/colo-compare.c b/net/colo-compare.c
index 398b7546ff..e0be98e494 100644
--- a/net/colo-compare.c
+++ b/net/colo-compare.c
@@ -54,6 +54,8 @@ static NotifierList colo_compare_notifiers =
#define REGULAR_PACKET_CHECK_MS 3000
#define DEFAULT_TIME_OUT_MS 3000
+/* #define DEBUG_COLO_PACKETS */
+
static QemuMutex colo_compare_mutex;
static bool colo_compare_active;
static QemuMutex event_mtx;
@@ -327,7 +329,7 @@ static int colo_compare_packet_payload(Packet *ppkt,
uint16_t len)
{
- if (trace_event_get_state_backends(TRACE_COLO_COMPARE_MISCOMPARE)) {
+ if (trace_event_get_state_backends(TRACE_COLO_COMPARE_IP_INFO)) {
char pri_ip_src[20], pri_ip_dst[20], sec_ip_src[20], sec_ip_dst[20];
strcpy(pri_ip_src, inet_ntoa(ppkt->ip->ip_src));
@@ -492,12 +494,12 @@ sec:
g_queue_push_head(&conn->primary_list, ppkt);
g_queue_push_head(&conn->secondary_list, spkt);
- if (trace_event_get_state_backends(TRACE_COLO_COMPARE_MISCOMPARE)) {
- qemu_hexdump((char *)ppkt->data, stderr,
- "colo-compare ppkt", ppkt->size);
- qemu_hexdump((char *)spkt->data, stderr,
- "colo-compare spkt", spkt->size);
- }
+#ifdef DEBUG_COLO_PACKETS
+ qemu_hexdump((char *)ppkt->data, stderr,
+ "colo-compare ppkt", ppkt->size);
+ qemu_hexdump((char *)spkt->data, stderr,
+ "colo-compare spkt", spkt->size);
+#endif
colo_compare_inconsistency_notify(s);
}
@@ -533,12 +535,12 @@ static int colo_packet_compare_udp(Packet *spkt, Packet *ppkt)
ppkt->size - offset)) {
trace_colo_compare_udp_miscompare("primary pkt size", ppkt->size);
trace_colo_compare_udp_miscompare("Secondary pkt size", spkt->size);
- if (trace_event_get_state_backends(TRACE_COLO_COMPARE_MISCOMPARE)) {
- qemu_hexdump((char *)ppkt->data, stderr, "colo-compare pri pkt",
- ppkt->size);
- qemu_hexdump((char *)spkt->data, stderr, "colo-compare sec pkt",
- spkt->size);
- }
+#ifdef DEBUG_COLO_PACKETS
+ qemu_hexdump((char *)ppkt->data, stderr, "colo-compare pri pkt",
+ ppkt->size);
+ qemu_hexdump((char *)spkt->data, stderr, "colo-compare sec pkt",
+ spkt->size);
+#endif
return -1;
} else {
return 0;
@@ -576,12 +578,12 @@ static int colo_packet_compare_icmp(Packet *spkt, Packet *ppkt)
ppkt->size);
trace_colo_compare_icmp_miscompare("Secondary pkt size",
spkt->size);
- if (trace_event_get_state_backends(TRACE_COLO_COMPARE_MISCOMPARE)) {
- qemu_hexdump((char *)ppkt->data, stderr, "colo-compare pri pkt",
- ppkt->size);
- qemu_hexdump((char *)spkt->data, stderr, "colo-compare sec pkt",
- spkt->size);
- }
+#ifdef DEBUG_COLO_PACKETS
+ qemu_hexdump((char *)ppkt->data, stderr, "colo-compare pri pkt",
+ ppkt->size);
+ qemu_hexdump((char *)spkt->data, stderr, "colo-compare sec pkt",
+ spkt->size);
+#endif
return -1;
} else {
return 0;
@@ -597,7 +599,7 @@ static int colo_packet_compare_other(Packet *spkt, Packet *ppkt)
uint16_t offset = ppkt->vnet_hdr_len;
trace_colo_compare_main("compare other");
- if (trace_event_get_state_backends(TRACE_COLO_COMPARE_MISCOMPARE)) {
+ if (trace_event_get_state_backends(TRACE_COLO_COMPARE_IP_INFO)) {
char pri_ip_src[20], pri_ip_dst[20], sec_ip_src[20], sec_ip_dst[20];
strcpy(pri_ip_src, inet_ntoa(ppkt->ip->ip_src));
diff --git a/net/filter-rewriter.c b/net/filter-rewriter.c
index 1aaad101b6..576b019d09 100644
--- a/net/filter-rewriter.c
+++ b/net/filter-rewriter.c
@@ -76,11 +76,14 @@ static int handle_primary_tcp_pkt(RewriterState *rf,
struct tcp_hdr *tcp_pkt;
tcp_pkt = (struct tcp_hdr *)pkt->transport_header;
- if (trace_event_get_state_backends(TRACE_COLO_FILTER_REWRITER_DEBUG)) {
+ if (trace_event_get_state_backends(TRACE_COLO_FILTER_REWRITER_PKT_INFO)) {
trace_colo_filter_rewriter_pkt_info(__func__,
inet_ntoa(pkt->ip->ip_src), inet_ntoa(pkt->ip->ip_dst),
ntohl(tcp_pkt->th_seq), ntohl(tcp_pkt->th_ack),
tcp_pkt->th_flags);
+ }
+ if (trace_event_get_state_backends(
+ TRACE_COLO_FILTER_REWRITER_CONN_OFFSET)) {
trace_colo_filter_rewriter_conn_offset(conn->offset);
}
@@ -180,11 +183,14 @@ static int handle_secondary_tcp_pkt(RewriterState *rf,
tcp_pkt = (struct tcp_hdr *)pkt->transport_header;
- if (trace_event_get_state_backends(TRACE_COLO_FILTER_REWRITER_DEBUG)) {
+ if (trace_event_get_state_backends(TRACE_COLO_FILTER_REWRITER_PKT_INFO)) {
trace_colo_filter_rewriter_pkt_info(__func__,
inet_ntoa(pkt->ip->ip_src), inet_ntoa(pkt->ip->ip_dst),
ntohl(tcp_pkt->th_seq), ntohl(tcp_pkt->th_ack),
tcp_pkt->th_flags);
+ }
+ if (trace_event_get_state_backends(
+ TRACE_COLO_FILTER_REWRITER_CONN_OFFSET)) {
trace_colo_filter_rewriter_conn_offset(conn->offset);
}
diff --git a/net/trace-events b/net/trace-events
index fa49c71533..bfaff7891d 100644
--- a/net/trace-events
+++ b/net/trace-events
@@ -17,10 +17,8 @@ colo_compare_udp_miscompare(const char *sta, int size) ": %s = %d"
colo_compare_icmp_miscompare(const char *sta, int size) ": %s = %d"
colo_compare_ip_info(int psize, const char *sta, const char *stb, int ssize, const char *stc, const char *std) "ppkt size = %d, ip_src = %s, ip_dst = %s, spkt size = %d, ip_src = %s, ip_dst = %s"
colo_old_packet_check_found(int64_t old_time) "%" PRId64
-colo_compare_miscompare(void) ""
colo_compare_tcp_info(const char *pkt, uint32_t seq, uint32_t ack, int hdlen, int pdlen, int offset, int flags) "%s: seq/ack= %u/%u hdlen= %d pdlen= %d offset= %d flags=%d"
# filter-rewriter.c
-colo_filter_rewriter_debug(void) ""
colo_filter_rewriter_pkt_info(const char *func, const char *src, const char *dst, uint32_t seq, uint32_t ack, uint32_t flag) "%s: src/dst: %s/%s p: seq/ack=%u/%u flags=0x%x"
colo_filter_rewriter_conn_offset(uint32_t offset) ": offset=%u"
--
2.26.1
^ permalink raw reply related [flat|nested] 21+ messages in thread
* RE: [PATCH v2 4/4] net/colo: Match is-enabled probe to tracepoint
2020-07-17 9:35 ` [PATCH v2 4/4] net/colo: Match is-enabled probe to tracepoint Roman Bolshakov
@ 2020-07-18 17:58 ` Zhang, Chen
2020-07-20 10:59 ` Roman Bolshakov
2020-07-21 14:06 ` Daniel P. Berrangé
0 siblings, 2 replies; 21+ messages in thread
From: Zhang, Chen @ 2020-07-18 17:58 UTC (permalink / raw)
To: Roman Bolshakov, qemu-devel
Cc: Daniel P. Berrangé,
Li Zhijian, Jason Wang, Cameron Esfahani, Stefan Hajnoczi,
Philippe Mathieu-Daudé
> -----Original Message-----
> From: Roman Bolshakov <r.bolshakov@yadro.com>
> Sent: Friday, July 17, 2020 5:35 PM
> To: qemu-devel@nongnu.org
> Cc: Daniel P. Berrangé <berrange@redhat.com>; Stefan Hajnoczi
> <stefanha@redhat.com>; Cameron Esfahani <dirty@apple.com>; Roman
> Bolshakov <r.bolshakov@yadro.com>; Philippe Mathieu-Daudé
> <philmd@redhat.com>; Zhang, Chen <chen.zhang@intel.com>; Li Zhijian
> <lizhijian@cn.fujitsu.com>; Jason Wang <jasowang@redhat.com>
> Subject: [PATCH v2 4/4] net/colo: Match is-enabled probe to tracepoint
>
> Build of QEMU with dtrace fails on macOS:
>
> LINK x86_64-softmmu/qemu-system-x86_64
> error: probe colo_compare_miscompare doesn't exist
> error: Could not register probes
> ld: error creating dtrace DOF section for architecture x86_64
>
> The reason of the error is explained by Adam Leventhal [1]:
>
> Note that is-enabled probes don't have the stability magic so I'm not
> sure how things would work if only is-enabled probes were used.
>
> net/colo code uses is-enabled probes to determine if other probes should be
> used but colo_compare_miscompare itself is not used explicitly.
> Linker doesn't include the symbol and build fails.
>
> The issue can be resolved if is-enabled probe matches the actual trace point
> that is used inside the test. Packet dump toggle is replaced with a compile-
> time conditional definition.
>
> 1. http://markmail.org/message/6grq2ygr5nwdwsnb
>
> Fixes: f4b618360e ("colo-compare: add TCP, UDP, ICMP packet comparison")
> Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
> Cc: Cameron Esfahani <dirty@apple.com>
> Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com>
> ---
> net/colo-compare.c | 42 ++++++++++++++++++++++--------------------
> net/filter-rewriter.c | 10 ++++++++--
> net/trace-events | 2 --
> 3 files changed, 30 insertions(+), 24 deletions(-)
>
> diff --git a/net/colo-compare.c b/net/colo-compare.c index
> 398b7546ff..e0be98e494 100644
> --- a/net/colo-compare.c
> +++ b/net/colo-compare.c
> @@ -54,6 +54,8 @@ static NotifierList colo_compare_notifiers = #define
> REGULAR_PACKET_CHECK_MS 3000 #define DEFAULT_TIME_OUT_MS 3000
>
> +/* #define DEBUG_COLO_PACKETS */
> +
> static QemuMutex colo_compare_mutex;
> static bool colo_compare_active;
> static QemuMutex event_mtx;
> @@ -327,7 +329,7 @@ static int colo_compare_packet_payload(Packet
> *ppkt,
> uint16_t len)
>
> {
> - if
> (trace_event_get_state_backends(TRACE_COLO_COMPARE_MISCOMPARE))
> {
> + if (trace_event_get_state_backends(TRACE_COLO_COMPARE_IP_INFO))
> {
> char pri_ip_src[20], pri_ip_dst[20], sec_ip_src[20], sec_ip_dst[20];
>
> strcpy(pri_ip_src, inet_ntoa(ppkt->ip->ip_src)); @@ -492,12 +494,12
> @@ sec:
> g_queue_push_head(&conn->primary_list, ppkt);
> g_queue_push_head(&conn->secondary_list, spkt);
>
> - if
> (trace_event_get_state_backends(TRACE_COLO_COMPARE_MISCOMPARE))
> {
> - qemu_hexdump((char *)ppkt->data, stderr,
> - "colo-compare ppkt", ppkt->size);
> - qemu_hexdump((char *)spkt->data, stderr,
> - "colo-compare spkt", spkt->size);
> - }
> +#ifdef DEBUG_COLO_PACKETS
> + qemu_hexdump((char *)ppkt->data, stderr,
> + "colo-compare ppkt", ppkt->size);
> + qemu_hexdump((char *)spkt->data, stderr,
> + "colo-compare spkt", spkt->size); #endif
>
> colo_compare_inconsistency_notify(s);
> }
> @@ -533,12 +535,12 @@ static int colo_packet_compare_udp(Packet *spkt,
> Packet *ppkt)
> ppkt->size - offset)) {
> trace_colo_compare_udp_miscompare("primary pkt size", ppkt->size);
> trace_colo_compare_udp_miscompare("Secondary pkt size", spkt-
> >size);
> - if
> (trace_event_get_state_backends(TRACE_COLO_COMPARE_MISCOMPARE))
> {
> - qemu_hexdump((char *)ppkt->data, stderr, "colo-compare pri pkt",
> - ppkt->size);
> - qemu_hexdump((char *)spkt->data, stderr, "colo-compare sec pkt",
> - spkt->size);
> - }
> +#ifdef DEBUG_COLO_PACKETS
> + qemu_hexdump((char *)ppkt->data, stderr, "colo-compare pri pkt",
> + ppkt->size);
> + qemu_hexdump((char *)spkt->data, stderr, "colo-compare sec pkt",
> + spkt->size);
> +#endif
Hi Roman,
I think change the " trace_event_get_state_backends()" to "trace_colo_compare_main("Dump packet hex: ")" is a better choice here.
It will keep the original code logic and avoid the problem here.
Thanks
Zhang Chen
> return -1;
> } else {
> return 0;
> @@ -576,12 +578,12 @@ static int colo_packet_compare_icmp(Packet *spkt,
> Packet *ppkt)
> ppkt->size);
> trace_colo_compare_icmp_miscompare("Secondary pkt size",
> spkt->size);
> - if
> (trace_event_get_state_backends(TRACE_COLO_COMPARE_MISCOMPARE))
> {
> - qemu_hexdump((char *)ppkt->data, stderr, "colo-compare pri pkt",
> - ppkt->size);
> - qemu_hexdump((char *)spkt->data, stderr, "colo-compare sec pkt",
> - spkt->size);
> - }
> +#ifdef DEBUG_COLO_PACKETS
> + qemu_hexdump((char *)ppkt->data, stderr, "colo-compare pri pkt",
> + ppkt->size);
> + qemu_hexdump((char *)spkt->data, stderr, "colo-compare sec pkt",
> + spkt->size);
> +#endif
> return -1;
> } else {
> return 0;
> @@ -597,7 +599,7 @@ static int colo_packet_compare_other(Packet *spkt,
> Packet *ppkt)
> uint16_t offset = ppkt->vnet_hdr_len;
>
> trace_colo_compare_main("compare other");
> - if
> (trace_event_get_state_backends(TRACE_COLO_COMPARE_MISCOMPARE))
> {
> + if (trace_event_get_state_backends(TRACE_COLO_COMPARE_IP_INFO))
> {
> char pri_ip_src[20], pri_ip_dst[20], sec_ip_src[20], sec_ip_dst[20];
>
> strcpy(pri_ip_src, inet_ntoa(ppkt->ip->ip_src)); diff --git a/net/filter-
> rewriter.c b/net/filter-rewriter.c index 1aaad101b6..576b019d09 100644
> --- a/net/filter-rewriter.c
> +++ b/net/filter-rewriter.c
> @@ -76,11 +76,14 @@ static int handle_primary_tcp_pkt(RewriterState *rf,
> struct tcp_hdr *tcp_pkt;
>
> tcp_pkt = (struct tcp_hdr *)pkt->transport_header;
> - if
> (trace_event_get_state_backends(TRACE_COLO_FILTER_REWRITER_DEBUG
> )) {
> + if
> +
> (trace_event_get_state_backends(TRACE_COLO_FILTER_REWRITER_PKT_IN
> FO))
> + {
> trace_colo_filter_rewriter_pkt_info(__func__,
> inet_ntoa(pkt->ip->ip_src), inet_ntoa(pkt->ip->ip_dst),
> ntohl(tcp_pkt->th_seq), ntohl(tcp_pkt->th_ack),
> tcp_pkt->th_flags);
> + }
> + if (trace_event_get_state_backends(
> + TRACE_COLO_FILTER_REWRITER_CONN_OFFSET)) {
> trace_colo_filter_rewriter_conn_offset(conn->offset);
> }
>
> @@ -180,11 +183,14 @@ static int handle_secondary_tcp_pkt(RewriterState
> *rf,
>
> tcp_pkt = (struct tcp_hdr *)pkt->transport_header;
>
> - if
> (trace_event_get_state_backends(TRACE_COLO_FILTER_REWRITER_DEBUG
> )) {
> + if
> +
> (trace_event_get_state_backends(TRACE_COLO_FILTER_REWRITER_PKT_IN
> FO))
> + {
> trace_colo_filter_rewriter_pkt_info(__func__,
> inet_ntoa(pkt->ip->ip_src), inet_ntoa(pkt->ip->ip_dst),
> ntohl(tcp_pkt->th_seq), ntohl(tcp_pkt->th_ack),
> tcp_pkt->th_flags);
> + }
> + if (trace_event_get_state_backends(
> + TRACE_COLO_FILTER_REWRITER_CONN_OFFSET)) {
> trace_colo_filter_rewriter_conn_offset(conn->offset);
> }
>
> diff --git a/net/trace-events b/net/trace-events index
> fa49c71533..bfaff7891d 100644
> --- a/net/trace-events
> +++ b/net/trace-events
> @@ -17,10 +17,8 @@ colo_compare_udp_miscompare(const char *sta, int
> size) ": %s = %d"
> colo_compare_icmp_miscompare(const char *sta, int size) ": %s = %d"
> colo_compare_ip_info(int psize, const char *sta, const char *stb, int ssize,
> const char *stc, const char *std) "ppkt size = %d, ip_src = %s, ip_dst = %s,
> spkt size = %d, ip_src = %s, ip_dst = %s"
> colo_old_packet_check_found(int64_t old_time) "%" PRId64
> -colo_compare_miscompare(void) ""
> colo_compare_tcp_info(const char *pkt, uint32_t seq, uint32_t ack, int
> hdlen, int pdlen, int offset, int flags) "%s: seq/ack= %u/%u hdlen= %d
> pdlen= %d offset= %d flags=%d"
>
> # filter-rewriter.c
> -colo_filter_rewriter_debug(void) ""
> colo_filter_rewriter_pkt_info(const char *func, const char *src, const char
> *dst, uint32_t seq, uint32_t ack, uint32_t flag) "%s: src/dst: %s/%s p:
> seq/ack=%u/%u flags=0x%x"
> colo_filter_rewriter_conn_offset(uint32_t offset) ": offset=%u"
> --
> 2.26.1
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 1/4] scripts/tracetool: Fix dtrace generation for macOS
2020-07-17 9:35 ` [PATCH v2 1/4] scripts/tracetool: Fix dtrace generation for macOS Roman Bolshakov
@ 2020-07-19 13:52 ` Philippe Mathieu-Daudé
2020-07-20 10:50 ` Roman Bolshakov
2020-07-21 12:33 ` Stefan Hajnoczi
1 sibling, 1 reply; 21+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-07-19 13:52 UTC (permalink / raw)
To: Roman Bolshakov, qemu-devel
Cc: Daniel P. Berrangé, Stefan Hajnoczi, Cameron Esfahani
On 7/17/20 11:35 AM, Roman Bolshakov wrote:
> dtrace USDT is fully supported since OS X 10.6. There are a few
> peculiarities compared to other dtrace flavors.
>
> 1. It doesn't accept empty files.
> 2. It doesn't recognize bool type but accepts C99 _Bool.
> 3. It converts int8_t * in probe points to char * in
> header files and introduces [-Wpointer-sign] warning.
>
> Cc: Cameron Esfahani <dirty@apple.com>
> Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com>
> ---
> scripts/tracetool/format/d.py | 15 ++++++++++++++-
> 1 file changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/scripts/tracetool/format/d.py b/scripts/tracetool/format/d.py
> index 0afb5f3f47..353722f89c 100644
> --- a/scripts/tracetool/format/d.py
> +++ b/scripts/tracetool/format/d.py
> @@ -13,6 +13,7 @@ __email__ = "stefanha@redhat.com"
>
>
> from tracetool import out
> +from sys import platform
>
>
> # Reserved keywords from
> @@ -34,7 +35,8 @@ def generate(events, backend, group):
>
> # SystemTap's dtrace(1) warns about empty "provider qemu {}" but is happy
> # with an empty file. Avoid the warning.
> - if not events:
> + # But dtrace on macOS can't deal with empty files.
> + if not events and platform != "darwin":
or?
> return
>
> out('/* This file is autogenerated by tracetool, do not edit. */'
> @@ -44,6 +46,17 @@ def generate(events, backend, group):
> for e in events:
> args = []
> for type_, name in e.args:
> + if platform == "darwin":
> + # macOS dtrace accepts only C99 _Bool
Why not do that for all platforms?
> + if type_ == 'bool':
> + type_ = '_Bool'
> + if type_ == 'bool *':
> + type_ = '_Bool *'
> + # It converts int8_t * in probe points to char * in header
> + # files and introduces [-Wpointer-sign] warning.
> + # Avoid it by changing probe type to signed char * beforehand.
> + if type_ == 'int8_t *':
> + type_ = 'signed char *'
> if name in RESERVED_WORDS:
> name += '_'
> args.append(type_ + ' ' + name)
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 1/4] scripts/tracetool: Fix dtrace generation for macOS
2020-07-19 13:52 ` Philippe Mathieu-Daudé
@ 2020-07-20 10:50 ` Roman Bolshakov
2020-07-20 10:53 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 21+ messages in thread
From: Roman Bolshakov @ 2020-07-20 10:50 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Stefan Hajnoczi, Daniel P. Berrangé, qemu-devel, Cameron Esfahani
On Sun, Jul 19, 2020 at 03:52:08PM +0200, Philippe Mathieu-Daudé wrote:
> On 7/17/20 11:35 AM, Roman Bolshakov wrote:
> > dtrace USDT is fully supported since OS X 10.6. There are a few
> > peculiarities compared to other dtrace flavors.
> >
> > 1. It doesn't accept empty files.
> > 2. It doesn't recognize bool type but accepts C99 _Bool.
> > 3. It converts int8_t * in probe points to char * in
> > header files and introduces [-Wpointer-sign] warning.
> >
> > Cc: Cameron Esfahani <dirty@apple.com>
> > Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com>
> > ---
> > scripts/tracetool/format/d.py | 15 ++++++++++++++-
> > 1 file changed, 14 insertions(+), 1 deletion(-)
> >
> > diff --git a/scripts/tracetool/format/d.py b/scripts/tracetool/format/d.py
> > index 0afb5f3f47..353722f89c 100644
> > --- a/scripts/tracetool/format/d.py
> > +++ b/scripts/tracetool/format/d.py
> > @@ -13,6 +13,7 @@ __email__ = "stefanha@redhat.com"
> >
> >
> > from tracetool import out
> > +from sys import platform
> >
> >
> > # Reserved keywords from
> > @@ -34,7 +35,8 @@ def generate(events, backend, group):
> >
> > # SystemTap's dtrace(1) warns about empty "provider qemu {}" but is happy
> > # with an empty file. Avoid the warning.
> > - if not events:
> > + # But dtrace on macOS can't deal with empty files.
> > + if not events and platform != "darwin":
>
> or?
no, the event list is empty for some files where all events are
disabled (e.g. hppa/trace-events), so it should have an "and" here. This
limits early exit only on macOS. The precendence looks correct:
https://docs.python.org/3/reference/expressions.html#operator-precedence
>
> > return
> >
> > out('/* This file is autogenerated by tracetool, do not edit. */'
> > @@ -44,6 +46,17 @@ def generate(events, backend, group):
> > for e in events:
> > args = []
> > for type_, name in e.args:
> > + if platform == "darwin":
> > + # macOS dtrace accepts only C99 _Bool
>
> Why not do that for all platforms?
>
Because I can only test the changes on darwin :)
I don't know how other dtrace flavors behave and whether it is an issue
for dtrace on Linux/Solaris/FreeBSD/etc.
Thanks,
Roman
> > + if type_ == 'bool':
> > + type_ = '_Bool'
> > + if type_ == 'bool *':
> > + type_ = '_Bool *'
> > + # It converts int8_t * in probe points to char * in header
> > + # files and introduces [-Wpointer-sign] warning.
> > + # Avoid it by changing probe type to signed char * beforehand.
> > + if type_ == 'int8_t *':
> > + type_ = 'signed char *'
> > if name in RESERVED_WORDS:
> > name += '_'
> > args.append(type_ + ' ' + name)
> >
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 1/4] scripts/tracetool: Fix dtrace generation for macOS
2020-07-20 10:50 ` Roman Bolshakov
@ 2020-07-20 10:53 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 21+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-07-20 10:53 UTC (permalink / raw)
To: Roman Bolshakov
Cc: Stefan Hajnoczi, Daniel P. Berrangé, qemu-devel, Cameron Esfahani
On 7/20/20 12:50 PM, Roman Bolshakov wrote:
> On Sun, Jul 19, 2020 at 03:52:08PM +0200, Philippe Mathieu-Daudé wrote:
>> On 7/17/20 11:35 AM, Roman Bolshakov wrote:
>>> dtrace USDT is fully supported since OS X 10.6. There are a few
>>> peculiarities compared to other dtrace flavors.
>>>
>>> 1. It doesn't accept empty files.
>>> 2. It doesn't recognize bool type but accepts C99 _Bool.
>>> 3. It converts int8_t * in probe points to char * in
>>> header files and introduces [-Wpointer-sign] warning.
>>>
>>> Cc: Cameron Esfahani <dirty@apple.com>
>>> Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com>
>>> ---
>>> scripts/tracetool/format/d.py | 15 ++++++++++++++-
>>> 1 file changed, 14 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/scripts/tracetool/format/d.py b/scripts/tracetool/format/d.py
>>> index 0afb5f3f47..353722f89c 100644
>>> --- a/scripts/tracetool/format/d.py
>>> +++ b/scripts/tracetool/format/d.py
>>> @@ -13,6 +13,7 @@ __email__ = "stefanha@redhat.com"
>>>
>>>
>>> from tracetool import out
>>> +from sys import platform
>>>
>>>
>>> # Reserved keywords from
>>> @@ -34,7 +35,8 @@ def generate(events, backend, group):
>>>
>>> # SystemTap's dtrace(1) warns about empty "provider qemu {}" but is happy
>>> # with an empty file. Avoid the warning.
>>> - if not events:
>>> + # But dtrace on macOS can't deal with empty files.
>>> + if not events and platform != "darwin":
>>
>> or?
>
> no, the event list is empty for some files where all events are
> disabled (e.g. hppa/trace-events), so it should have an "and" here. This
> limits early exit only on macOS.
Ah yes you are right, I misread it.
> The precendence looks correct:
>
> https://docs.python.org/3/reference/expressions.html#operator-precedence
>
>>
>>> return
>>>
>>> out('/* This file is autogenerated by tracetool, do not edit. */'
>>> @@ -44,6 +46,17 @@ def generate(events, backend, group):
>>> for e in events:
>>> args = []
>>> for type_, name in e.args:
>>> + if platform == "darwin":
>>> + # macOS dtrace accepts only C99 _Bool
>>
>> Why not do that for all platforms?
>>
>
> Because I can only test the changes on darwin :)
> I don't know how other dtrace flavors behave and whether it is an issue
> for dtrace on Linux/Solaris/FreeBSD/etc.
>
> Thanks,
> Roman
>
>>> + if type_ == 'bool':
>>> + type_ = '_Bool'
>>> + if type_ == 'bool *':
>>> + type_ = '_Bool *'
>>> + # It converts int8_t * in probe points to char * in header
>>> + # files and introduces [-Wpointer-sign] warning.
>>> + # Avoid it by changing probe type to signed char * beforehand.
>>> + if type_ == 'int8_t *':
>>> + type_ = 'signed char *'
>>> if name in RESERVED_WORDS:
>>> name += '_'
>>> args.append(type_ + ' ' + name)
>>>
>>
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 4/4] net/colo: Match is-enabled probe to tracepoint
2020-07-18 17:58 ` Zhang, Chen
@ 2020-07-20 10:59 ` Roman Bolshakov
2020-07-21 14:06 ` Daniel P. Berrangé
1 sibling, 0 replies; 21+ messages in thread
From: Roman Bolshakov @ 2020-07-20 10:59 UTC (permalink / raw)
To: Zhang, Chen
Cc: Daniel P. Berrangé,
Li Zhijian, Jason Wang, qemu-devel, Cameron Esfahani,
Stefan Hajnoczi, Philippe Mathieu-Daudé
On Sat, Jul 18, 2020 at 05:58:56PM +0000, Zhang, Chen wrote:
> > -----Original Message-----
> > From: Roman Bolshakov <r.bolshakov@yadro.com>
> > Sent: Friday, July 17, 2020 5:35 PM
> > @@ -533,12 +535,12 @@ static int colo_packet_compare_udp(Packet *spkt,
> > Packet *ppkt)
> > ppkt->size - offset)) {
> > trace_colo_compare_udp_miscompare("primary pkt size", ppkt->size);
> > trace_colo_compare_udp_miscompare("Secondary pkt size", spkt-
> > >size);
> > - if
> > (trace_event_get_state_backends(TRACE_COLO_COMPARE_MISCOMPARE))
> > {
> > - qemu_hexdump((char *)ppkt->data, stderr, "colo-compare pri pkt",
> > - ppkt->size);
> > - qemu_hexdump((char *)spkt->data, stderr, "colo-compare sec pkt",
> > - spkt->size);
> > - }
> > +#ifdef DEBUG_COLO_PACKETS
> > + qemu_hexdump((char *)ppkt->data, stderr, "colo-compare pri pkt",
> > + ppkt->size);
> > + qemu_hexdump((char *)spkt->data, stderr, "colo-compare sec pkt",
> > + spkt->size);
> > +#endif
>
> I think change the " trace_event_get_state_backends()" to "trace_colo_compare_main("Dump packet hex: ")" is a better choice here.
> It will keep the original code logic and avoid the problem here.
>
Hi Chen,
Do you mean to use another is-enabled probe?
e.g. change from
"if (trace_event_get_state_backends(TRACE_COLO_COMPARE_MISCOMPARE))"
to
"if (trace_event_get_state_backends(TRACE_COLO_COMPARE_MAIN))"
Thanks,
Roman
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 1/4] scripts/tracetool: Fix dtrace generation for macOS
2020-07-17 9:35 ` [PATCH v2 1/4] scripts/tracetool: Fix dtrace generation for macOS Roman Bolshakov
2020-07-19 13:52 ` Philippe Mathieu-Daudé
@ 2020-07-21 12:33 ` Stefan Hajnoczi
1 sibling, 0 replies; 21+ messages in thread
From: Stefan Hajnoczi @ 2020-07-21 12:33 UTC (permalink / raw)
To: Roman Bolshakov; +Cc: Daniel P. Berrangé, qemu-devel, Cameron Esfahani
[-- Attachment #1: Type: text/plain, Size: 673 bytes --]
On Fri, Jul 17, 2020 at 12:35:14PM +0300, Roman Bolshakov wrote:
> dtrace USDT is fully supported since OS X 10.6. There are a few
> peculiarities compared to other dtrace flavors.
>
> 1. It doesn't accept empty files.
> 2. It doesn't recognize bool type but accepts C99 _Bool.
> 3. It converts int8_t * in probe points to char * in
> header files and introduces [-Wpointer-sign] warning.
>
> Cc: Cameron Esfahani <dirty@apple.com>
> Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com>
> ---
> scripts/tracetool/format/d.py | 15 ++++++++++++++-
> 1 file changed, 14 insertions(+), 1 deletion(-)
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 2/4] scripts/tracetool: Use void pointer for vcpu
2020-07-17 9:35 ` [PATCH v2 2/4] scripts/tracetool: Use void pointer for vcpu Roman Bolshakov
@ 2020-07-21 12:34 ` Stefan Hajnoczi
0 siblings, 0 replies; 21+ messages in thread
From: Stefan Hajnoczi @ 2020-07-21 12:34 UTC (permalink / raw)
To: Roman Bolshakov; +Cc: Daniel P. Berrangé, qemu-devel, Cameron Esfahani
[-- Attachment #1: Type: text/plain, Size: 889 bytes --]
On Fri, Jul 17, 2020 at 12:35:15PM +0300, Roman Bolshakov wrote:
> dtrace on macOS complains that CPUState * is used for a few probes:
>
> dtrace: failed to compile script trace-dtrace-root.dtrace: line 130: syntax error near "CPUState"
>
> A comment in scripts/tracetool/__init__.py mentions that:
>
> We only want to allow standard C types or fixed sized
> integer types. We don't want QEMU specific types
> as we can't assume trace backends can resolve all the
> typedefs
>
> Fixes: 3d211d9f4dbee ("trace: Add 'vcpu' event property to trace guest vCPU")
> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
> Cc: Cameron Esfahani <dirty@apple.com>
> Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com>
> ---
> scripts/tracetool/vcpu.py | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 3/4] build: Don't make object files for dtrace on macOS
2020-07-17 9:35 ` [PATCH v2 3/4] build: Don't make object files for dtrace on macOS Roman Bolshakov
@ 2020-07-21 12:34 ` Stefan Hajnoczi
2020-07-24 17:00 ` Roman Bolshakov
1 sibling, 0 replies; 21+ messages in thread
From: Stefan Hajnoczi @ 2020-07-21 12:34 UTC (permalink / raw)
To: Roman Bolshakov; +Cc: Daniel P. Berrangé, qemu-devel, Cameron Esfahani
[-- Attachment #1: Type: text/plain, Size: 701 bytes --]
On Fri, Jul 17, 2020 at 12:35:16PM +0300, Roman Bolshakov wrote:
> dtrace on macOS uses unresolved symbols with a special prefix to define
> probes [1], only headers should be generated for USDT (dtrace(1)). But
> it doesn't support backwards compatible no-op -G flag [2] and implicit
> build rules fail.
>
> 1. https://markmail.org/message/6grq2ygr5nwdwsnb
> 2. https://markmail.org/message/5xrxt2w5m42nojkz
>
> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
> Cc: Cameron Esfahani <dirty@apple.com>
> Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com>
> ---
> Makefile.objs | 2 ++
> 1 file changed, 2 insertions(+)
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 0/4] Add dtrace support on macOS
2020-07-17 9:35 [PATCH v2 0/4] Add dtrace support on macOS Roman Bolshakov
` (3 preceding siblings ...)
2020-07-17 9:35 ` [PATCH v2 4/4] net/colo: Match is-enabled probe to tracepoint Roman Bolshakov
@ 2020-07-21 12:36 ` Stefan Hajnoczi
2020-08-07 13:00 ` Stefan Hajnoczi
5 siblings, 0 replies; 21+ messages in thread
From: Stefan Hajnoczi @ 2020-07-21 12:36 UTC (permalink / raw)
To: Roman Bolshakov; +Cc: Daniel P. Berrangé, qemu-devel, Cameron Esfahani
[-- Attachment #1: Type: text/plain, Size: 395 bytes --]
On Fri, Jul 17, 2020 at 12:35:13PM +0300, Roman Bolshakov wrote:
> Hi,
>
> This is a small series that enables dtrace tracing backend on macOS.
> Whether or not it should go to 5.1 is up to discretion of tracing
> maintainers.
Thanks for the patches. I would like to apply them for QEMU 5.2.
I have reviewed patches 1-3 and will wait for discussion to finish on
patch 4.
Stefan
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 4/4] net/colo: Match is-enabled probe to tracepoint
2020-07-18 17:58 ` Zhang, Chen
2020-07-20 10:59 ` Roman Bolshakov
@ 2020-07-21 14:06 ` Daniel P. Berrangé
2020-07-29 12:33 ` Roman Bolshakov
1 sibling, 1 reply; 21+ messages in thread
From: Daniel P. Berrangé @ 2020-07-21 14:06 UTC (permalink / raw)
To: Zhang, Chen
Cc: Li Zhijian, Jason Wang, qemu-devel, Cameron Esfahani,
Roman Bolshakov, Stefan Hajnoczi, Philippe Mathieu-Daudé
On Sat, Jul 18, 2020 at 05:58:56PM +0000, Zhang, Chen wrote:
>
>
> > -----Original Message-----
> > From: Roman Bolshakov <r.bolshakov@yadro.com>
> > Sent: Friday, July 17, 2020 5:35 PM
> > To: qemu-devel@nongnu.org
> > Cc: Daniel P. Berrangé <berrange@redhat.com>; Stefan Hajnoczi
> > <stefanha@redhat.com>; Cameron Esfahani <dirty@apple.com>; Roman
> > Bolshakov <r.bolshakov@yadro.com>; Philippe Mathieu-Daudé
> > <philmd@redhat.com>; Zhang, Chen <chen.zhang@intel.com>; Li Zhijian
> > <lizhijian@cn.fujitsu.com>; Jason Wang <jasowang@redhat.com>
> > Subject: [PATCH v2 4/4] net/colo: Match is-enabled probe to tracepoint
> >
> > Build of QEMU with dtrace fails on macOS:
> >
> > LINK x86_64-softmmu/qemu-system-x86_64
> > error: probe colo_compare_miscompare doesn't exist
> > error: Could not register probes
> > ld: error creating dtrace DOF section for architecture x86_64
> >
> > The reason of the error is explained by Adam Leventhal [1]:
> >
> > Note that is-enabled probes don't have the stability magic so I'm not
> > sure how things would work if only is-enabled probes were used.
> >
> > net/colo code uses is-enabled probes to determine if other probes should be
> > used but colo_compare_miscompare itself is not used explicitly.
> > Linker doesn't include the symbol and build fails.
> >
> > The issue can be resolved if is-enabled probe matches the actual trace point
> > that is used inside the test. Packet dump toggle is replaced with a compile-
> > time conditional definition.
> >
> > 1. http://markmail.org/message/6grq2ygr5nwdwsnb
> >
> > Fixes: f4b618360e ("colo-compare: add TCP, UDP, ICMP packet comparison")
> > Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
> > Cc: Cameron Esfahani <dirty@apple.com>
> > Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com>
> > ---
> > net/colo-compare.c | 42 ++++++++++++++++++++++--------------------
> > net/filter-rewriter.c | 10 ++++++++--
> > net/trace-events | 2 --
> > 3 files changed, 30 insertions(+), 24 deletions(-)
> > (trace_event_get_state_backends(TRACE_COLO_COMPARE_MISCOMPARE))
> > {
> > + if (trace_event_get_state_backends(TRACE_COLO_COMPARE_IP_INFO))
> > {
> > char pri_ip_src[20], pri_ip_dst[20], sec_ip_src[20], sec_ip_dst[20];
> >
> > strcpy(pri_ip_src, inet_ntoa(ppkt->ip->ip_src)); @@ -492,12 +494,12
> > @@ sec:
> > g_queue_push_head(&conn->primary_list, ppkt);
> > g_queue_push_head(&conn->secondary_list, spkt);
> >
> > - if
> > (trace_event_get_state_backends(TRACE_COLO_COMPARE_MISCOMPARE))
> > {
> > - qemu_hexdump((char *)ppkt->data, stderr,
> > - "colo-compare ppkt", ppkt->size);
> > - qemu_hexdump((char *)spkt->data, stderr,
> > - "colo-compare spkt", spkt->size);
> > - }
> > +#ifdef DEBUG_COLO_PACKETS
> > + qemu_hexdump((char *)ppkt->data, stderr,
> > + "colo-compare ppkt", ppkt->size);
> > + qemu_hexdump((char *)spkt->data, stderr,
> > + "colo-compare spkt", spkt->size); #endif
> >
> > colo_compare_inconsistency_notify(s);
> > }
> > @@ -533,12 +535,12 @@ static int colo_packet_compare_udp(Packet *spkt,
> > Packet *ppkt)
> > ppkt->size - offset)) {
> > trace_colo_compare_udp_miscompare("primary pkt size", ppkt->size);
> > trace_colo_compare_udp_miscompare("Secondary pkt size", spkt-
> > >size);
> > - if
> > (trace_event_get_state_backends(TRACE_COLO_COMPARE_MISCOMPARE))
> > {
> > - qemu_hexdump((char *)ppkt->data, stderr, "colo-compare pri pkt",
> > - ppkt->size);
> > - qemu_hexdump((char *)spkt->data, stderr, "colo-compare sec pkt",
> > - spkt->size);
> > - }
> > +#ifdef DEBUG_COLO_PACKETS
> > + qemu_hexdump((char *)ppkt->data, stderr, "colo-compare pri pkt",
> > + ppkt->size);
> > + qemu_hexdump((char *)spkt->data, stderr, "colo-compare sec pkt",
> > + spkt->size);
> > +#endif
>
> Hi Roman,
>
> I think change the " trace_event_get_state_backends()" to
> "trace_colo_compare_main("Dump packet hex: ")" is a better choice here.
> It will keep the original code logic and avoid the problem here.
That may workaround the immediate bug, but this is still a misuse of the
tracing code. Use of any trace point should only trigger actions in the
trace infrastructure.
If I'm using dtrace backend to monitor events I don't want to see QEMU
dumping stuff to stderr. Anything written to stderr is going to trigger
disk I/O writing to the VM's logfile, and is also liable to trigger rate
limiting which can impact the guest performance.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 3/4] build: Don't make object files for dtrace on macOS
2020-07-17 9:35 ` [PATCH v2 3/4] build: Don't make object files for dtrace on macOS Roman Bolshakov
2020-07-21 12:34 ` Stefan Hajnoczi
@ 2020-07-24 17:00 ` Roman Bolshakov
1 sibling, 0 replies; 21+ messages in thread
From: Roman Bolshakov @ 2020-07-24 17:00 UTC (permalink / raw)
To: qemu-devel; +Cc: Daniel P. Berrangé, Cameron Esfahani, Stefan Hajnoczi
On Fri, Jul 17, 2020 at 12:35:16PM +0300, Roman Bolshakov wrote:
> dtrace on macOS uses unresolved symbols with a special prefix to define
> probes [1], only headers should be generated for USDT (dtrace(1)). But
> it doesn't support backwards compatible no-op -G flag [2] and implicit
> build rules fail.
>
> 1. https://markmail.org/message/6grq2ygr5nwdwsnb
> 2. https://markmail.org/message/5xrxt2w5m42nojkz
>
> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
> Cc: Cameron Esfahani <dirty@apple.com>
> Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com>
> ---
> Makefile.objs | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/Makefile.objs b/Makefile.objs
> index d22b3b45d7..982f15ba30 100644
> --- a/Makefile.objs
> +++ b/Makefile.objs
> @@ -211,5 +211,7 @@ trace-events-files = $(SRC_PATH)/trace-events $(trace-events-subdirs:%=$(SRC_PAT
> trace-obj-y = trace-root.o
> trace-obj-y += $(trace-events-subdirs:%=%/trace.o)
> trace-obj-$(CONFIG_TRACE_UST) += trace-ust-all.o
> +ifneq ($(CONFIG_DARWIN),y)
> trace-obj-$(CONFIG_TRACE_DTRACE) += trace-dtrace-root.o
> trace-obj-$(CONFIG_TRACE_DTRACE) += $(trace-events-subdirs:%=%/trace-dtrace.o)
> +endif
> --
> 2.26.1
>
An article about DTrace [1] mentions that FreeBSD also doesn't need that:
"On FreeBSD/Mac OS X, you do not have to generate a separate probe
object file for linking. This makes the compilation process much more
straightforward [...]"
I don't know for sure but perhaps "-G" makes dummy object files there.
1. https://www.ibm.com/developerworks/aix/library/au-dtraceprobes.html
Thanks,
Roman
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 4/4] net/colo: Match is-enabled probe to tracepoint
2020-07-21 14:06 ` Daniel P. Berrangé
@ 2020-07-29 12:33 ` Roman Bolshakov
2020-07-29 12:34 ` Daniel P. Berrangé
0 siblings, 1 reply; 21+ messages in thread
From: Roman Bolshakov @ 2020-07-29 12:33 UTC (permalink / raw)
To: Daniel P. Berrangé, Zhang, Chen, Stefan Hajnoczi
Cc: Cameron Esfahani, Jason Wang, Philippe Mathieu-Daudé,
qemu-devel, Li Zhijian
On Tue, Jul 21, 2020 at 03:06:57PM +0100, Daniel P. Berrangé wrote:
> On Sat, Jul 18, 2020 at 05:58:56PM +0000, Zhang, Chen wrote:
> >
> >
> > > -----Original Message-----
> > > From: Roman Bolshakov <r.bolshakov@yadro.com>
> > > Sent: Friday, July 17, 2020 5:35 PM
> > > To: qemu-devel@nongnu.org
> > > Cc: Daniel P. Berrangé <berrange@redhat.com>; Stefan Hajnoczi
> > > <stefanha@redhat.com>; Cameron Esfahani <dirty@apple.com>; Roman
> > > Bolshakov <r.bolshakov@yadro.com>; Philippe Mathieu-Daudé
> > > <philmd@redhat.com>; Zhang, Chen <chen.zhang@intel.com>; Li Zhijian
> > > <lizhijian@cn.fujitsu.com>; Jason Wang <jasowang@redhat.com>
> > > Subject: [PATCH v2 4/4] net/colo: Match is-enabled probe to tracepoint
> > >
> > > Build of QEMU with dtrace fails on macOS:
> > >
> > > LINK x86_64-softmmu/qemu-system-x86_64
> > > error: probe colo_compare_miscompare doesn't exist
> > > error: Could not register probes
> > > ld: error creating dtrace DOF section for architecture x86_64
> > >
> > > The reason of the error is explained by Adam Leventhal [1]:
> > >
> > > Note that is-enabled probes don't have the stability magic so I'm not
> > > sure how things would work if only is-enabled probes were used.
> > >
> > > net/colo code uses is-enabled probes to determine if other probes should be
> > > used but colo_compare_miscompare itself is not used explicitly.
> > > Linker doesn't include the symbol and build fails.
> > >
> > > The issue can be resolved if is-enabled probe matches the actual trace point
> > > that is used inside the test. Packet dump toggle is replaced with a compile-
> > > time conditional definition.
> > >
> > > 1. http://markmail.org/message/6grq2ygr5nwdwsnb
> > >
> > > Fixes: f4b618360e ("colo-compare: add TCP, UDP, ICMP packet comparison")
> > > Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
> > > Cc: Cameron Esfahani <dirty@apple.com>
> > > Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com>
> > > ---
> > > net/colo-compare.c | 42 ++++++++++++++++++++++--------------------
> > > net/filter-rewriter.c | 10 ++++++++--
> > > net/trace-events | 2 --
> > > 3 files changed, 30 insertions(+), 24 deletions(-)
>
>
> > > (trace_event_get_state_backends(TRACE_COLO_COMPARE_MISCOMPARE))
> > > {
> > > + if (trace_event_get_state_backends(TRACE_COLO_COMPARE_IP_INFO))
> > > {
> > > char pri_ip_src[20], pri_ip_dst[20], sec_ip_src[20], sec_ip_dst[20];
> > >
> > > strcpy(pri_ip_src, inet_ntoa(ppkt->ip->ip_src)); @@ -492,12 +494,12
> > > @@ sec:
> > > g_queue_push_head(&conn->primary_list, ppkt);
> > > g_queue_push_head(&conn->secondary_list, spkt);
> > >
> > > - if
> > > (trace_event_get_state_backends(TRACE_COLO_COMPARE_MISCOMPARE))
> > > {
> > > - qemu_hexdump((char *)ppkt->data, stderr,
> > > - "colo-compare ppkt", ppkt->size);
> > > - qemu_hexdump((char *)spkt->data, stderr,
> > > - "colo-compare spkt", spkt->size);
> > > - }
> > > +#ifdef DEBUG_COLO_PACKETS
> > > + qemu_hexdump((char *)ppkt->data, stderr,
> > > + "colo-compare ppkt", ppkt->size);
> > > + qemu_hexdump((char *)spkt->data, stderr,
> > > + "colo-compare spkt", spkt->size); #endif
> > >
> > > colo_compare_inconsistency_notify(s);
> > > }
> > > @@ -533,12 +535,12 @@ static int colo_packet_compare_udp(Packet *spkt,
> > > Packet *ppkt)
> > > ppkt->size - offset)) {
> > > trace_colo_compare_udp_miscompare("primary pkt size", ppkt->size);
> > > trace_colo_compare_udp_miscompare("Secondary pkt size", spkt-
> > > >size);
> > > - if
> > > (trace_event_get_state_backends(TRACE_COLO_COMPARE_MISCOMPARE))
> > > {
> > > - qemu_hexdump((char *)ppkt->data, stderr, "colo-compare pri pkt",
> > > - ppkt->size);
> > > - qemu_hexdump((char *)spkt->data, stderr, "colo-compare sec pkt",
> > > - spkt->size);
> > > - }
> > > +#ifdef DEBUG_COLO_PACKETS
> > > + qemu_hexdump((char *)ppkt->data, stderr, "colo-compare pri pkt",
> > > + ppkt->size);
> > > + qemu_hexdump((char *)spkt->data, stderr, "colo-compare sec pkt",
> > > + spkt->size);
> > > +#endif
> >
> > Hi Roman,
> >
> > I think change the " trace_event_get_state_backends()" to
> > "trace_colo_compare_main("Dump packet hex: ")" is a better choice here.
> > It will keep the original code logic and avoid the problem here.
>
> That may workaround the immediate bug, but this is still a misuse of the
> tracing code. Use of any trace point should only trigger actions in the
> trace infrastructure.
>
> If I'm using dtrace backend to monitor events I don't want to see QEMU
> dumping stuff to stderr. Anything written to stderr is going to trigger
> disk I/O writing to the VM's logfile, and is also liable to trigger rate
> limiting which can impact the guest performance.
>
Hi Daniel, Chen, Stefan,
So, what do we want to do about the series? Do we have an agreement? Is
the patch okay or I should make a change?
BTW. I've found that Apple added trace probes to Hypervisor.framework in
Big Sur and there're fbt probes in AppleHV.kext. Addition of dtrace on
macOS helps to find performance or functional issues. (I'm using the
series in my private branches for debugging).
Thanks,
Roman
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 4/4] net/colo: Match is-enabled probe to tracepoint
2020-07-29 12:33 ` Roman Bolshakov
@ 2020-07-29 12:34 ` Daniel P. Berrangé
2020-08-05 10:53 ` Stefan Hajnoczi
0 siblings, 1 reply; 21+ messages in thread
From: Daniel P. Berrangé @ 2020-07-29 12:34 UTC (permalink / raw)
To: Roman Bolshakov
Cc: Li Zhijian, Jason Wang, qemu-devel, Cameron Esfahani, Zhang,
Chen, Stefan Hajnoczi, Philippe Mathieu-Daudé
On Wed, Jul 29, 2020 at 03:33:22PM +0300, Roman Bolshakov wrote:
> On Tue, Jul 21, 2020 at 03:06:57PM +0100, Daniel P. Berrangé wrote:
> > On Sat, Jul 18, 2020 at 05:58:56PM +0000, Zhang, Chen wrote:
> > >
> > >
> > > > -----Original Message-----
> > > > From: Roman Bolshakov <r.bolshakov@yadro.com>
> > > > Sent: Friday, July 17, 2020 5:35 PM
> > > > To: qemu-devel@nongnu.org
> > > > Cc: Daniel P. Berrangé <berrange@redhat.com>; Stefan Hajnoczi
> > > > <stefanha@redhat.com>; Cameron Esfahani <dirty@apple.com>; Roman
> > > > Bolshakov <r.bolshakov@yadro.com>; Philippe Mathieu-Daudé
> > > > <philmd@redhat.com>; Zhang, Chen <chen.zhang@intel.com>; Li Zhijian
> > > > <lizhijian@cn.fujitsu.com>; Jason Wang <jasowang@redhat.com>
> > > > Subject: [PATCH v2 4/4] net/colo: Match is-enabled probe to tracepoint
> > > >
> > > > Build of QEMU with dtrace fails on macOS:
> > > >
> > > > LINK x86_64-softmmu/qemu-system-x86_64
> > > > error: probe colo_compare_miscompare doesn't exist
> > > > error: Could not register probes
> > > > ld: error creating dtrace DOF section for architecture x86_64
> > > >
> > > > The reason of the error is explained by Adam Leventhal [1]:
> > > >
> > > > Note that is-enabled probes don't have the stability magic so I'm not
> > > > sure how things would work if only is-enabled probes were used.
> > > >
> > > > net/colo code uses is-enabled probes to determine if other probes should be
> > > > used but colo_compare_miscompare itself is not used explicitly.
> > > > Linker doesn't include the symbol and build fails.
> > > >
> > > > The issue can be resolved if is-enabled probe matches the actual trace point
> > > > that is used inside the test. Packet dump toggle is replaced with a compile-
> > > > time conditional definition.
> > > >
> > > > 1. http://markmail.org/message/6grq2ygr5nwdwsnb
> > > >
> > > > Fixes: f4b618360e ("colo-compare: add TCP, UDP, ICMP packet comparison")
> > > > Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
> > > > Cc: Cameron Esfahani <dirty@apple.com>
> > > > Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com>
> > > > ---
> > > > net/colo-compare.c | 42 ++++++++++++++++++++++--------------------
> > > > net/filter-rewriter.c | 10 ++++++++--
> > > > net/trace-events | 2 --
> > > > 3 files changed, 30 insertions(+), 24 deletions(-)
> >
> >
> > > > (trace_event_get_state_backends(TRACE_COLO_COMPARE_MISCOMPARE))
> > > > {
> > > > + if (trace_event_get_state_backends(TRACE_COLO_COMPARE_IP_INFO))
> > > > {
> > > > char pri_ip_src[20], pri_ip_dst[20], sec_ip_src[20], sec_ip_dst[20];
> > > >
> > > > strcpy(pri_ip_src, inet_ntoa(ppkt->ip->ip_src)); @@ -492,12 +494,12
> > > > @@ sec:
> > > > g_queue_push_head(&conn->primary_list, ppkt);
> > > > g_queue_push_head(&conn->secondary_list, spkt);
> > > >
> > > > - if
> > > > (trace_event_get_state_backends(TRACE_COLO_COMPARE_MISCOMPARE))
> > > > {
> > > > - qemu_hexdump((char *)ppkt->data, stderr,
> > > > - "colo-compare ppkt", ppkt->size);
> > > > - qemu_hexdump((char *)spkt->data, stderr,
> > > > - "colo-compare spkt", spkt->size);
> > > > - }
> > > > +#ifdef DEBUG_COLO_PACKETS
> > > > + qemu_hexdump((char *)ppkt->data, stderr,
> > > > + "colo-compare ppkt", ppkt->size);
> > > > + qemu_hexdump((char *)spkt->data, stderr,
> > > > + "colo-compare spkt", spkt->size); #endif
> > > >
> > > > colo_compare_inconsistency_notify(s);
> > > > }
> > > > @@ -533,12 +535,12 @@ static int colo_packet_compare_udp(Packet *spkt,
> > > > Packet *ppkt)
> > > > ppkt->size - offset)) {
> > > > trace_colo_compare_udp_miscompare("primary pkt size", ppkt->size);
> > > > trace_colo_compare_udp_miscompare("Secondary pkt size", spkt-
> > > > >size);
> > > > - if
> > > > (trace_event_get_state_backends(TRACE_COLO_COMPARE_MISCOMPARE))
> > > > {
> > > > - qemu_hexdump((char *)ppkt->data, stderr, "colo-compare pri pkt",
> > > > - ppkt->size);
> > > > - qemu_hexdump((char *)spkt->data, stderr, "colo-compare sec pkt",
> > > > - spkt->size);
> > > > - }
> > > > +#ifdef DEBUG_COLO_PACKETS
> > > > + qemu_hexdump((char *)ppkt->data, stderr, "colo-compare pri pkt",
> > > > + ppkt->size);
> > > > + qemu_hexdump((char *)spkt->data, stderr, "colo-compare sec pkt",
> > > > + spkt->size);
> > > > +#endif
> > >
> > > Hi Roman,
> > >
> > > I think change the " trace_event_get_state_backends()" to
> > > "trace_colo_compare_main("Dump packet hex: ")" is a better choice here.
> > > It will keep the original code logic and avoid the problem here.
> >
> > That may workaround the immediate bug, but this is still a misuse of the
> > tracing code. Use of any trace point should only trigger actions in the
> > trace infrastructure.
> >
> > If I'm using dtrace backend to monitor events I don't want to see QEMU
> > dumping stuff to stderr. Anything written to stderr is going to trigger
> > disk I/O writing to the VM's logfile, and is also liable to trigger rate
> > limiting which can impact the guest performance.
> >
>
> Hi Daniel, Chen, Stefan,
>
> So, what do we want to do about the series? Do we have an agreement? Is
> the patch okay or I should make a change?
I think your current patch here should be merged as is, as it is removing
the mis-use of the trace infrastructure.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 4/4] net/colo: Match is-enabled probe to tracepoint
2020-07-29 12:34 ` Daniel P. Berrangé
@ 2020-08-05 10:53 ` Stefan Hajnoczi
2020-08-05 18:01 ` Zhang, Chen
0 siblings, 1 reply; 21+ messages in thread
From: Stefan Hajnoczi @ 2020-08-05 10:53 UTC (permalink / raw)
To: Zhang, Chen
Cc: Daniel Berrange, Li Zhijian, Jason Wang, qemu-devel,
Cameron Esfahani, Roman Bolshakov, Philippe Mathieu-Daudé
[-- Attachment #1: Type: text/plain, Size: 6100 bytes --]
On Wed, Jul 29, 2020 at 01:34:52PM +0100, Daniel P. Berrangé wrote:
> On Wed, Jul 29, 2020 at 03:33:22PM +0300, Roman Bolshakov wrote:
> > On Tue, Jul 21, 2020 at 03:06:57PM +0100, Daniel P. Berrangé wrote:
> > > On Sat, Jul 18, 2020 at 05:58:56PM +0000, Zhang, Chen wrote:
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Roman Bolshakov <r.bolshakov@yadro.com>
> > > > > Sent: Friday, July 17, 2020 5:35 PM
> > > > > To: qemu-devel@nongnu.org
> > > > > Cc: Daniel P. Berrangé <berrange@redhat.com>; Stefan Hajnoczi
> > > > > <stefanha@redhat.com>; Cameron Esfahani <dirty@apple.com>; Roman
> > > > > Bolshakov <r.bolshakov@yadro.com>; Philippe Mathieu-Daudé
> > > > > <philmd@redhat.com>; Zhang, Chen <chen.zhang@intel.com>; Li Zhijian
> > > > > <lizhijian@cn.fujitsu.com>; Jason Wang <jasowang@redhat.com>
> > > > > Subject: [PATCH v2 4/4] net/colo: Match is-enabled probe to tracepoint
> > > > >
> > > > > Build of QEMU with dtrace fails on macOS:
> > > > >
> > > > > LINK x86_64-softmmu/qemu-system-x86_64
> > > > > error: probe colo_compare_miscompare doesn't exist
> > > > > error: Could not register probes
> > > > > ld: error creating dtrace DOF section for architecture x86_64
> > > > >
> > > > > The reason of the error is explained by Adam Leventhal [1]:
> > > > >
> > > > > Note that is-enabled probes don't have the stability magic so I'm not
> > > > > sure how things would work if only is-enabled probes were used.
> > > > >
> > > > > net/colo code uses is-enabled probes to determine if other probes should be
> > > > > used but colo_compare_miscompare itself is not used explicitly.
> > > > > Linker doesn't include the symbol and build fails.
> > > > >
> > > > > The issue can be resolved if is-enabled probe matches the actual trace point
> > > > > that is used inside the test. Packet dump toggle is replaced with a compile-
> > > > > time conditional definition.
> > > > >
> > > > > 1. http://markmail.org/message/6grq2ygr5nwdwsnb
> > > > >
> > > > > Fixes: f4b618360e ("colo-compare: add TCP, UDP, ICMP packet comparison")
> > > > > Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
> > > > > Cc: Cameron Esfahani <dirty@apple.com>
> > > > > Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com>
> > > > > ---
> > > > > net/colo-compare.c | 42 ++++++++++++++++++++++--------------------
> > > > > net/filter-rewriter.c | 10 ++++++++--
> > > > > net/trace-events | 2 --
> > > > > 3 files changed, 30 insertions(+), 24 deletions(-)
> > >
> > >
> > > > > (trace_event_get_state_backends(TRACE_COLO_COMPARE_MISCOMPARE))
> > > > > {
> > > > > + if (trace_event_get_state_backends(TRACE_COLO_COMPARE_IP_INFO))
> > > > > {
> > > > > char pri_ip_src[20], pri_ip_dst[20], sec_ip_src[20], sec_ip_dst[20];
> > > > >
> > > > > strcpy(pri_ip_src, inet_ntoa(ppkt->ip->ip_src)); @@ -492,12 +494,12
> > > > > @@ sec:
> > > > > g_queue_push_head(&conn->primary_list, ppkt);
> > > > > g_queue_push_head(&conn->secondary_list, spkt);
> > > > >
> > > > > - if
> > > > > (trace_event_get_state_backends(TRACE_COLO_COMPARE_MISCOMPARE))
> > > > > {
> > > > > - qemu_hexdump((char *)ppkt->data, stderr,
> > > > > - "colo-compare ppkt", ppkt->size);
> > > > > - qemu_hexdump((char *)spkt->data, stderr,
> > > > > - "colo-compare spkt", spkt->size);
> > > > > - }
> > > > > +#ifdef DEBUG_COLO_PACKETS
> > > > > + qemu_hexdump((char *)ppkt->data, stderr,
> > > > > + "colo-compare ppkt", ppkt->size);
> > > > > + qemu_hexdump((char *)spkt->data, stderr,
> > > > > + "colo-compare spkt", spkt->size); #endif
> > > > >
> > > > > colo_compare_inconsistency_notify(s);
> > > > > }
> > > > > @@ -533,12 +535,12 @@ static int colo_packet_compare_udp(Packet *spkt,
> > > > > Packet *ppkt)
> > > > > ppkt->size - offset)) {
> > > > > trace_colo_compare_udp_miscompare("primary pkt size", ppkt->size);
> > > > > trace_colo_compare_udp_miscompare("Secondary pkt size", spkt-
> > > > > >size);
> > > > > - if
> > > > > (trace_event_get_state_backends(TRACE_COLO_COMPARE_MISCOMPARE))
> > > > > {
> > > > > - qemu_hexdump((char *)ppkt->data, stderr, "colo-compare pri pkt",
> > > > > - ppkt->size);
> > > > > - qemu_hexdump((char *)spkt->data, stderr, "colo-compare sec pkt",
> > > > > - spkt->size);
> > > > > - }
> > > > > +#ifdef DEBUG_COLO_PACKETS
> > > > > + qemu_hexdump((char *)ppkt->data, stderr, "colo-compare pri pkt",
> > > > > + ppkt->size);
> > > > > + qemu_hexdump((char *)spkt->data, stderr, "colo-compare sec pkt",
> > > > > + spkt->size);
> > > > > +#endif
> > > >
> > > > Hi Roman,
> > > >
> > > > I think change the " trace_event_get_state_backends()" to
> > > > "trace_colo_compare_main("Dump packet hex: ")" is a better choice here.
> > > > It will keep the original code logic and avoid the problem here.
> > >
> > > That may workaround the immediate bug, but this is still a misuse of the
> > > tracing code. Use of any trace point should only trigger actions in the
> > > trace infrastructure.
> > >
> > > If I'm using dtrace backend to monitor events I don't want to see QEMU
> > > dumping stuff to stderr. Anything written to stderr is going to trigger
> > > disk I/O writing to the VM's logfile, and is also liable to trigger rate
> > > limiting which can impact the guest performance.
> > >
> >
> > Hi Daniel, Chen, Stefan,
> >
> > So, what do we want to do about the series? Do we have an agreement? Is
> > the patch okay or I should make a change?
>
> I think your current patch here should be merged as is, as it is removing
> the mis-use of the trace infrastructure.
Hi Zhang Chen,
Do you agree?
Thanks,
Stefan
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 21+ messages in thread
* RE: [PATCH v2 4/4] net/colo: Match is-enabled probe to tracepoint
2020-08-05 10:53 ` Stefan Hajnoczi
@ 2020-08-05 18:01 ` Zhang, Chen
0 siblings, 0 replies; 21+ messages in thread
From: Zhang, Chen @ 2020-08-05 18:01 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: Daniel Berrange, Li Zhijian, Jason Wang, qemu-devel,
Cameron Esfahani, Roman Bolshakov, Philippe Mathieu-Daudé
> -----Original Message-----
> From: Stefan Hajnoczi <stefanha@redhat.com>
> Sent: Wednesday, August 5, 2020 6:53 PM
> To: Zhang, Chen <chen.zhang@intel.com>
> Cc: Roman Bolshakov <r.bolshakov@yadro.com>; Li Zhijian
> <lizhijian@cn.fujitsu.com>; Jason Wang <jasowang@redhat.com>; qemu-
> devel@nongnu.org; Cameron Esfahani <dirty@apple.com>; Philippe
> Mathieu-Daudé <philmd@redhat.com>; Daniel Berrange
> <berrange@redhat.com>
> Subject: Re: [PATCH v2 4/4] net/colo: Match is-enabled probe to tracepoint
>
> On Wed, Jul 29, 2020 at 01:34:52PM +0100, Daniel P. Berrangé wrote:
> > On Wed, Jul 29, 2020 at 03:33:22PM +0300, Roman Bolshakov wrote:
> > > On Tue, Jul 21, 2020 at 03:06:57PM +0100, Daniel P. Berrangé wrote:
> > > > On Sat, Jul 18, 2020 at 05:58:56PM +0000, Zhang, Chen wrote:
> > > > >
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Roman Bolshakov <r.bolshakov@yadro.com>
> > > > > > Sent: Friday, July 17, 2020 5:35 PM
> > > > > > To: qemu-devel@nongnu.org
> > > > > > Cc: Daniel P. Berrangé <berrange@redhat.com>; Stefan Hajnoczi
> > > > > > <stefanha@redhat.com>; Cameron Esfahani <dirty@apple.com>;
> > > > > > Roman Bolshakov <r.bolshakov@yadro.com>; Philippe
> > > > > > Mathieu-Daudé <philmd@redhat.com>; Zhang, Chen
> > > > > > <chen.zhang@intel.com>; Li Zhijian <lizhijian@cn.fujitsu.com>;
> > > > > > Jason Wang <jasowang@redhat.com>
> > > > > > Subject: [PATCH v2 4/4] net/colo: Match is-enabled probe to
> > > > > > tracepoint
> > > > > >
> > > > > > Build of QEMU with dtrace fails on macOS:
> > > > > >
> > > > > > LINK x86_64-softmmu/qemu-system-x86_64
> > > > > > error: probe colo_compare_miscompare doesn't exist
> > > > > > error: Could not register probes
> > > > > > ld: error creating dtrace DOF section for architecture x86_64
> > > > > >
> > > > > > The reason of the error is explained by Adam Leventhal [1]:
> > > > > >
> > > > > > Note that is-enabled probes don't have the stability magic so I'm
> not
> > > > > > sure how things would work if only is-enabled probes were used.
> > > > > >
> > > > > > net/colo code uses is-enabled probes to determine if other
> > > > > > probes should be used but colo_compare_miscompare itself is not
> used explicitly.
> > > > > > Linker doesn't include the symbol and build fails.
> > > > > >
> > > > > > The issue can be resolved if is-enabled probe matches the
> > > > > > actual trace point that is used inside the test. Packet dump
> > > > > > toggle is replaced with a compile- time conditional definition.
> > > > > >
> > > > > > 1. http://markmail.org/message/6grq2ygr5nwdwsnb
> > > > > >
> > > > > > Fixes: f4b618360e ("colo-compare: add TCP, UDP, ICMP packet
> > > > > > comparison")
> > > > > > Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
> > > > > > Cc: Cameron Esfahani <dirty@apple.com>
> > > > > > Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com>
> > > > > > ---
> > > > > > net/colo-compare.c | 42 ++++++++++++++++++++++---------------
> -----
> > > > > > net/filter-rewriter.c | 10 ++++++++--
> > > > > > net/trace-events | 2 --
> > > > > > 3 files changed, 30 insertions(+), 24 deletions(-)
> > > >
> > > >
> > > > > >
> (trace_event_get_state_backends(TRACE_COLO_COMPARE_MISCOMPARE)
> > > > > > )
> > > > > > {
> > > > > > + if
> > > > > > +
> (trace_event_get_state_backends(TRACE_COLO_COMPARE_IP_INFO))
> > > > > > {
> > > > > > char pri_ip_src[20], pri_ip_dst[20], sec_ip_src[20],
> > > > > > sec_ip_dst[20];
> > > > > >
> > > > > > strcpy(pri_ip_src, inet_ntoa(ppkt->ip->ip_src)); @@
> > > > > > -492,12 +494,12 @@ sec:
> > > > > > g_queue_push_head(&conn->primary_list, ppkt);
> > > > > > g_queue_push_head(&conn->secondary_list, spkt);
> > > > > >
> > > > > > - if
> > > > > >
> (trace_event_get_state_backends(TRACE_COLO_COMPARE_MISCOMPARE)
> > > > > > )
> > > > > > {
> > > > > > - qemu_hexdump((char *)ppkt->data, stderr,
> > > > > > - "colo-compare ppkt", ppkt->size);
> > > > > > - qemu_hexdump((char *)spkt->data, stderr,
> > > > > > - "colo-compare spkt", spkt->size);
> > > > > > - }
> > > > > > +#ifdef DEBUG_COLO_PACKETS
> > > > > > + qemu_hexdump((char *)ppkt->data, stderr,
> > > > > > + "colo-compare ppkt", ppkt->size);
> > > > > > + qemu_hexdump((char *)spkt->data, stderr,
> > > > > > + "colo-compare spkt", spkt->size); #endif
> > > > > >
> > > > > > colo_compare_inconsistency_notify(s);
> > > > > > }
> > > > > > @@ -533,12 +535,12 @@ static int
> > > > > > colo_packet_compare_udp(Packet *spkt, Packet *ppkt)
> > > > > > ppkt->size - offset)) {
> > > > > > trace_colo_compare_udp_miscompare("primary pkt size",
> ppkt->size);
> > > > > > trace_colo_compare_udp_miscompare("Secondary pkt
> > > > > > size", spkt-
> > > > > > >size);
> > > > > > - if
> > > > > >
> (trace_event_get_state_backends(TRACE_COLO_COMPARE_MISCOMPARE)
> > > > > > )
> > > > > > {
> > > > > > - qemu_hexdump((char *)ppkt->data, stderr, "colo-compare
> pri pkt",
> > > > > > - ppkt->size);
> > > > > > - qemu_hexdump((char *)spkt->data, stderr, "colo-compare
> sec pkt",
> > > > > > - spkt->size);
> > > > > > - }
> > > > > > +#ifdef DEBUG_COLO_PACKETS
> > > > > > + qemu_hexdump((char *)ppkt->data, stderr, "colo-compare pri
> pkt",
> > > > > > + ppkt->size);
> > > > > > + qemu_hexdump((char *)spkt->data, stderr, "colo-compare
> sec pkt",
> > > > > > + spkt->size); #endif
> > > > >
> > > > > Hi Roman,
> > > > >
> > > > > I think change the " trace_event_get_state_backends()" to
> > > > > "trace_colo_compare_main("Dump packet hex: ")" is a better choice
> here.
> > > > > It will keep the original code logic and avoid the problem here.
> > > >
> > > > That may workaround the immediate bug, but this is still a misuse
> > > > of the tracing code. Use of any trace point should only trigger
> > > > actions in the trace infrastructure.
> > > >
> > > > If I'm using dtrace backend to monitor events I don't want to see
> > > > QEMU dumping stuff to stderr. Anything written to stderr is going
> > > > to trigger disk I/O writing to the VM's logfile, and is also
> > > > liable to trigger rate limiting which can impact the guest performance.
> > > >
> > >
> > > Hi Daniel, Chen, Stefan,
> > >
> > > So, what do we want to do about the series? Do we have an agreement?
> > > Is the patch okay or I should make a change?
> >
> > I think your current patch here should be merged as is, as it is
> > removing the mis-use of the trace infrastructure.
>
> Hi Zhang Chen,
> Do you agree?
It's OK for me.
Reviewed-by: Zhang Chen <chen.zhang@intel.com>
Thanks
Zhang Chen
>
> Thanks,
> Stefan
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 0/4] Add dtrace support on macOS
2020-07-17 9:35 [PATCH v2 0/4] Add dtrace support on macOS Roman Bolshakov
` (4 preceding siblings ...)
2020-07-21 12:36 ` [PATCH v2 0/4] Add dtrace support on macOS Stefan Hajnoczi
@ 2020-08-07 13:00 ` Stefan Hajnoczi
5 siblings, 0 replies; 21+ messages in thread
From: Stefan Hajnoczi @ 2020-08-07 13:00 UTC (permalink / raw)
To: Roman Bolshakov; +Cc: Daniel P. Berrangé, qemu-devel, Cameron Esfahani
[-- Attachment #1: Type: text/plain, Size: 1384 bytes --]
On Fri, Jul 17, 2020 at 12:35:13PM +0300, Roman Bolshakov wrote:
> Hi,
>
> This is a small series that enables dtrace tracing backend on macOS.
> Whether or not it should go to 5.1 is up to discretion of tracing
> maintainers.
>
> Thanks,
> Roman
>
> Changes since v1:
> - Fixed a typo ANSI C to C99, wrt to _Bool in the first patch.
> - Prevented a few [-Wpointer-sign] warnings by converting int8_t * to
> signed char * in static probe definitions.
> - Moved COLO packet dump under #ifdef DEBUG_COLO_PACKETS (Daniel).
> - Separated tracepoints in net/filter-rewriter.c to use matching
> is-enabled probe (Daniel).
>
> Roman Bolshakov (4):
> scripts/tracetool: Fix dtrace generation for macOS
> scripts/tracetool: Use void pointer for vcpu
> build: Don't make object files for dtrace on macOS
> net/colo: Match is-enabled probe to tracepoint
>
> Makefile.objs | 2 ++
> net/colo-compare.c | 42 ++++++++++++++++++-----------------
> net/filter-rewriter.c | 10 +++++++--
> net/trace-events | 2 --
> scripts/tracetool/format/d.py | 15 ++++++++++++-
> scripts/tracetool/vcpu.py | 2 +-
> 6 files changed, 47 insertions(+), 26 deletions(-)
>
> --
> 2.26.1
>
Thanks, applied to my tracing-next tree:
https://github.com/stefanha/qemu/commits/tracing-next
Stefan
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2020-08-07 13:01 UTC | newest]
Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-17 9:35 [PATCH v2 0/4] Add dtrace support on macOS Roman Bolshakov
2020-07-17 9:35 ` [PATCH v2 1/4] scripts/tracetool: Fix dtrace generation for macOS Roman Bolshakov
2020-07-19 13:52 ` Philippe Mathieu-Daudé
2020-07-20 10:50 ` Roman Bolshakov
2020-07-20 10:53 ` Philippe Mathieu-Daudé
2020-07-21 12:33 ` Stefan Hajnoczi
2020-07-17 9:35 ` [PATCH v2 2/4] scripts/tracetool: Use void pointer for vcpu Roman Bolshakov
2020-07-21 12:34 ` Stefan Hajnoczi
2020-07-17 9:35 ` [PATCH v2 3/4] build: Don't make object files for dtrace on macOS Roman Bolshakov
2020-07-21 12:34 ` Stefan Hajnoczi
2020-07-24 17:00 ` Roman Bolshakov
2020-07-17 9:35 ` [PATCH v2 4/4] net/colo: Match is-enabled probe to tracepoint Roman Bolshakov
2020-07-18 17:58 ` Zhang, Chen
2020-07-20 10:59 ` Roman Bolshakov
2020-07-21 14:06 ` Daniel P. Berrangé
2020-07-29 12:33 ` Roman Bolshakov
2020-07-29 12:34 ` Daniel P. Berrangé
2020-08-05 10:53 ` Stefan Hajnoczi
2020-08-05 18:01 ` Zhang, Chen
2020-07-21 12:36 ` [PATCH v2 0/4] Add dtrace support on macOS Stefan Hajnoczi
2020-08-07 13:00 ` Stefan Hajnoczi
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.