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

[Stefan Hajnoczi]

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

signature.asc
Description: PGP signature