From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 88755A0093; Fri, 22 Apr 2022 14:17:42 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 2516C410D5; Fri, 22 Apr 2022 14:17:42 +0200 (CEST) Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by mails.dpdk.org (Postfix) with ESMTP id 2799D40042 for ; Fri, 22 Apr 2022 14:17:41 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1650629860; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=uInw3uifgoe8+csdvXVuEkS0ZLnyPwOhu/n9ZZjL414=; b=EFNDL7znsZvOBaqWptCD9/lUe8umYWZQuM9G/yH+G4F0Ti80iucWdAtP5Ff8yJOhkbEcOK 29AoEucuKEwNvWNbBoKiSjz9TrWmvbqpVmMx0gFGOjgUenA4EsNjmOPqKTs7VDdbWrs5oM vFXGLz8YSK0yMTSnMJ78p0apPWp8aD4= Received: from mimecast-mx02.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-586-BuEC1WQ6M32wrdk50H3WDw-1; Fri, 22 Apr 2022 08:17:39 -0400 X-MC-Unique: BuEC1WQ6M32wrdk50H3WDw-1 Received: from smtp.corp.redhat.com (int-mx09.intmail.prod.int.rdu2.redhat.com [10.11.54.9]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 563123C10AB2; Fri, 22 Apr 2022 12:17:39 +0000 (UTC) Received: from [10.39.208.35] (unknown [10.39.208.35]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 7A10354CE39; Fri, 22 Apr 2022 12:17:38 +0000 (UTC) Message-ID: <599c3d1e-cae7-faeb-36a2-1e5d23413853@redhat.com> Date: Fri, 22 Apr 2022 14:17:37 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.7.0 To: "Xia, Chenbo" , "dev@dpdk.org" , "david.marchand@redhat.com" References: <20220127145655.558029-1-maxime.coquelin@redhat.com> <20220127145655.558029-3-maxime.coquelin@redhat.com> From: Maxime Coquelin Subject: Re: [PATCH 2/5] vhost: add per-virtqueue statistics support In-Reply-To: X-Scanned-By: MIMEDefang 2.85 on 10.11.54.9 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=maxime.coquelin@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Hi Chenbo, On 4/21/22 16:09, Xia, Chenbo wrote: > Hi Maxime, > >> -----Original Message----- >> From: Maxime Coquelin >> Sent: Thursday, January 27, 2022 10:57 PM >> To: dev@dpdk.org; Xia, Chenbo ; >> david.marchand@redhat.com >> Cc: Maxime Coquelin >> Subject: [PATCH 2/5] vhost: add per-virtqueue statistics support >> >> This patch introduces new APIs for the application >> to query and reset per-virtqueue statistics. The >> patch also introduces generic counters. >> >> Signed-off-by: Maxime Coquelin >> --- >> lib/vhost/rte_vhost.h | 89 +++++++++++++++++++++++++++++++++ >> lib/vhost/socket.c | 4 +- >> lib/vhost/version.map | 5 ++ >> lib/vhost/vhost.c | 109 ++++++++++++++++++++++++++++++++++++++++- >> lib/vhost/vhost.h | 18 ++++++- >> lib/vhost/virtio_net.c | 53 ++++++++++++++++++++ >> 6 files changed, 274 insertions(+), 4 deletions(-) >> >> diff --git a/lib/vhost/rte_vhost.h b/lib/vhost/rte_vhost.h >> index b454c05868..e739091ca0 100644 >> --- a/lib/vhost/rte_vhost.h >> +++ b/lib/vhost/rte_vhost.h >> @@ -37,6 +37,7 @@ extern "C" { >> #define RTE_VHOST_USER_LINEARBUF_SUPPORT (1ULL << 6) >> #define RTE_VHOST_USER_ASYNC_COPY (1ULL << 7) >> #define RTE_VHOST_USER_NET_COMPLIANT_OL_FLAGS (1ULL << 8) >> +#define RTE_VHOST_USER_NET_STATS_ENABLE (1ULL << 9) >> >> /* Features. */ >> #ifndef VIRTIO_NET_F_GUEST_ANNOUNCE >> @@ -317,6 +318,32 @@ struct rte_vhost_power_monitor_cond { >> uint8_t match; >> }; >> >> +/** Maximum name length for the statistics counters */ >> +#define RTE_VHOST_STATS_NAME_SIZE 64 >> + >> +/** >> + * Vhost virtqueue statistics structure >> + * >> + * This structure is used by rte_vhost_vring_stats_get() to provide >> + * virtqueue statistics to the calling application. >> + * It maps a name ID, corresponding to an index in the array returned >> + * by rte_vhost_vring_stats_get_names(), to a statistic value. >> + */ >> +struct rte_vhost_stat { >> + uint64_t id; /**< The index in xstats name array. */ >> + uint64_t value; /**< The statistic counter value. */ >> +}; >> + >> +/** >> + * Vhost virtqueue statistic name element >> + * >> + * This structure is used by rte_vhost_vring_stats_get_anmes() to > > Anmes -> names > >> + * provide virtqueue statistics names to the calling application. >> + */ >> +struct rte_vhost_stat_name { >> + char name[RTE_VHOST_STATS_NAME_SIZE]; /**< The statistic name. */ > > Should we consider using ethdev one? Since vhost lib already depends on ethdev > lib. I initially thought about it, but this is not a good idea IMHO, as it might confuse the user, which would think it could use the ethdev API to control it. >> +}; >> + >> /** >> * Convert guest physical address to host virtual address >> * >> @@ -1059,6 +1086,68 @@ __rte_experimental >> int >> rte_vhost_slave_config_change(int vid, bool need_reply); >> >> +/** >> + * Retrieve names of statistics of a Vhost virtqueue. >> + * >> + * There is an assumption that 'stat_names' and 'stats' arrays are >> matched >> + * by array index: stats_names[i].name => stats[i].value >> + * >> + * @param vid >> + * vhost device ID >> + * @param queue_id >> + * vhost queue index >> + * @param stats_names >> + * array of at least size elements to be filled. >> + * If set to NULL, the function returns the required number of elements. >> + * @param size >> + * The number of elements in stats_names array. >> + * @return >> + * A negative value on error, otherwise the number of entries filled in >> the >> + * stats name array. >> + */ >> +__rte_experimental >> +int >> +rte_vhost_vring_stats_get_names(int vid, uint16_t queue_id, >> + struct rte_vhost_stat_name *name, unsigned int size); > > '@param stats_names' and 'struct rte_vhost_stat_name *name' do not align and reports > error: > > http://mails.dpdk.org/archives/test-report/2022-March/270275.html Ha yes, it slept through the cracks when I reworked the series, thanks for the heads-up. >> + >> +/** >> + * Retrieve statistics of a Vhost virtqueue. >> + * >> + * There is an assumption that 'stat_names' and 'stats' arrays are >> matched >> + * by array index: stats_names[i].name => stats[i].value >> + * >> + * @param vid >> + * vhost device ID >> + * @param queue_id >> + * vhost queue index >> + * @param stats >> + * A pointer to a table of structure of type rte_vhost_stat to be >> filled with >> + * virtqueue statistics ids and values. >> + * @param n >> + * The number of elements in stats array. >> + * @return >> + * A negative value on error, otherwise the number of entries filled in >> the >> + * stats table. >> + */ >> +__rte_experimental >> +int >> +rte_vhost_vring_stats_get(int vid, uint16_t queue_id, >> + struct rte_vhost_stat *stats, unsigned int n); >> + >> +/** >> + * Reset statistics of a Vhost virtqueue. >> + * >> + * @param vid >> + * vhost device ID >> + * @param queue_id >> + * vhost queue index >> + * @return >> + * 0 on success, a negative value on error. >> + */ >> +__rte_experimental >> +int >> +rte_vhost_vring_stats_reset(int vid, uint16_t queue_id); >> + >> #ifdef __cplusplus >> } >> #endif >> diff --git a/lib/vhost/socket.c b/lib/vhost/socket.c >> index c2f8013cd5..6020565fb6 100644 >> --- a/lib/vhost/socket.c >> +++ b/lib/vhost/socket.c >> @@ -43,6 +43,7 @@ struct vhost_user_socket { >> bool linearbuf; >> bool async_copy; >> bool net_compliant_ol_flags; >> + bool stats_enabled; >> >> /* >> * The "supported_features" indicates the feature bits the >> @@ -228,7 +229,7 @@ vhost_user_add_connection(int fd, struct >> vhost_user_socket *vsocket) >> vhost_set_ifname(vid, vsocket->path, size); >> >> vhost_setup_virtio_net(vid, vsocket->use_builtin_virtio_net, >> - vsocket->net_compliant_ol_flags); >> + vsocket->net_compliant_ol_flags, vsocket->stats_enabled); >> >> vhost_attach_vdpa_device(vid, vsocket->vdpa_dev); >> >> @@ -864,6 +865,7 @@ rte_vhost_driver_register(const char *path, uint64_t >> flags) >> vsocket->linearbuf = flags & RTE_VHOST_USER_LINEARBUF_SUPPORT; >> vsocket->async_copy = flags & RTE_VHOST_USER_ASYNC_COPY; >> vsocket->net_compliant_ol_flags = flags & >> RTE_VHOST_USER_NET_COMPLIANT_OL_FLAGS; >> + vsocket->stats_enabled = flags & RTE_VHOST_USER_NET_STATS_ENABLE; >> >> if (vsocket->async_copy && >> (flags & (RTE_VHOST_USER_IOMMU_SUPPORT | >> diff --git a/lib/vhost/version.map b/lib/vhost/version.map >> index a7ef7f1976..b83f79c87f 100644 >> --- a/lib/vhost/version.map >> +++ b/lib/vhost/version.map >> @@ -84,6 +84,11 @@ EXPERIMENTAL { >> >> # added in 21.11 >> rte_vhost_get_monitor_addr; >> + >> + # added in 22.03 >> + rte_vhost_vring_stats_get_names; >> + rte_vhost_vring_stats_get; >> + rte_vhost_vring_stats_reset; >> }; >> >> INTERNAL { >> diff --git a/lib/vhost/vhost.c b/lib/vhost/vhost.c >> index 42c01abf25..0c6a737aca 100644 >> --- a/lib/vhost/vhost.c >> +++ b/lib/vhost/vhost.c >> @@ -28,6 +28,28 @@ >> struct virtio_net *vhost_devices[MAX_VHOST_DEVICE]; >> pthread_mutex_t vhost_dev_lock = PTHREAD_MUTEX_INITIALIZER; >> >> +struct vhost_vq_stats_name_off { >> + char name[RTE_VHOST_STATS_NAME_SIZE]; >> + unsigned int offset; >> +}; >> + >> +static const struct vhost_vq_stats_name_off vhost_vq_stat_strings[] = { >> + {"good_packets", offsetof(struct vhost_virtqueue, >> stats.packets)}, >> + {"good_bytes", offsetof(struct vhost_virtqueue, >> stats.bytes)}, >> + {"multicast_packets", offsetof(struct vhost_virtqueue, >> stats.multicast)}, >> + {"broadcast_packets", offsetof(struct vhost_virtqueue, >> stats.broadcast)}, >> + {"undersize_packets", offsetof(struct vhost_virtqueue, >> stats.size_bins[0])}, >> + {"size_64_packets", offsetof(struct vhost_virtqueue, >> stats.size_bins[1])}, >> + {"size_65_127_packets", offsetof(struct vhost_virtqueue, >> stats.size_bins[2])}, >> + {"size_128_255_packets", offsetof(struct vhost_virtqueue, >> stats.size_bins[3])}, >> + {"size_256_511_packets", offsetof(struct vhost_virtqueue, >> stats.size_bins[4])}, >> + {"size_512_1023_packets", offsetof(struct vhost_virtqueue, >> stats.size_bins[5])}, >> + {"size_1024_1518_packets", offsetof(struct vhost_virtqueue, >> stats.size_bins[6])}, >> + {"size_1519_max_packets", offsetof(struct vhost_virtqueue, >> stats.size_bins[7])}, >> +}; >> + >> +#define VHOST_NB_VQ_STATS RTE_DIM(vhost_vq_stat_strings) >> + >> /* Called with iotlb_lock read-locked */ >> uint64_t >> __vhost_iova_to_vva(struct virtio_net *dev, struct vhost_virtqueue *vq, >> @@ -758,7 +780,7 @@ vhost_set_ifname(int vid, const char *if_name, >> unsigned int if_len) >> } >> >> void >> -vhost_setup_virtio_net(int vid, bool enable, bool compliant_ol_flags) >> +vhost_setup_virtio_net(int vid, bool enable, bool compliant_ol_flags, >> bool stats_enabled) >> { >> struct virtio_net *dev = get_device(vid); >> >> @@ -773,6 +795,10 @@ vhost_setup_virtio_net(int vid, bool enable, bool >> compliant_ol_flags) >> dev->flags |= VIRTIO_DEV_LEGACY_OL_FLAGS; >> else >> dev->flags &= ~VIRTIO_DEV_LEGACY_OL_FLAGS; >> + if (stats_enabled) >> + dev->flags |= VIRTIO_DEV_STATS_ENABLED; >> + else >> + dev->flags &= ~VIRTIO_DEV_STATS_ENABLED; >> } >> >> void >> @@ -1908,5 +1934,86 @@ rte_vhost_get_monitor_addr(int vid, uint16_t >> queue_id, >> return 0; >> } >> >> + >> +int >> +rte_vhost_vring_stats_get_names(int vid, uint16_t queue_id, >> + struct rte_vhost_stat_name *name, unsigned int size) >> +{ >> + struct virtio_net *dev = get_device(vid); >> + unsigned int i; >> + >> + if (dev == NULL) >> + return -1; >> + >> + if (queue_id >= dev->nr_vring) >> + return -1; >> + >> + if (!(dev->flags & VIRTIO_DEV_STATS_ENABLED)) >> + return -1; >> + >> + if (name == NULL || size < VHOST_NB_VQ_STATS) >> + return VHOST_NB_VQ_STATS; >> + >> + for (i = 0; i < VHOST_NB_VQ_STATS; i++) >> + snprintf(name[i].name, sizeof(name[i].name), "%s_q%u_%s", >> + (queue_id & 1) ? "rx" : "tx", >> + queue_id / 2, vhost_vq_stat_strings[i].name); >> + >> + return VHOST_NB_VQ_STATS; >> +} >> + >> +int >> +rte_vhost_vring_stats_get(int vid, uint16_t queue_id, >> + struct rte_vhost_stat *stats, unsigned int n) >> +{ >> + struct virtio_net *dev = get_device(vid); >> + struct vhost_virtqueue *vq; >> + unsigned int i; >> + >> + if (dev == NULL) >> + return -1; >> + >> + if (queue_id >= dev->nr_vring) >> + return -1; >> + >> + if (!(dev->flags & VIRTIO_DEV_STATS_ENABLED)) >> + return -1; >> + >> + if (stats == NULL || n < VHOST_NB_VQ_STATS) >> + return VHOST_NB_VQ_STATS; >> + >> + vq = dev->virtqueue[queue_id]; >> + >> + rte_spinlock_lock(&vq->access_lock); >> + for (i = 0; i < VHOST_NB_VQ_STATS; i++) { >> + stats[i].value = >> + *(uint64_t *)(((char *)vq) + >> vhost_vq_stat_strings[i].offset); >> + stats[i].id = i; >> + } >> + rte_spinlock_unlock(&vq->access_lock); >> + >> + return VHOST_NB_VQ_STATS; >> +} >> + >> +int rte_vhost_vring_stats_reset(int vid, uint16_t queue_id) >> +{ >> + struct virtio_net *dev = get_device(vid); >> + struct vhost_virtqueue *vq; >> + >> + if (dev == NULL) >> + return -1; >> + >> + if (queue_id >= dev->nr_vring) >> + return -1; >> + > > Guess you forgot to check VIRTIO_DEV_STATS_ENABLED? Yes, I will add it in next revision. >> + vq = dev->virtqueue[queue_id]; >> + >> + rte_spinlock_lock(&vq->access_lock); >> + memset(&vq->stats, 0, sizeof(vq->stats)); >> + rte_spinlock_unlock(&vq->access_lock); >> + >> + return 0; >> +} >> + >> RTE_LOG_REGISTER_SUFFIX(vhost_config_log_level, config, INFO); >> RTE_LOG_REGISTER_SUFFIX(vhost_data_log_level, data, WARNING); >> diff --git a/lib/vhost/vhost.h b/lib/vhost/vhost.h >> index 7085e0885c..4c151244c7 100644 >> --- a/lib/vhost/vhost.h >> +++ b/lib/vhost/vhost.h >> @@ -38,6 +38,8 @@ >> #define VIRTIO_DEV_FEATURES_FAILED ((uint32_t)1 << 4) >> /* Used to indicate that the virtio_net tx code should fill TX ol_flags >> */ >> #define VIRTIO_DEV_LEGACY_OL_FLAGS ((uint32_t)1 << 5) >> +/* Used to indicate the application has requested statistics collection > > Should be only one space between '/*' and 'Used' :P > > Thanks, > Chenbo > Thanks for the reivew, Maxime