* [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
* 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
* [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
* 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
* [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
* 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
* [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 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
* 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
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 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.