From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:45860) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zguql-000723-SX for qemu-devel@nongnu.org; Tue, 29 Sep 2015 09:21:47 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Zguqg-0006dC-47 for qemu-devel@nongnu.org; Tue, 29 Sep 2015 09:21:43 -0400 Received: from lhrrgout.huawei.com ([194.213.3.17]:52473) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zguqf-0006c7-Rh for qemu-devel@nongnu.org; Tue, 29 Sep 2015 09:21:38 -0400 References: <1443094669-4144-1-git-send-email-marcandre.lureau@redhat.com> <1443094669-4144-18-git-send-email-marcandre.lureau@redhat.com> <560A8B89.1070808@huawei.com> <379496541.19670855.1443532361334.JavaMail.zimbra@redhat.com> From: Claudio Fontana Message-ID: <560A905B.6060900@huawei.com> Date: Tue, 29 Sep 2015 15:21:31 +0200 MIME-Version: 1.0 In-Reply-To: <379496541.19670855.1443532361334.JavaMail.zimbra@redhat.com> Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCH v4 17/47] ivshmem: improve debug messages List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?UTF-8?Q?Marc-Andr=c3=a9_Lureau?= Cc: drjones@redhat.com, qemu-devel@nongnu.org, stefanha@redhat.com, pbonzini@redhat.com, marcandre lureau , cam@cs.ualberta.ca On 29.09.2015 15:12, Marc-André Lureau wrote: > > > ----- Original Message ----- >> On 24.09.2015 13:37, marcandre.lureau@redhat.com wrote: >>> From: Marc-André Lureau >>> >>> Some misc improvements to ivshmem debug. >>> >>> Signed-off-by: Marc-André Lureau >>> --- >>> hw/misc/ivshmem.c | 10 +++++++--- >>> 1 file changed, 7 insertions(+), 3 deletions(-) >>> >>> diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c >>> index c4c130d..71a71fc 100644 >>> --- a/hw/misc/ivshmem.c >>> +++ b/hw/misc/ivshmem.c >>> @@ -208,10 +208,13 @@ static void ivshmem_io_write(void *opaque, hwaddr >>> addr, >>> if (vector < s->peers[dest].nb_eventfds) { >>> IVSHMEM_DPRINTF("Notifying VM %d on vector %d\n", dest, >>> vector); >>> event_notifier_set(&s->peers[dest].eventfds[vector]); >>> + } else { >>> + IVSHMEM_DPRINTF("Invalid destination vector %d on VM >>> %d\n", >>> + vector, dest); >>> } >>> break; >>> default: >>> - IVSHMEM_DPRINTF("Invalid VM Doorbell VM %d\n", dest); >>> + IVSHMEM_DPRINTF("Unhandled write " TARGET_FMT_plx "\n", addr); >>> } >>> } >>> >>> @@ -263,9 +266,9 @@ static void ivshmem_receive(void *opaque, const uint8_t >>> *buf, int size) >>> { >>> IVShmemState *s = opaque; >>> >>> - ivshmem_IntrStatus_write(s, *buf); >>> + IVSHMEM_DPRINTF("ivshmem_receive 0x%02x size: %d\n", *buf, size); >>> >>> - IVSHMEM_DPRINTF("ivshmem_receive 0x%02x\n", *buf); >>> + ivshmem_IntrStatus_write(s, *buf); >>> } >> >> I may understand how this went here, so these debug messages are clearly >> specific about this particular function. >> here you have "ivshmem_receive". Do you want to put () to help someone >> getting these messages after enabling debug to understand that he should >> look up this function? > > sure, notice it was there before without () > >> In the other messages though you (and the previous code) have been less >> function-specific in the message. > > imho this kind of debug messages should just give enough context to understand and locate them. It's not important whether or not they have the function name. > > >>> >>> static int ivshmem_can_receive(void * opaque) >>> @@ -592,6 +595,7 @@ static void ivshmem_use_msix(IVShmemState * s) >>> PCIDevice *d = PCI_DEVICE(s); >>> int i; >>> >>> + IVSHMEM_DPRINTF("use msix, present: %d\n", msix_present(d)); >>> if (!msix_present(d)) { >>> return; >>> } >> >> hmm do you want to say "ivshmem_use_msix(): present: true/false?" >> Or do you want to say something more generic about MSI-X in this case? >> >> As is the comment you have: >> >> 1) if it is specific to this function behavior, it does not help to look up >> the correct function > > It's actually easy to locate with a grep "use msix" in ivshmem code (the macro prepend IVHSMEM:) > >> 2) if it is a more generic comment, it does not help to understand what is >> going on >> >> Can you explain more what the purpose of that debug statement is? > > It helps to trace what's going on, at least it helped me. But if it's superflous, we can remove it (though it's really only there to help developers) > Ok lets be nice to them and say ivshmem_use_msix in there, so they find stuff quickly? $ time fgrep -R "use msix" * /.../ real 0m27.846s user 0m0.923s sys 0m0.667s $ time gid ivshmem_use_msix /.../ real 0m0.003s user 0m0.000s sys 0m0.000s