* [PATCH 0/4] Add dtrace support on macOS
@ 2020-07-16 8:17 Roman Bolshakov
2020-07-16 8:17 ` [PATCH 1/4] scripts/tracetool: Fix dtrace generation for macOS Roman Bolshakov
` (4 more replies)
0 siblings, 5 replies; 11+ messages in thread
From: Roman Bolshakov @ 2020-07-16 8:17 UTC (permalink / raw)
To: qemu-devel
Cc: Philippe Mathieu-Daudé, Roman Bolshakov, Daniel P. Berrangé
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
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 | 12 +++++++-----
net/filter-rewriter.c | 8 ++++++--
net/trace-events | 2 --
scripts/tracetool/format/d.py | 9 ++++++++-
scripts/tracetool/vcpu.py | 2 +-
6 files changed, 24 insertions(+), 11 deletions(-)
--
2.26.1
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 1/4] scripts/tracetool: Fix dtrace generation for macOS
2020-07-16 8:17 [PATCH 0/4] Add dtrace support on macOS Roman Bolshakov
@ 2020-07-16 8:17 ` Roman Bolshakov
2020-07-16 8:56 ` Daniel P. Berrangé
2020-07-16 8:17 ` [PATCH 2/4] scripts/tracetool: Use void pointer for vcpu Roman Bolshakov
` (3 subsequent siblings)
4 siblings, 1 reply; 11+ messages in thread
From: Roman Bolshakov @ 2020-07-16 8:17 UTC (permalink / raw)
To: qemu-devel
Cc: Philippe Mathieu-Daudé,
Roman Bolshakov, Daniel P. Berrangé,
Stefan Hajnoczi, Cameron Esfahani
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 ANSI C _Bool.
Cc: Cameron Esfahani <dirty@apple.com>
Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com>
---
scripts/tracetool/format/d.py | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/scripts/tracetool/format/d.py b/scripts/tracetool/format/d.py
index 0afb5f3f47..be4a2aa254 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,11 @@ def generate(events, backend, group):
for e in events:
args = []
for type_, name in e.args:
+ if platform == "darwin":
+ if type_ == 'bool':
+ type_ = '_Bool'
+ if type_ == 'bool *':
+ type_ = '_Bool *'
if name in RESERVED_WORDS:
name += '_'
args.append(type_ + ' ' + name)
--
2.26.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 2/4] scripts/tracetool: Use void pointer for vcpu
2020-07-16 8:17 [PATCH 0/4] Add dtrace support on macOS Roman Bolshakov
2020-07-16 8:17 ` [PATCH 1/4] scripts/tracetool: Fix dtrace generation for macOS Roman Bolshakov
@ 2020-07-16 8:17 ` Roman Bolshakov
2020-07-16 8:57 ` Daniel P. Berrangé
2020-07-16 8:17 ` [PATCH 3/4] build: Don't make object files for dtrace on macOS Roman Bolshakov
` (2 subsequent siblings)
4 siblings, 1 reply; 11+ messages in thread
From: Roman Bolshakov @ 2020-07-16 8:17 UTC (permalink / raw)
To: qemu-devel
Cc: Philippe Mathieu-Daudé,
Roman Bolshakov, Daniel P. Berrangé,
Stefan Hajnoczi, Cameron Esfahani
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")
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] 11+ messages in thread
* [PATCH 3/4] build: Don't make object files for dtrace on macOS
2020-07-16 8:17 [PATCH 0/4] Add dtrace support on macOS Roman Bolshakov
2020-07-16 8:17 ` [PATCH 1/4] scripts/tracetool: Fix dtrace generation for macOS Roman Bolshakov
2020-07-16 8:17 ` [PATCH 2/4] scripts/tracetool: Use void pointer for vcpu Roman Bolshakov
@ 2020-07-16 8:17 ` Roman Bolshakov
2020-07-16 8:54 ` Daniel P. Berrangé
2020-07-16 8:17 ` [PATCH 4/4] net/colo: Match is-enabled probe to tracepoint Roman Bolshakov
2020-07-16 8:55 ` [PATCH 0/4] Add dtrace support on macOS Daniel P. Berrangé
4 siblings, 1 reply; 11+ messages in thread
From: Roman Bolshakov @ 2020-07-16 8:17 UTC (permalink / raw)
To: qemu-devel
Cc: Philippe Mathieu-Daudé,
Roman Bolshakov, Daniel P. Berrangé,
Cameron Esfahani
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
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] 11+ messages in thread
* [PATCH 4/4] net/colo: Match is-enabled probe to tracepoint
2020-07-16 8:17 [PATCH 0/4] Add dtrace support on macOS Roman Bolshakov
` (2 preceding siblings ...)
2020-07-16 8:17 ` [PATCH 3/4] build: Don't make object files for dtrace on macOS Roman Bolshakov
@ 2020-07-16 8:17 ` Roman Bolshakov
2020-07-16 8:51 ` Daniel P. Berrangé
2020-07-16 8:55 ` [PATCH 0/4] Add dtrace support on macOS Daniel P. Berrangé
4 siblings, 1 reply; 11+ messages in thread
From: Roman Bolshakov @ 2020-07-16 8:17 UTC (permalink / raw)
To: qemu-devel
Cc: Daniel P. Berrangé,
Li Zhijian, Jason Wang, Cameron Esfahani, Zhang Chen,
Roman Bolshakov, 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.
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 | 12 +++++++-----
net/filter-rewriter.c | 8 ++++++--
net/trace-events | 2 --
3 files changed, 13 insertions(+), 9 deletions(-)
diff --git a/net/colo-compare.c b/net/colo-compare.c
index 398b7546ff..9525ed703b 100644
--- a/net/colo-compare.c
+++ b/net/colo-compare.c
@@ -327,7 +327,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,7 +492,7 @@ 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)) {
+ if (trace_event_get_state_backends(TRACE_COLO_COMPARE_TCP_INFO)) {
qemu_hexdump((char *)ppkt->data, stderr,
"colo-compare ppkt", ppkt->size);
qemu_hexdump((char *)spkt->data, stderr,
@@ -533,7 +533,8 @@ 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)) {
+ if (trace_event_get_state_backends(
+ TRACE_COLO_COMPARE_UDP_MISCOMPARE)) {
qemu_hexdump((char *)ppkt->data, stderr, "colo-compare pri pkt",
ppkt->size);
qemu_hexdump((char *)spkt->data, stderr, "colo-compare sec pkt",
@@ -576,7 +577,8 @@ 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)) {
+ if (trace_event_get_state_backends(
+ TRACE_COLO_COMPARE_ICMP_MISCOMPARE)) {
qemu_hexdump((char *)ppkt->data, stderr, "colo-compare pri pkt",
ppkt->size);
qemu_hexdump((char *)spkt->data, stderr, "colo-compare sec pkt",
@@ -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..ff04165cc4 100644
--- a/net/filter-rewriter.c
+++ b/net/filter-rewriter.c
@@ -76,7 +76,9 @@ 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_event_get_state_backends(
+ TRACE_COLO_FILTER_REWRITER_CONN_OFFSET)) {
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),
@@ -180,7 +182,9 @@ 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_event_get_state_backends(
+ TRACE_COLO_FILTER_REWRITER_CONN_OFFSET)) {
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),
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] 11+ messages in thread
* Re: [PATCH 4/4] net/colo: Match is-enabled probe to tracepoint
2020-07-16 8:17 ` [PATCH 4/4] net/colo: Match is-enabled probe to tracepoint Roman Bolshakov
@ 2020-07-16 8:51 ` Daniel P. Berrangé
2020-07-16 10:55 ` Roman Bolshakov
0 siblings, 1 reply; 11+ messages in thread
From: Daniel P. Berrangé @ 2020-07-16 8:51 UTC (permalink / raw)
To: Roman Bolshakov
Cc: Li Zhijian, Jason Wang, qemu-devel, Cameron Esfahani, Zhang Chen,
Stefan Hajnoczi, Philippe Mathieu-Daudé
Adding Stefan to CC as the trace maintainer.
On Thu, Jul 16, 2020 at 11:17:54AM +0300, Roman Bolshakov wrote:
> 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.
>
> 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 | 12 +++++++-----
> net/filter-rewriter.c | 8 ++++++--
> net/trace-events | 2 --
> 3 files changed, 13 insertions(+), 9 deletions(-)
>
> diff --git a/net/colo-compare.c b/net/colo-compare.c
> index 398b7546ff..9525ed703b 100644
> --- a/net/colo-compare.c
> +++ b/net/colo-compare.c
> @@ -327,7 +327,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,7 +492,7 @@ 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)) {
> + if (trace_event_get_state_backends(TRACE_COLO_COMPARE_TCP_INFO)) {
> qemu_hexdump((char *)ppkt->data, stderr,
> "colo-compare ppkt", ppkt->size);
> qemu_hexdump((char *)spkt->data, stderr,
Not your fault, as this problem is pre-existing, but IMHO this block of code
is simply broken by design. It is checking a trace event enablement state,
and then not emitting any trace event, but instead dumping info to stderr.
This is mis-use of the trace framework, and changing the event name fixes
your immediate macOS bug but the code is still flawed.
Basically it is using the trace framework as a way to dynamically turn on /
off general debugging information.
I'm not quite sure what todo to fix it, but I don't think it should be wrapped
in any trace_event_get_state_backends() call at all.
The simplest immediate option I think is to change it to be a compile time
debug
// #define DEBUG_COLO_PACKETS
and then use
#ifdef DEBUG_COLO_PACKETS
qemu_hexdump(...)
#endif
and then leave it upto the maintainer to come up with a more advanced
solution if they want to make it runtime configurable again, but not
abusing the trace framework.
> @@ -533,7 +533,8 @@ 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)) {
> + if (trace_event_get_state_backends(
> + TRACE_COLO_COMPARE_UDP_MISCOMPARE)) {
> qemu_hexdump((char *)ppkt->data, stderr, "colo-compare pri pkt",
> ppkt->size);
> qemu_hexdump((char *)spkt->data, stderr, "colo-compare sec pkt",
Same brokenness
> @@ -576,7 +577,8 @@ 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)) {
> + if (trace_event_get_state_backends(
> + TRACE_COLO_COMPARE_ICMP_MISCOMPARE)) {
> qemu_hexdump((char *)ppkt->data, stderr, "colo-compare pri pkt",
> ppkt->size);
> qemu_hexdump((char *)spkt->data, stderr, "colo-compare sec pkt",
Same brokenness
> @@ -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..ff04165cc4 100644
> --- a/net/filter-rewriter.c
> +++ b/net/filter-rewriter.c
> @@ -76,7 +76,9 @@ 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_event_get_state_backends(
> + TRACE_COLO_FILTER_REWRITER_CONN_OFFSET)) {
> 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),
Since there are two separate trace events here, each should be wrapped with
its own check. IOW, two completely separate if (...) trace(..); blocks
> @@ -180,7 +182,9 @@ 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_event_get_state_backends(
> + TRACE_COLO_FILTER_REWRITER_CONN_OFFSET)) {
> 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),
Same here.
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] 11+ messages in thread
* Re: [PATCH 3/4] build: Don't make object files for dtrace on macOS
2020-07-16 8:17 ` [PATCH 3/4] build: Don't make object files for dtrace on macOS Roman Bolshakov
@ 2020-07-16 8:54 ` Daniel P. Berrangé
0 siblings, 0 replies; 11+ messages in thread
From: Daniel P. Berrangé @ 2020-07-16 8:54 UTC (permalink / raw)
To: Roman Bolshakov; +Cc: Philippe Mathieu-Daudé, qemu-devel, Cameron Esfahani
On Thu, Jul 16, 2020 at 11:17:53AM +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
>
> 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: Daniel P. Berrangé <berrange@redhat.com>
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] 11+ messages in thread
* Re: [PATCH 0/4] Add dtrace support on macOS
2020-07-16 8:17 [PATCH 0/4] Add dtrace support on macOS Roman Bolshakov
` (3 preceding siblings ...)
2020-07-16 8:17 ` [PATCH 4/4] net/colo: Match is-enabled probe to tracepoint Roman Bolshakov
@ 2020-07-16 8:55 ` Daniel P. Berrangé
4 siblings, 0 replies; 11+ messages in thread
From: Daniel P. Berrangé @ 2020-07-16 8:55 UTC (permalink / raw)
To: Roman Bolshakov; +Cc: Philippe Mathieu-Daudé, qemu-devel, Stefan Hajnoczi
Adding Stefan as the trace maintainer.
On Thu, Jul 16, 2020 at 11:17:50AM +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
>
> 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 | 12 +++++++-----
> net/filter-rewriter.c | 8 ++++++--
> net/trace-events | 2 --
> scripts/tracetool/format/d.py | 9 ++++++++-
> scripts/tracetool/vcpu.py | 2 +-
> 6 files changed, 24 insertions(+), 11 deletions(-)
>
> --
> 2.26.1
>
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] 11+ messages in thread
* Re: [PATCH 1/4] scripts/tracetool: Fix dtrace generation for macOS
2020-07-16 8:17 ` [PATCH 1/4] scripts/tracetool: Fix dtrace generation for macOS Roman Bolshakov
@ 2020-07-16 8:56 ` Daniel P. Berrangé
0 siblings, 0 replies; 11+ messages in thread
From: Daniel P. Berrangé @ 2020-07-16 8:56 UTC (permalink / raw)
To: Roman Bolshakov
Cc: Stefan Hajnoczi, Philippe Mathieu-Daudé,
qemu-devel, Cameron Esfahani
On Thu, Jul 16, 2020 at 11:17:51AM +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 ANSI C _Bool.
>
> Cc: Cameron Esfahani <dirty@apple.com>
> Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com>
> ---
> scripts/tracetool/format/d.py | 9 ++++++++-
> 1 file changed, 8 insertions(+), 1 deletion(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
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] 11+ messages in thread
* Re: [PATCH 2/4] scripts/tracetool: Use void pointer for vcpu
2020-07-16 8:17 ` [PATCH 2/4] scripts/tracetool: Use void pointer for vcpu Roman Bolshakov
@ 2020-07-16 8:57 ` Daniel P. Berrangé
0 siblings, 0 replies; 11+ messages in thread
From: Daniel P. Berrangé @ 2020-07-16 8:57 UTC (permalink / raw)
To: Roman Bolshakov
Cc: Stefan Hajnoczi, Philippe Mathieu-Daudé,
qemu-devel, Cameron Esfahani
On Thu, Jul 16, 2020 at 11:17:52AM +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")
> 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: Daniel P. Berrangé <berrange@redhat.com>
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] 11+ messages in thread
* Re: [PATCH 4/4] net/colo: Match is-enabled probe to tracepoint
2020-07-16 8:51 ` Daniel P. Berrangé
@ 2020-07-16 10:55 ` Roman Bolshakov
0 siblings, 0 replies; 11+ messages in thread
From: Roman Bolshakov @ 2020-07-16 10:55 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: Li Zhijian, Jason Wang, qemu-devel, Cameron Esfahani, Zhang Chen,
Stefan Hajnoczi, Philippe Mathieu-Daudé
On Thu, Jul 16, 2020 at 09:51:27AM +0100, Daniel P. Berrangé wrote:
> Adding Stefan to CC as the trace maintainer.
>
> On Thu, Jul 16, 2020 at 11:17:54AM +0300, Roman Bolshakov wrote:
> > 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.
> >
> > 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 | 12 +++++++-----
> > net/filter-rewriter.c | 8 ++++++--
> > net/trace-events | 2 --
> > 3 files changed, 13 insertions(+), 9 deletions(-)
> >
> > diff --git a/net/colo-compare.c b/net/colo-compare.c
> > index 398b7546ff..9525ed703b 100644
> > --- a/net/colo-compare.c
> > +++ b/net/colo-compare.c
> > @@ -327,7 +327,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,7 +492,7 @@ 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)) {
> > + if (trace_event_get_state_backends(TRACE_COLO_COMPARE_TCP_INFO)) {
> > qemu_hexdump((char *)ppkt->data, stderr,
> > "colo-compare ppkt", ppkt->size);
> > qemu_hexdump((char *)spkt->data, stderr,
>
> Not your fault, as this problem is pre-existing, but IMHO this block of code
> is simply broken by design. It is checking a trace event enablement state,
> and then not emitting any trace event, but instead dumping info to stderr.
> This is mis-use of the trace framework, and changing the event name fixes
> your immediate macOS bug but the code is still flawed.
>
> Basically it is using the trace framework as a way to dynamically turn on /
> off general debugging information.
>
> I'm not quite sure what todo to fix it, but I don't think it should be wrapped
> in any trace_event_get_state_backends() call at all.
>
> The simplest immediate option I think is to change it to be a compile time
> debug
>
> // #define DEBUG_COLO_PACKETS
>
> and then use
>
> #ifdef DEBUG_COLO_PACKETS
> qemu_hexdump(...)
> #endif
>
> and then leave it upto the maintainer to come up with a more advanced
> solution if they want to make it runtime configurable again, but not
> abusing the trace framework.
>
Hi Daniel,
It makes sense, compile-time define works better because the trace data
doesn't go into a trace point. I'll use that approach in v2.
> > @@ -533,7 +533,8 @@ 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)) {
> > + if (trace_event_get_state_backends(
> > + TRACE_COLO_COMPARE_UDP_MISCOMPARE)) {
> > qemu_hexdump((char *)ppkt->data, stderr, "colo-compare pri pkt",
> > ppkt->size);
> > qemu_hexdump((char *)spkt->data, stderr, "colo-compare sec pkt",
>
> Same brokenness
>
> > @@ -576,7 +577,8 @@ 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)) {
> > + if (trace_event_get_state_backends(
> > + TRACE_COLO_COMPARE_ICMP_MISCOMPARE)) {
> > qemu_hexdump((char *)ppkt->data, stderr, "colo-compare pri pkt",
> > ppkt->size);
> > qemu_hexdump((char *)spkt->data, stderr, "colo-compare sec pkt",
>
> Same brokenness
>
> > @@ -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..ff04165cc4 100644
> > --- a/net/filter-rewriter.c
> > +++ b/net/filter-rewriter.c
> > @@ -76,7 +76,9 @@ 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_event_get_state_backends(
> > + TRACE_COLO_FILTER_REWRITER_CONN_OFFSET)) {
> > 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),
>
> Since there are two separate trace events here, each should be wrapped with
> its own check. IOW, two completely separate if (...) trace(..); blocks
>
Agreed, I'll do that, thanks!
> > @@ -180,7 +182,9 @@ 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_event_get_state_backends(
> > + TRACE_COLO_FILTER_REWRITER_CONN_OFFSET)) {
> > 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),
>
> Same here.
>
>
Best regards,
Roman
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2020-07-16 10:56 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-16 8:17 [PATCH 0/4] Add dtrace support on macOS Roman Bolshakov
2020-07-16 8:17 ` [PATCH 1/4] scripts/tracetool: Fix dtrace generation for macOS Roman Bolshakov
2020-07-16 8:56 ` Daniel P. Berrangé
2020-07-16 8:17 ` [PATCH 2/4] scripts/tracetool: Use void pointer for vcpu Roman Bolshakov
2020-07-16 8:57 ` Daniel P. Berrangé
2020-07-16 8:17 ` [PATCH 3/4] build: Don't make object files for dtrace on macOS Roman Bolshakov
2020-07-16 8:54 ` Daniel P. Berrangé
2020-07-16 8:17 ` [PATCH 4/4] net/colo: Match is-enabled probe to tracepoint Roman Bolshakov
2020-07-16 8:51 ` Daniel P. Berrangé
2020-07-16 10:55 ` Roman Bolshakov
2020-07-16 8:55 ` [PATCH 0/4] Add dtrace support on macOS Daniel P. Berrangé
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).