Hi Peng & Fei,
> -----Original Message-----
> From: dev <dev-boun...@dpdk.org> On Behalf Of Peng He
> Sent: Friday, January 29, 2021 3:36 PM
> To: dev@dpdk.org
> Cc: maxime.coque...@redhat.com
> Subject: [dpdk-dev] [PATCH] lib/librte_vhost: fix vid allocation race
Fix the title to 'vhost: XXXXX'
>
> From: "chenwei.0515" <chenwei.0...@bytedance.com>
This should not be here.. you could just delete it as the author is already
in commit message.
>
> vhost_new_devcie might be called in different threads at the same time.
s/devcie/device
> thread 1(config thread)
> rte_vhost_driver_start
> ->vhost_user_start_client
> ->vhost_user_add_connection
> -> vhost_new_device
>
> thread 2(vhost-events)
> vhost_user_read_cb
> ->vhost_user_msg_handler (return value < 0)
> -> vhost_user_start_client
> -> vhost_new_device
>
> So there could be a case that a same vid has been allocated twice, or
> some vid might be lost in DPDK lib however still held by the upper
> applications.
Good catch! I checked the code and find there exists cases that different
threads
may allocate vhost slot.
And I also find that other functions which use the global vhost_devices[] may
also
meet the same problem. For example, vhost_destroy_device() could be called by
different
thread. So I suggest to fix the problem completely in all related functions like
vhost_destroy_device() and get_device(). What do you think?
If you agree with above, note that the title should also be changed.
Besides, please also add 'Fixes:$COMMID_ID' and cc to sta...@dpdk.org so it
could be
fixed in LTS. You can check other commit for reference.
>
> Reported-by: Peng He <hepeng.0...@bytedance.com>
> Signed-off-by: Fei Chen <chenwei.0...@bytedance.com>
> Reviewed-by: Zhihong Wang <wangzhihong....@bytedance.com>
> ---
> lib/librte_vhost/vhost.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/lib/librte_vhost/vhost.c b/lib/librte_vhost/vhost.c
> index efb136edd1..db11d293d2 100644
> --- a/lib/librte_vhost/vhost.c
> +++ b/lib/librte_vhost/vhost.c
> @@ -26,6 +26,7 @@
> #include "vhost_user.h"
>
> struct virtio_net *vhost_devices[MAX_VHOST_DEVICE];
> +pthread_mutex_t vhost_dev_lock = PTHREAD_MUTEX_INITIALIZER;
There's a duplicate space between 'pthread_mutex_t' and 'vhost_dev_lock',
Let's just leave one 😊
Thanks,
Chenbo
>
> /* Called with iotlb_lock read-locked */
> uint64_t
> @@ -645,6 +646,7 @@ vhost_new_device(void)
> struct virtio_net *dev;
> int i;
>
> + pthread_mutex_lock(&vhost_dev_lock);
> for (i = 0; i < MAX_VHOST_DEVICE; i++) {
> if (vhost_devices[i] == NULL)
> break;
> @@ -653,6 +655,7 @@ vhost_new_device(void)
> if (i == MAX_VHOST_DEVICE) {
> VHOST_LOG_CONFIG(ERR,
> "Failed to find a free slot for new device.\n");
> + pthread_mutex_unlock(&vhost_dev_lock);
> return -1;
> }
>
> @@ -660,10 +663,13 @@ vhost_new_device(void)
> if (dev == NULL) {
> VHOST_LOG_CONFIG(ERR,
> "Failed to allocate memory for new dev.\n");
> + pthread_mutex_unlock(&vhost_dev_lock);
> return -1;
> }
>
> vhost_devices[i] = dev;
> + pthread_mutex_unlock(&vhost_dev_lock);
> +
> dev->vid = i;
> dev->flags = VIRTIO_DEV_BUILTIN_VIRTIO_NET;
> dev->slave_req_fd = -1;
> --
> 2.23.0
