diff mbox series

[v2,4/4] net/colo: Match is-enabled probe to tracepoint

Message ID 20200717093517.73397-5-r.bolshakov@yadro.com (mailing list archive)
State New, archived
Headers show
Series Add dtrace support on macOS | expand

Commit Message

Roman Bolshakov July 17, 2020, 9:35 a.m. UTC
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(-)

Comments

Zhang, Chen July 18, 2020, 5:58 p.m. UTC | #1
> -----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
Roman Bolshakov July 20, 2020, 10:59 a.m. UTC | #2
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
Daniel P. Berrangé July 21, 2020, 2:06 p.m. UTC | #3
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
Roman Bolshakov July 29, 2020, 12:33 p.m. UTC | #4
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
Daniel P. Berrangé July 29, 2020, 12:34 p.m. UTC | #5
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
Stefan Hajnoczi Aug. 5, 2020, 10:53 a.m. UTC | #6
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
Zhang, Chen Aug. 5, 2020, 6:01 p.m. UTC | #7
> -----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
diff mbox series

Patch

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"