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 321DEA0A0D; Tue, 2 Feb 2021 00:46:28 +0100 (CET) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 0D4D2240215; Tue, 2 Feb 2021 00:46:23 +0100 (CET) Received: from mail-ej1-f44.google.com (mail-ej1-f44.google.com [209.85.218.44]) by mails.dpdk.org (Postfix) with ESMTP id E2FF040693 for ; Mon, 1 Feb 2021 09:53:20 +0100 (CET) Received: by mail-ej1-f44.google.com with SMTP id b9so3482890ejy.12 for ; Mon, 01 Feb 2021 00:53:20 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc:content-transfer-encoding; bh=kOGdupcHEf8ecMiHR1pj6WLMBJP1MTH1VZys5MdLlLI=; b=Blx8NmW6oXl/gfFCWmzT3e3pDsQBk+dH18PUo5Hdu9MEVeCh8++ibFU5SadGbNeo5+ k/CHl42smk4Z8Hniwun1n/r95Bb/Q+gZc4qUoymqsU9OkdSPytfDb4sr72HRrxGmEgLI JBeLUr+CZhglo3v5zjR0tS3SDgSNAdV+bKQs6qFH/0y+Cy8kB21W2SjaWp3A6AESu2M0 xwD3+WUsqbU69DSWGf0/pY+QaT3zl4kVLfiIgDGaJRzj45bzN+r6fPRB0q6Nl9MFQiVf 7kLaibagwXnMbh2aqPb2yGQiJcKxlxNSmsaWRrhXSBiA+2IJevVmvBJG9gqxmQzkAKUY jibA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=kOGdupcHEf8ecMiHR1pj6WLMBJP1MTH1VZys5MdLlLI=; b=by5FBhQ2Xp1fVp/+MWsM/tPJjM4Np+vmQjRn/cVuq2NFMyG3Cn6oF+0IyF+zN9jxvi 0qI2uTtfETeEE27nou/sqk9JcillI41UhViTe2Y1lEqPa0qPnWqtTLFeBSR4XviMlBcZ IDYDiiW/2FDYv9efbqKZok8pJBTo0nG/9+qTwi5Q+naP7LXLuPLWE/S8HYv0lJ0LYgmK +TQT+/4FyIKZQOedo/Ot27XH6ZYZYdnSF/1oezMhckfQTLMfVIdyLQSS2q+0zWbHyNyW n5SXsuAmoD1WaJUkUQYX2ldY1xbbAT4NEO0GRMHG/WymSsMl2BBwSmbyQLUaiVq42mxD lEXw== X-Gm-Message-State: AOAM5324jE7RNT1uMAL1bpgwn/Jn3RKG1JXsDdbYuB0YInh8InYQhAN9 Ju2bmxTBfi3utdcgIiWSIfCxuxQUNmPjHb7MUkM= X-Google-Smtp-Source: ABdhPJznORLmUN1rz5WEpwLRkFOqDhwL6hUue/Vf8n7FS20OWbHk2wctUDwbmmwo7f9AYJpYmJNLXNWVj/RVz/mheYM= X-Received: by 2002:a17:906:f0c3:: with SMTP id dk3mr16413215ejb.540.1612169600614; Mon, 01 Feb 2021 00:53:20 -0800 (PST) MIME-Version: 1.0 References: <20210129073547.80108-1-hepeng.0320@bytedance.com> In-Reply-To: From: =?UTF-8?B?6LS66bmP?= Date: Mon, 1 Feb 2021 16:53:09 +0800 Message-ID: To: "Xia, Chenbo" Cc: "dev@dpdk.org" , "maxime.coquelin@redhat.com" , "chenwei.0515@bytedance.com" , "wangzhihong.wzh@bytedance.com" Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Mailman-Approved-At: Tue, 02 Feb 2021 00:46:20 +0100 Subject: Re: [dpdk-dev] [PATCH] lib/librte_vhost: fix vid allocation race 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 Sender: "dev" Hi, Chenbo, Thanks for the detailed review! Xia, Chenbo =E4=BA=8E2021=E5=B9=B42=E6=9C=881=E6=97= =A5=E5=91=A8=E4=B8=80 =E4=B8=8B=E5=8D=882:27=E5=86=99=E9=81=93=EF=BC=9A > > Hi Peng & Fei, > > > -----Original Message----- > > From: dev On Behalf Of Peng He > > Sent: Friday, January 29, 2021 3:36 PM > > To: dev@dpdk.org > > Cc: maxime.coquelin@redhat.com > > Subject: [dpdk-dev] [PATCH] lib/librte_vhost: fix vid allocation race > > Fix the title to 'vhost: XXXXX' > > > > > From: "chenwei.0515" > > This should not be here.. you could just delete it as the author is alrea= dy > in commit message. > > > > > vhost_new_devcie might be called in different threads at the same time. > > s/devcie/device > will fix it in v2. > > 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 calle= d by different > thread. So I suggest to fix the problem completely in all related functio= ns like > vhost_destroy_device() and get_device(). What do you think? > > If you agree with above, note that the title should also be changed. > Yes, we've investigated also these places where race would exist. In *vhost_destroy_device*, the access to vhost_devices is just to set the specific slot to NULL. If the vid is not the same, the race will not exist. Two threads will not destroy the same vid at the same time. We will add these notes in the commits for clarity. > Besides, please also add 'Fixes:$COMMID_ID' and cc to stable@dpdk.org so = it could be > fixed in LTS. You can check other commit for reference. will do it the v2. > > > > > Reported-by: Peng He > > Signed-off-by: Fei Chen > > Reviewed-by: Zhihong Wang > > --- > > 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 =3D PTHREAD_MUTEX_INITIALIZER; > > There's a duplicate space between 'pthread_mutex_t' and 'vhost_dev_lock', > Let's just leave one will fix it in v2. > > 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 =3D 0; i < MAX_VHOST_DEVICE; i++) { > > if (vhost_devices[i] =3D=3D NULL) > > break; > > @@ -653,6 +655,7 @@ vhost_new_device(void) > > if (i =3D=3D 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 =3D=3D NULL) { > > VHOST_LOG_CONFIG(ERR, > > "Failed to allocate memory for new dev.\n"); > > + pthread_mutex_unlock(&vhost_dev_lock); > > return -1; > > } > > > > vhost_devices[i] =3D dev; > > + pthread_mutex_unlock(&vhost_dev_lock); > > + > > dev->vid =3D i; > > dev->flags =3D VIRTIO_DEV_BUILTIN_VIRTIO_NET; > > dev->slave_req_fd =3D -1; > > -- > > 2.23.0 > --=20 hepeng