From: "Xia, Chenbo" <chenbo.xia@intel.com>
To: Maxime Coquelin <maxime.coquelin@redhat.com>,
"dev@dpdk.org" <dev@dpdk.org>,
"david.marchand@redhat.com" <david.marchand@redhat.com>
Subject: RE: [PATCH v2 7/9] vhost: remove multi-line logs
Date: Wed, 26 Jan 2022 03:27:01 +0000 [thread overview]
Message-ID: <SN6PR11MB3504B5FF1D1E3FC304D8E2D09C209@SN6PR11MB3504.namprd11.prod.outlook.com> (raw)
In-Reply-To: <20220125112457.166434-8-maxime.coquelin@redhat.com>
Hi Maxime,
> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Tuesday, January 25, 2022 7:25 PM
> To: dev@dpdk.org; Xia, Chenbo <chenbo.xia@intel.com>;
> david.marchand@redhat.com
> Cc: Maxime Coquelin <maxime.coquelin@redhat.com>
> Subject: [PATCH v2 7/9] vhost: remove multi-line logs
>
> This patch replaces multi-lines logs in multiple single-
> line logs in order to ease logs filtering based on their
> socket path.
>
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> ---
> lib/vhost/socket.c | 10 ++--
> lib/vhost/vhost.c | 8 ++--
> lib/vhost/vhost_user.c | 106 +++++++++++++++++++----------------------
> 3 files changed, 60 insertions(+), 64 deletions(-)
>
> diff --git a/lib/vhost/socket.c b/lib/vhost/socket.c
> index ad3471d6a9..c2f8013cd5 100644
> --- a/lib/vhost/socket.c
> +++ b/lib/vhost/socket.c
> @@ -868,8 +868,8 @@ rte_vhost_driver_register(const char *path, uint64_t flags)
> if (vsocket->async_copy &&
> (flags & (RTE_VHOST_USER_IOMMU_SUPPORT |
> RTE_VHOST_USER_POSTCOPY_SUPPORT))) {
> - VHOST_LOG_CONFIG(ERR, "(%s) enabling async copy and IOMMU "
> - "or post-copy feature simultaneously is not supported\n",
> path);
> + VHOST_LOG_CONFIG(ERR, "(%s) async copy with IOMMU or post-copy not
> supported\n",
> + path);
> goto out_mutex;
> }
>
> @@ -908,8 +908,10 @@ rte_vhost_driver_register(const char *path, uint64_t
> flags)
> (1ULL << VIRTIO_NET_F_HOST_TSO6) |
> (1ULL << VIRTIO_NET_F_HOST_UFO);
>
> - VHOST_LOG_CONFIG(INFO, "(%s) Linear buffers requested without
> external buffers, "
> - "disabling host segmentation offloading support\n", path);
> + VHOST_LOG_CONFIG(INFO, "(%s) Linear buffers requested without
> external buffers,\n",
> + path);
> + VHOST_LOG_CONFIG(INFO, "(%s) disabling host segmentation
> offloading support\n",
> + path);
> vsocket->supported_features &= ~seg_offload_features;
> vsocket->features &= ~seg_offload_features;
> }
> diff --git a/lib/vhost/vhost.c b/lib/vhost/vhost.c
> index 3b05f17a50..cd62dc238b 100644
> --- a/lib/vhost/vhost.c
> +++ b/lib/vhost/vhost.c
> @@ -1786,8 +1786,8 @@ rte_vhost_async_channel_unregister(int vid, uint16_t
> queue_id)
> }
>
> if (vq->async->pkts_inflight_n) {
> - VHOST_LOG_CONFIG(ERR, "(%s) failed to unregister async channel. "
> - "async inflight packets must be completed before
> unregistration.\n",
> + VHOST_LOG_CONFIG(ERR, "(%s) failed to unregister async channel.\n",
> dev->ifname);
> + VHOST_LOG_CONFIG(ERR, "(%s) inflight packets must be completed
> before unregistration.\n",
> dev->ifname);
> ret = -1;
> goto out;
> @@ -1821,8 +1821,8 @@ rte_vhost_async_channel_unregister_thread_unsafe(int vid,
> uint16_t queue_id)
> return 0;
>
> if (vq->async->pkts_inflight_n) {
> - VHOST_LOG_CONFIG(ERR, "(%s) failed to unregister async channel. "
> - "async inflight packets must be completed before
> unregistration.\n",
> + VHOST_LOG_CONFIG(ERR, "(%s) failed to unregister async channel.\n",
> dev->ifname);
> + VHOST_LOG_CONFIG(ERR, "(%s) inflight packets must be completed
> before unregistration.\n",
> dev->ifname);
> return -1;
> }
> diff --git a/lib/vhost/vhost_user.c b/lib/vhost/vhost_user.c
> index e207ace426..3a3f9723cc 100644
> --- a/lib/vhost/vhost_user.c
> +++ b/lib/vhost/vhost_user.c
> @@ -872,13 +872,13 @@ translate_ring_addresses(struct virtio_net *dev, int
> vq_index)
> }
>
> if (vq->last_used_idx != vq->used->idx) {
> - VHOST_LOG_CONFIG(WARNING,
> - "(%s) last_used_idx (%u) and vq->used->idx (%u) mismatches;
> "
> - "some packets maybe resent for Tx and dropped for Rx\n",
> + VHOST_LOG_CONFIG(WARNING, "(%s) last_used_idx (%u) and vq->used-
> >idx (%u) mismatches;\n",
> dev->ifname,
> vq->last_used_idx, vq->used->idx);
> vq->last_used_idx = vq->used->idx;
> vq->last_avail_idx = vq->used->idx;
> + VHOST_LOG_CONFIG(WARNING, "(%s) some packets maybe resent for Tx
> and dropped for Rx\n",
> + dev->ifname);
> }
>
> vq->access_ok = true;
> @@ -1066,15 +1066,14 @@ dump_guest_pages(struct virtio_net *dev)
> for (i = 0; i < dev->nr_guest_pages; i++) {
> page = &dev->guest_pages[i];
>
> - VHOST_LOG_CONFIG(INFO,
> - "(%s) guest physical page region %u\n"
> - "\t guest_phys_addr: %" PRIx64 "\n"
> - "\t host_phys_addr : %" PRIx64 "\n"
> - "\t size : %" PRIx64 "\n",
> - dev->ifname, i,
> - page->guest_phys_addr,
> - page->host_phys_addr,
> - page->size);
> + VHOST_LOG_CONFIG(INFO, "(%s) guest physical page region %u\n",
> + dev->ifname, i);
> + VHOST_LOG_CONFIG(INFO, "(%s)\tguest_phys_addr: %" PRIx64 "\n",
> + dev->ifname, page->guest_phys_addr);
> + VHOST_LOG_CONFIG(INFO, "(%s)\thost_phys_addr : %" PRIx64 "\n",
> + dev->ifname, page->host_phys_addr);
> + VHOST_LOG_CONFIG(INFO, "(%s)\tsize : %" PRIx64 "\n",
> + dev->ifname, page->size);
> }
> }
> #else
> @@ -1122,8 +1121,7 @@ vhost_user_postcopy_region_register(struct virtio_net
> *dev,
>
> if (ioctl(dev->postcopy_ufd, UFFDIO_REGISTER,
> ®_struct)) {
> - VHOST_LOG_CONFIG(ERR, "(%s) failed to register ufd for region "
> - "%" PRIx64 " - %" PRIx64 " (ufd = %d) %s\n",
> + VHOST_LOG_CONFIG(ERR, "(%s) failed to register ufd for region %"
> PRIx64 " - %" PRIx64 " (ufd = %d) %s\n",
This line is > 100 chars, and I think the original one is fine for log filtering
or using log to search code.
What do you think?
Thanks,
Chenbo
> dev->ifname,
> (uint64_t)reg_struct.range.start,
> (uint64_t)reg_struct.range.start +
> @@ -1133,7 +1131,8 @@ vhost_user_postcopy_region_register(struct virtio_net
> *dev,
> return -1;
> }
>
> - VHOST_LOG_CONFIG(INFO, "(%s)\t userfaultfd registered for range : %"
> PRIx64 " - %" PRIx64 "\n",
> + VHOST_LOG_CONFIG(INFO,
> + "(%s)\t userfaultfd registered for range : %" PRIx64 " - %"
> PRIx64 "\n",
> dev->ifname,
> (uint64_t)reg_struct.range.start,
> (uint64_t)reg_struct.range.start +
> @@ -1218,8 +1217,7 @@ vhost_user_mmap_region(struct virtio_net *dev,
>
> /* Check for memory_size + mmap_offset overflow */
> if (mmap_offset >= -region->size) {
> - VHOST_LOG_CONFIG(ERR, "(%s) mmap_offset (%#"PRIx64") and
> memory_size "
> - "(%#"PRIx64") overflow\n",
> + VHOST_LOG_CONFIG(ERR, "(%s) mmap_offset (%#"PRIx64") and
> memory_size (%#"PRIx64") overflow\n",
> dev->ifname, mmap_offset, region->size);
> return -1;
> }
> @@ -1248,8 +1246,7 @@ vhost_user_mmap_region(struct virtio_net *dev,
> * mmap() kernel implementation would return an error, but
> * better catch it before and provide useful info in the logs.
> */
> - VHOST_LOG_CONFIG(ERR, "(%s) mmap size (0x%" PRIx64 ") "
> - "or alignment (0x%" PRIx64 ") is invalid\n",
> + VHOST_LOG_CONFIG(ERR, "(%s) mmap size (0x%" PRIx64 ") or alignment
> (0x%" PRIx64 ") is invalid\n",
> dev->ifname, region->size + mmap_offset, alignment);
> return -1;
> }
> @@ -1285,24 +1282,22 @@ vhost_user_mmap_region(struct virtio_net *dev,
> }
> }
>
> - VHOST_LOG_CONFIG(INFO,
> - "(%s) guest memory region size: 0x%" PRIx64 "\n"
> - "\t guest physical addr: 0x%" PRIx64 "\n"
> - "\t guest virtual addr: 0x%" PRIx64 "\n"
> - "\t host virtual addr: 0x%" PRIx64 "\n"
> - "\t mmap addr : 0x%" PRIx64 "\n"
> - "\t mmap size : 0x%" PRIx64 "\n"
> - "\t mmap align: 0x%" PRIx64 "\n"
> - "\t mmap off : 0x%" PRIx64 "\n",
> - dev->ifname,
> - region->size,
> - region->guest_phys_addr,
> - region->guest_user_addr,
> - region->host_user_addr,
> - (uint64_t)(uintptr_t)mmap_addr,
> - mmap_size,
> - alignment,
> - mmap_offset);
> + VHOST_LOG_CONFIG(INFO, "(%s) guest memory region size: 0x%" PRIx64 "\n",
> + dev->ifname, region->size);
> + VHOST_LOG_CONFIG(INFO, "(%s)\t guest physical addr: 0x%" PRIx64 "\n",
> + dev->ifname, region->guest_phys_addr);
> + VHOST_LOG_CONFIG(INFO, "(%s)\t guest virtual addr: 0x%" PRIx64 "\n",
> + dev->ifname, region->guest_user_addr);
> + VHOST_LOG_CONFIG(INFO, "(%s)\t host virtual addr: 0x%" PRIx64 "\n",
> + dev->ifname, region->host_user_addr);
> + VHOST_LOG_CONFIG(INFO, "(%s)\t mmap addr : 0x%" PRIx64 "\n",
> + dev->ifname, (uint64_t)(uintptr_t)mmap_addr);
> + VHOST_LOG_CONFIG(INFO, "(%s)\t mmap size : 0x%" PRIx64 "\n",
> + dev->ifname, mmap_size);
> + VHOST_LOG_CONFIG(INFO, "(%s)\t mmap align: 0x%" PRIx64 "\n",
> + dev->ifname, alignment);
> + VHOST_LOG_CONFIG(INFO, "(%s)\t mmap off : 0x%" PRIx64 "\n",
> + dev->ifname, mmap_offset);
>
> return 0;
> }
> @@ -2201,9 +2196,9 @@ vhost_user_set_vring_enable(struct virtio_net **pdev,
>
> if (enable && dev->virtqueue[index]->async) {
> if (dev->virtqueue[index]->async->pkts_inflight_n) {
> - VHOST_LOG_CONFIG(ERR, "(%s) failed to enable vring. "
> - "async inflight packets must be completed first\n",
> - dev->ifname);
> + VHOST_LOG_CONFIG(ERR,
> + "(%s) failed to enable vring. Inflight packets must be
> completed first\n",
> + dev->ifname);
> return RTE_VHOST_MSG_RESULT_ERR;
> }
> }
> @@ -2709,22 +2704,21 @@ vhost_user_set_status(struct virtio_net **pdev, struct
> VhostUserMsg *msg,
> dev->status &= ~VIRTIO_DEVICE_STATUS_FEATURES_OK;
> }
>
> - VHOST_LOG_CONFIG(INFO, "(%s) new device status(0x%08x):\n"
> - "\t-RESET: %u\n"
> - "\t-ACKNOWLEDGE: %u\n"
> - "\t-DRIVER: %u\n"
> - "\t-FEATURES_OK: %u\n"
> - "\t-DRIVER_OK: %u\n"
> - "\t-DEVICE_NEED_RESET: %u\n"
> - "\t-FAILED: %u\n",
> - dev->ifname,
> - dev->status,
> - (dev->status == VIRTIO_DEVICE_STATUS_RESET),
> - !!(dev->status & VIRTIO_DEVICE_STATUS_ACK),
> - !!(dev->status & VIRTIO_DEVICE_STATUS_DRIVER),
> - !!(dev->status & VIRTIO_DEVICE_STATUS_FEATURES_OK),
> - !!(dev->status & VIRTIO_DEVICE_STATUS_DRIVER_OK),
> - !!(dev->status & VIRTIO_DEVICE_STATUS_DEV_NEED_RESET),
> + VHOST_LOG_CONFIG(INFO, "(%s) new device status(0x%08x):\n", dev->ifname,
> + dev->status);
> + VHOST_LOG_CONFIG(INFO, "(%s)\t-RESET: %u\n", dev->ifname,
> + (dev->status == VIRTIO_DEVICE_STATUS_RESET));
> + VHOST_LOG_CONFIG(INFO, "(%s)\t-ACKNOWLEDGE: %u\n", dev->ifname,
> + !!(dev->status & VIRTIO_DEVICE_STATUS_ACK));
> + VHOST_LOG_CONFIG(INFO, "(%s)\t-DRIVER: %u\n", dev->ifname,
> + !!(dev->status & VIRTIO_DEVICE_STATUS_DRIVER));
> + VHOST_LOG_CONFIG(INFO, "(%s)\t-FEATURES_OK: %u\n", dev->ifname,
> + !!(dev->status & VIRTIO_DEVICE_STATUS_FEATURES_OK));
> + VHOST_LOG_CONFIG(INFO, "(%s)\t-DRIVER_OK: %u\n", dev->ifname,
> + !!(dev->status & VIRTIO_DEVICE_STATUS_DRIVER_OK));
> + VHOST_LOG_CONFIG(INFO, "(%s)\t-DEVICE_NEED_RESET: %u\n", dev->ifname,
> + !!(dev->status & VIRTIO_DEVICE_STATUS_DEV_NEED_RESET));
> + VHOST_LOG_CONFIG(INFO, "(%s)\t-FAILED: %u\n", dev->ifname,
> !!(dev->status & VIRTIO_DEVICE_STATUS_FAILED));
>
> return RTE_VHOST_MSG_RESULT_OK;
> --
> 2.34.1
next prev parent reply other threads:[~2022-01-26 3:27 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-01-25 11:24 [PATCH v2 0/9] vhost: improve logging Maxime Coquelin
2022-01-25 11:24 ` [PATCH v2 1/9] vhost: improve IOTLB logs Maxime Coquelin
2022-01-26 3:26 ` Xia, Chenbo
2022-01-25 11:24 ` [PATCH v2 2/9] vhost: improve vDPA registration failure log Maxime Coquelin
2022-01-26 3:26 ` Xia, Chenbo
2022-01-25 11:24 ` [PATCH v2 3/9] vhost: improve Vhost layer logs Maxime Coquelin
2022-01-26 3:26 ` Xia, Chenbo
2022-01-25 11:24 ` [PATCH v2 4/9] vhost: improve Vhost-user " Maxime Coquelin
2022-01-26 3:26 ` Xia, Chenbo
2022-01-25 11:24 ` [PATCH v2 5/9] vhost: improve socket " Maxime Coquelin
2022-01-26 3:26 ` Xia, Chenbo
2022-01-25 11:24 ` [PATCH v2 6/9] vhost: improve Virtio-net " Maxime Coquelin
2022-01-26 3:26 ` Xia, Chenbo
2022-01-25 11:24 ` [PATCH v2 7/9] vhost: remove multi-line logs Maxime Coquelin
2022-01-26 3:27 ` Xia, Chenbo [this message]
2022-01-26 9:30 ` Maxime Coquelin
2022-01-25 11:24 ` [PATCH v2 8/9] vhost: differentiate IOTLB logs Maxime Coquelin
2022-01-26 3:27 ` Xia, Chenbo
2022-01-25 11:24 ` [PATCH v2 9/9] vhost: use proper logging type for data path Maxime Coquelin
2022-01-26 3:27 ` Xia, Chenbo
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=SN6PR11MB3504B5FF1D1E3FC304D8E2D09C209@SN6PR11MB3504.namprd11.prod.outlook.com \
--to=chenbo.xia@intel.com \
--cc=david.marchand@redhat.com \
--cc=dev@dpdk.org \
--cc=maxime.coquelin@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).