From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-5.3 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS, USER_AGENT_SANE_1 autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 65753C4338F for ; Fri, 23 Jul 2021 07:39:19 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 503E560E8E for ; Fri, 23 Jul 2021 07:39:19 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S233602AbhGWG6o (ORCPT ); Fri, 23 Jul 2021 02:58:44 -0400 Received: from verein.lst.de ([213.95.11.211]:37361 "EHLO verein.lst.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229799AbhGWG6o (ORCPT ); Fri, 23 Jul 2021 02:58:44 -0400 Received: by verein.lst.de (Postfix, from userid 2407) id BF95067373; Fri, 23 Jul 2021 09:39:14 +0200 (CEST) Date: Fri, 23 Jul 2021 09:39:14 +0200 From: Christoph Hellwig To: Jason Gunthorpe Cc: David Airlie , Tony Krowiak , Alex Williamson , Christian Borntraeger , Cornelia Huck , Jonathan Corbet , Daniel Vetter , Diana Craciun , dri-devel@lists.freedesktop.org, Eric Auger , Eric Farman , Harald Freudenberger , Vasily Gorbik , Heiko Carstens , intel-gfx@lists.freedesktop.org, intel-gvt-dev@lists.freedesktop.org, Jani Nikula , Jason Herne , Joonas Lahtinen , kvm@vger.kernel.org, Kirti Wankhede , linux-doc@vger.kernel.org, linux-s390@vger.kernel.org, Matthew Rosato , Peter Oberparleiter , Halil Pasic , Rodrigo Vivi , Vineeth Vijayan , Zhi Wang , "Raj, Ashok" , Christoph Hellwig , Leon Romanovsky , Max Gurtovoy , Yishai Hadas , Zhenyu Wang Subject: Re: [PATCH v2 04/14] vfio: Provide better generic support for open/release vfio_device_ops Message-ID: <20210723073914.GC864@lst.de> References: <0-v2-b6a5582525c9+ff96-vfio_reflck_jgg@nvidia.com> <4-v2-b6a5582525c9+ff96-vfio_reflck_jgg@nvidia.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <4-v2-b6a5582525c9+ff96-vfio_reflck_jgg@nvidia.com> User-Agent: Mutt/1.5.17 (2007-11-01) Precedence: bulk List-ID: X-Mailing-List: linux-s390@vger.kernel.org > +int vfio_assign_device_set(struct vfio_device *device, void *set_id) > +{ > + struct vfio_device_set *alloc_dev_set = NULL; > + struct vfio_device_set *dev_set; > + > + if (WARN_ON(!set_id)) > + return -EINVAL; > + > + /* > + * Atomically acquire a singleton object in the xarray for this set_id > + */ > +again: > + xa_lock(&vfio_device_set_xa); > + if (alloc_dev_set) { > + dev_set = __xa_cmpxchg(&vfio_device_set_xa, > + (unsigned long)set_id, NULL, > + alloc_dev_set, GFP_KERNEL); > + if (xa_is_err(dev_set)) { > + xa_unlock(&vfio_device_set_xa); > + kfree(alloc_dev_set); > + return xa_err(dev_set); > + } > + if (!dev_set) > + dev_set = alloc_dev_set; > + } else { > + dev_set = xa_load(&vfio_device_set_xa, (unsigned long)set_id); > + } > + > + if (dev_set) { > + dev_set->device_count++; > + xa_unlock(&vfio_device_set_xa); > + device->dev_set = dev_set; > + if (dev_set != alloc_dev_set) > + kfree(alloc_dev_set); > + return 0; > + } > + xa_unlock(&vfio_device_set_xa); > + > + if (WARN_ON(alloc_dev_set)) > + return -EINVAL; > + > + alloc_dev_set = kzalloc(sizeof(*alloc_dev_set), GFP_KERNEL); > + if (!alloc_dev_set) > + return -ENOMEM; > + mutex_init(&alloc_dev_set->lock); > + alloc_dev_set->set_id = set_id; > + goto again; > +} > +EXPORT_SYMBOL_GPL(vfio_assign_device_set); This looks unessecarily complicated. We can just try to load first and then store it under the same lock, e.g.: int vfio_assign_device_set(struct vfio_device *device, void *set_id) { unsigned long idx = (unsigned long)set_id; struct vfio_device_set *set, *new; int err; if (WARN_ON(!set_id)) return -EINVAL; xa_lock(&vfio_device_set_xa); set = xa_load(&vfio_device_set_xa, idx); if (set) goto found; xa_unlock(&vfio_device_set_xa); new = kzalloc(sizeof(*new), GFP_KERNEL); if (!new) return -ENOMEM; mutex_init(&new->lock); alloc_dev_set->set_id = set_id; xa_lock(&vfio_device_set_xa); set = xa_load(&vfio_device_set_xa, idx); if (set) { kfree(new); goto found; } err = xa_err(__xa_store(&vfio_device_set_xa, idx, new, GFP_KERNEL)); xa_unlock(&vfio_device_set_xa); if (err) kfree(new); return err; found: set->device_count++; xa_unlock(&vfio_device_set_xa); device->dev_set = set; return 0; } > +static void vfio_release_device_set(struct vfio_device *device) > +{ > + struct vfio_device_set *dev_set = device->dev_set; > + > + if (!dev_set) > + return; > + > + xa_lock(&vfio_device_set_xa); > + dev_set->device_count--; > + if (!dev_set->device_count) { Nit, by I'd find if (!--dev_set->device_count) { easier to follow as it clearly documents the dec_and_test pattern. From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-5.2 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS, URIBL_BLOCKED,USER_AGENT_SANE_1 autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 55BA4C4320E for ; Fri, 23 Jul 2021 07:39:20 +0000 (UTC) Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 07BCF60E91 for ; Fri, 23 Jul 2021 07:39:20 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org 07BCF60E91 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=lst.de Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=lists.freedesktop.org Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id B1EB86EA75; Fri, 23 Jul 2021 07:39:19 +0000 (UTC) Received: from verein.lst.de (verein.lst.de [213.95.11.211]) by gabe.freedesktop.org (Postfix) with ESMTPS id E14236E93F; Fri, 23 Jul 2021 07:39:17 +0000 (UTC) Received: by verein.lst.de (Postfix, from userid 2407) id BF95067373; Fri, 23 Jul 2021 09:39:14 +0200 (CEST) Date: Fri, 23 Jul 2021 09:39:14 +0200 From: Christoph Hellwig To: Jason Gunthorpe Message-ID: <20210723073914.GC864@lst.de> References: <0-v2-b6a5582525c9+ff96-vfio_reflck_jgg@nvidia.com> <4-v2-b6a5582525c9+ff96-vfio_reflck_jgg@nvidia.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <4-v2-b6a5582525c9+ff96-vfio_reflck_jgg@nvidia.com> User-Agent: Mutt/1.5.17 (2007-11-01) Subject: Re: [Intel-gfx] [PATCH v2 04/14] vfio: Provide better generic support for open/release vfio_device_ops X-BeenThere: intel-gfx@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Intel graphics driver community testing & development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: kvm@vger.kernel.org, linux-doc@vger.kernel.org, David Airlie , dri-devel@lists.freedesktop.org, Kirti Wankhede , Max Gurtovoy , Vineeth Vijayan , Diana Craciun , Leon Romanovsky , Christoph Hellwig , linux-s390@vger.kernel.org, Matthew Rosato , Jonathan Corbet , Halil Pasic , Christian Borntraeger , intel-gfx@lists.freedesktop.org, Jason Herne , Eric Farman , Vasily Gorbik , Heiko Carstens , Eric Auger , Harald Freudenberger , intel-gvt-dev@lists.freedesktop.org, "Raj, Ashok" , Tony Krowiak , Yishai Hadas , Cornelia Huck , Peter Oberparleiter Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" > +int vfio_assign_device_set(struct vfio_device *device, void *set_id) > +{ > + struct vfio_device_set *alloc_dev_set = NULL; > + struct vfio_device_set *dev_set; > + > + if (WARN_ON(!set_id)) > + return -EINVAL; > + > + /* > + * Atomically acquire a singleton object in the xarray for this set_id > + */ > +again: > + xa_lock(&vfio_device_set_xa); > + if (alloc_dev_set) { > + dev_set = __xa_cmpxchg(&vfio_device_set_xa, > + (unsigned long)set_id, NULL, > + alloc_dev_set, GFP_KERNEL); > + if (xa_is_err(dev_set)) { > + xa_unlock(&vfio_device_set_xa); > + kfree(alloc_dev_set); > + return xa_err(dev_set); > + } > + if (!dev_set) > + dev_set = alloc_dev_set; > + } else { > + dev_set = xa_load(&vfio_device_set_xa, (unsigned long)set_id); > + } > + > + if (dev_set) { > + dev_set->device_count++; > + xa_unlock(&vfio_device_set_xa); > + device->dev_set = dev_set; > + if (dev_set != alloc_dev_set) > + kfree(alloc_dev_set); > + return 0; > + } > + xa_unlock(&vfio_device_set_xa); > + > + if (WARN_ON(alloc_dev_set)) > + return -EINVAL; > + > + alloc_dev_set = kzalloc(sizeof(*alloc_dev_set), GFP_KERNEL); > + if (!alloc_dev_set) > + return -ENOMEM; > + mutex_init(&alloc_dev_set->lock); > + alloc_dev_set->set_id = set_id; > + goto again; > +} > +EXPORT_SYMBOL_GPL(vfio_assign_device_set); This looks unessecarily complicated. We can just try to load first and then store it under the same lock, e.g.: int vfio_assign_device_set(struct vfio_device *device, void *set_id) { unsigned long idx = (unsigned long)set_id; struct vfio_device_set *set, *new; int err; if (WARN_ON(!set_id)) return -EINVAL; xa_lock(&vfio_device_set_xa); set = xa_load(&vfio_device_set_xa, idx); if (set) goto found; xa_unlock(&vfio_device_set_xa); new = kzalloc(sizeof(*new), GFP_KERNEL); if (!new) return -ENOMEM; mutex_init(&new->lock); alloc_dev_set->set_id = set_id; xa_lock(&vfio_device_set_xa); set = xa_load(&vfio_device_set_xa, idx); if (set) { kfree(new); goto found; } err = xa_err(__xa_store(&vfio_device_set_xa, idx, new, GFP_KERNEL)); xa_unlock(&vfio_device_set_xa); if (err) kfree(new); return err; found: set->device_count++; xa_unlock(&vfio_device_set_xa); device->dev_set = set; return 0; } > +static void vfio_release_device_set(struct vfio_device *device) > +{ > + struct vfio_device_set *dev_set = device->dev_set; > + > + if (!dev_set) > + return; > + > + xa_lock(&vfio_device_set_xa); > + dev_set->device_count--; > + if (!dev_set->device_count) { Nit, by I'd find if (!--dev_set->device_count) { easier to follow as it clearly documents the dec_and_test pattern. _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx