From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by dpdk.org (Postfix) with ESMTP id 4C1A48D90 for ; Fri, 20 Apr 2018 16:19:26 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by fmsmga101.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 20 Apr 2018 07:19:25 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.49,301,1520924400"; d="scan'208";a="39064892" Received: from tanjianf-mobl.ccr.corp.intel.com (HELO [10.249.235.3]) ([10.249.235.3]) by fmsmga002.fm.intel.com with ESMTP; 20 Apr 2018 07:19:23 -0700 To: "Burakov, Anatoly" , dev@dpdk.org References: <1520177405-59091-1-git-send-email-jianfeng.tan@intel.com> <1524156618-81402-1-git-send-email-jianfeng.tan@intel.com> <1524156618-81402-3-git-send-email-jianfeng.tan@intel.com> Cc: thomas@monjalon.net From: "Tan, Jianfeng" Message-ID: <0025fac5-c007-c793-c3f7-2168343a1026@intel.com> Date: Fri, 20 Apr 2018 22:19:22 +0800 User-Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH v3 2/5] bus/vdev: add lock on vdev device list X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 20 Apr 2018 14:19:26 -0000 On 4/20/2018 4:26 PM, Burakov, Anatoly wrote: > On 19-Apr-18 5:50 PM, Jianfeng Tan wrote: >> As we could add virtual devices from different threads now, we >> add a spin lock to protect the vdev device list. >> >> Suggested-by: Anatoly Burakov >> Signed-off-by: Jianfeng Tan >> Reviewed-by: Qi Zhang >> --- > > <...> > >> +/* The caller shall be responsible for thread-safe */ >> static struct rte_vdev_device * >> find_vdev(const char *name) >> { >> @@ -203,10 +206,6 @@ rte_vdev_init(const char *name, const char *args) >> if (name == NULL) >> return -EINVAL; >> - dev = find_vdev(name); >> - if (dev) >> - return -EEXIST; >> - >> devargs = alloc_devargs(name, args); >> if (!devargs) >> return -ENOMEM; >> @@ -221,16 +220,28 @@ rte_vdev_init(const char *name, const char *args) >> dev->device.numa_node = SOCKET_ID_ANY; >> dev->device.name = devargs->name; >> + rte_spinlock_lock(&vdev_device_list_lock); >> + if (find_vdev(name)) { >> + rte_spinlock_unlock(&vdev_device_list_lock); >> + ret = -EEXIST; >> + goto fail; >> + } >> + TAILQ_INSERT_TAIL(&vdev_device_list, dev, next); >> + rte_spinlock_unlock(&vdev_device_list_lock); >> + > > I wonder if is possible to just leave the tailq locked until you > either insert the device into tailq, or figure out that it's not > possible? Seems like doing two locks here is unnecessary, unless > vdev_probe_all_drivers needs this tailq unlocked... My opinion is that we don't know what could be done in driver probe(). It could possibly insert a new vdev (it does not happen now, but could happen in future?). So here, we call this with tailq unlocked. Or we keep it as simple as possible as you say? > >> ret = vdev_probe_all_drivers(dev); >> if (ret) { >> if (ret > 0) >> VDEV_LOG(ERR, "no driver found for %s\n", name); >> + /* If fails, remove it from vdev list */ >> + rte_spinlock_lock(&vdev_device_list_lock); >> + TAILQ_REMOVE(&vdev_device_list, dev, next); >> + rte_spinlock_unlock(&vdev_device_list_lock); >> goto fail; >> } >> TAILQ_INSERT_TAIL(&devargs_list, devargs, next); >> - TAILQ_INSERT_TAIL(&vdev_device_list, dev, next); >> return 0; >> fail: >> @@ -266,17 +277,25 @@ rte_vdev_uninit(const char *name) >> if (name == NULL) >> return -EINVAL; >> + rte_spinlock_lock(&vdev_device_list_lock); >> dev = find_vdev(name); >> - if (!dev) >> + if (!dev) { >> + rte_spinlock_unlock(&vdev_device_list_lock); >> return -ENOENT; >> + } >> + TAILQ_REMOVE(&vdev_device_list, dev, next); >> + rte_spinlock_unlock(&vdev_device_list_lock); >> devargs = dev->device.devargs; >> ret = vdev_remove_driver(dev); >> - if (ret) >> + if (ret) { >> + /* If fails, add back to vdev list */ >> + rte_spinlock_lock(&vdev_device_list_lock); >> + TAILQ_INSERT_TAIL(&vdev_device_list, dev, next); >> + rte_spinlock_unlock(&vdev_device_list_lock); >> return ret; >> - >> - TAILQ_REMOVE(&vdev_device_list, dev, next); >> + } > > Same comment here - perhaps keep the lock locked all the way? Maybe a > good way to ensure you don't miss anything is put most of it in a > static function, and do > > static int vdev_uninit() { > ... > } > > static int rte_vdev_uninit() { > int ret; > lock(); > ret = vdev_uninit(); > unlock(); > return ret; > } > > ? In general, it is better to do lock/unlock in one place and not > disperse lock/unlock calls across various branches. Makes sense. Will change code accordingly once the above decision is made. > >> TAILQ_REMOVE(&devargs_list, devargs, next); >> @@ -314,19 +333,25 @@ vdev_scan(void) >> if (devargs->bus != &rte_vdev_bus) >> continue; >> - dev = find_vdev(devargs->name); >> - if (dev) >> - continue; >> - >> dev = calloc(1, sizeof(*dev)); >> if (!dev) >> return -1; >> + rte_spinlock_lock(&vdev_device_list_lock); >> + >> + if (find_vdev(devargs->name)) { >> + rte_spinlock_unlock(&vdev_device_list_lock); >> + free(dev); >> + continue; >> + } >> + >> dev->device.devargs = devargs; >> dev->device.numa_node = SOCKET_ID_ANY; >> dev->device.name = devargs->name; >> TAILQ_INSERT_TAIL(&vdev_device_list, dev, next); >> + >> + rte_spinlock_unlock(&vdev_device_list_lock); >> } >> return 0; >> @@ -340,6 +365,10 @@ vdev_probe(void) >> /* call the init function for each virtual device */ >> TAILQ_FOREACH(dev, &vdev_device_list, next) { >> + /* we don't use the vdev lock here, as it's only used in DPDK >> + * initialization; and we don't want to hold such a lock when >> + * we call each driver probe. >> + */ >> if (dev->device.driver) >> continue; >> @@ -360,14 +389,18 @@ vdev_find_device(const struct rte_device >> *start, rte_dev_cmp_t cmp, >> { >> struct rte_vdev_device *dev; >> + rte_spinlock_lock(&vdev_device_list_lock); >> TAILQ_FOREACH(dev, &vdev_device_list, next) { >> if (start && &dev->device == start) { >> start = NULL; >> continue; >> } >> - if (cmp(&dev->device, data) == 0) >> + if (cmp(&dev->device, data) == 0) { >> + rte_spinlock_unlock(&vdev_device_list_lock); >> return &dev->device; >> + } >> } >> + rte_spinlock_unlock(&vdev_device_list_lock); >> return NULL; > > How about > > break; > } > unlock(); > return dev ? &dev->device : NULL; > > ? Seems clearer to me. Yep, will change that. Thanks, Jianfeng