From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by dpdk.org (Postfix) with ESMTP id 348777CFF for ; Fri, 20 Apr 2018 10:26:09 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga004.jf.intel.com ([10.7.209.38]) by orsmga101.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 20 Apr 2018 01:26:08 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.49,300,1520924400"; d="scan'208";a="192991477" Received: from aburakov-mobl.ger.corp.intel.com (HELO [10.252.23.81]) ([10.252.23.81]) by orsmga004.jf.intel.com with ESMTP; 20 Apr 2018 01:26:06 -0700 To: Jianfeng Tan , dev@dpdk.org Cc: thomas@monjalon.net 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> From: "Burakov, Anatoly" Message-ID: Date: Fri, 20 Apr 2018 09:26:06 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0 MIME-Version: 1.0 In-Reply-To: <1524156618-81402-3-git-send-email-jianfeng.tan@intel.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US 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 08:26:10 -0000 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... > 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. > > 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. > } > > -- Thanks, Anatoly