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

["Zhang, Chen" <chen.zhang@intel.com>]

> -----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