[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2 4/4] net/colo: Match is-enabled probe to tracepoint
From: |
Daniel P . Berrangé |
Subject: |
Re: [PATCH v2 4/4] net/colo: Match is-enabled probe to tracepoint |
Date: |
Wed, 29 Jul 2020 13:34:52 +0100 |
User-agent: |
Mutt/1.14.5 (2020-06-23) |
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
--
|: 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 :|
Re: [PATCH v2 0/4] Add dtrace support on macOS, Stefan Hajnoczi, 2020/07/21