From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 2EDF48F57 for ; Sun, 16 Jul 2023 20:46:43 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id A2A67C433C7; Sun, 16 Jul 2023 20:46:42 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=linuxfoundation.org; s=korg; t=1689540403; bh=Df2ZYQNYDaqEBARHlygQOucqyxxJI792awgxIK/N02c=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=CaZUnnjKp/MSGmmyyfM9D2aNZP2hobgcciwUMYT/PkVhOB79m4wgwtgWCuPaWu424 pWWv3szjSt9UqKaXYDtaoNzOXfl7BA0GZVHEVccuU+1V5iKxXp6qirV9ZZPvqNinte Jjf4TGK+GAxOTJ33rdoHcQlh37XS2UXY5dTsUhh4= From: Greg Kroah-Hartman To: stable@vger.kernel.org Cc: Greg Kroah-Hartman , patches@lists.linux.dev, Yongqiang Liu , Paul Cassella , Ira Weiny , Dan Williams , Vishal Verma , Sasha Levin Subject: [PATCH 6.1 351/591] dax: Introduce alloc_dev_dax_id() Date: Sun, 16 Jul 2023 21:48:10 +0200 Message-ID: <20230716194932.991351904@linuxfoundation.org> X-Mailer: git-send-email 2.41.0 In-Reply-To: <20230716194923.861634455@linuxfoundation.org> References: <20230716194923.861634455@linuxfoundation.org> User-Agent: quilt/0.67 Precedence: bulk X-Mailing-List: patches@lists.linux.dev List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit From: Dan Williams [ Upstream commit 70aab281e18c68a1284bc387de127c2fc0bed3f8 ] The reference counting of dax_region objects is needlessly complicated, has lead to confusion [1], and has hidden a bug [2]. Towards cleaning up that mess introduce alloc_dev_dax_id() to minimize the holding of a dax_region reference to only what dev_dax_release() needs, the dax_region->ida. Part of the reason for the mess was the design to dereference a dax_region in all cases in free_dev_dax_id() even if the id was statically assigned by the upper level dax_region driver. Remove the need to call "is_static(dax_region)" by tracking whether the id is dynamic directly in the dev_dax instance itself. With that flag the dax_region pinning and release per dev_dax instance can move to alloc_dev_dax_id() and free_dev_dax_id() respectively. A follow-on cleanup address the unnecessary references in the dax_region setup and drivers. Fixes: 0f3da14a4f05 ("device-dax: introduce 'seed' devices") Link: http://lore.kernel.org/r/20221203095858.612027-1-liuyongqiang13@huawei.com [1] Link: http://lore.kernel.org/r/3cf0890b-4eb0-e70e-cd9c-2ecc3d496263@hpe.com [2] Reported-by: Yongqiang Liu Reported-by: Paul Cassella Reported-by: Ira Weiny Signed-off-by: Dan Williams Link: https://lore.kernel.org/r/168577284563.1672036.13493034988900989554.stgit@dwillia2-xfh.jf.intel.com Reviewed-by: Ira Weiny Signed-off-by: Vishal Verma Signed-off-by: Sasha Levin --- drivers/dax/bus.c | 56 ++++++++++++++++++++++++--------------- drivers/dax/dax-private.h | 4 ++- 2 files changed, 37 insertions(+), 23 deletions(-) diff --git a/drivers/dax/bus.c b/drivers/dax/bus.c index d7a838f651813..3a9348c7f7289 100644 --- a/drivers/dax/bus.c +++ b/drivers/dax/bus.c @@ -432,18 +432,34 @@ static void unregister_dev_dax(void *dev) put_device(dev); } +static void dax_region_free(struct kref *kref) +{ + struct dax_region *dax_region; + + dax_region = container_of(kref, struct dax_region, kref); + kfree(dax_region); +} + +void dax_region_put(struct dax_region *dax_region) +{ + kref_put(&dax_region->kref, dax_region_free); +} +EXPORT_SYMBOL_GPL(dax_region_put); + /* a return value >= 0 indicates this invocation invalidated the id */ static int __free_dev_dax_id(struct dev_dax *dev_dax) { - struct dax_region *dax_region = dev_dax->region; struct device *dev = &dev_dax->dev; + struct dax_region *dax_region; int rc = dev_dax->id; device_lock_assert(dev); - if (is_static(dax_region) || dev_dax->id < 0) + if (!dev_dax->dyn_id || dev_dax->id < 0) return -1; + dax_region = dev_dax->region; ida_free(&dax_region->ida, dev_dax->id); + dax_region_put(dax_region); dev_dax->id = -1; return rc; } @@ -459,6 +475,20 @@ static int free_dev_dax_id(struct dev_dax *dev_dax) return rc; } +static int alloc_dev_dax_id(struct dev_dax *dev_dax) +{ + struct dax_region *dax_region = dev_dax->region; + int id; + + id = ida_alloc(&dax_region->ida, GFP_KERNEL); + if (id < 0) + return id; + kref_get(&dax_region->kref); + dev_dax->dyn_id = true; + dev_dax->id = id; + return id; +} + static ssize_t delete_store(struct device *dev, struct device_attribute *attr, const char *buf, size_t len) { @@ -546,20 +576,6 @@ static const struct attribute_group *dax_region_attribute_groups[] = { NULL, }; -static void dax_region_free(struct kref *kref) -{ - struct dax_region *dax_region; - - dax_region = container_of(kref, struct dax_region, kref); - kfree(dax_region); -} - -void dax_region_put(struct dax_region *dax_region) -{ - kref_put(&dax_region->kref, dax_region_free); -} -EXPORT_SYMBOL_GPL(dax_region_put); - static void dax_region_unregister(void *region) { struct dax_region *dax_region = region; @@ -1284,12 +1300,10 @@ static const struct attribute_group *dax_attribute_groups[] = { static void dev_dax_release(struct device *dev) { struct dev_dax *dev_dax = to_dev_dax(dev); - struct dax_region *dax_region = dev_dax->region; struct dax_device *dax_dev = dev_dax->dax_dev; put_dax(dax_dev); free_dev_dax_id(dev_dax); - dax_region_put(dax_region); kfree(dev_dax->pgmap); kfree(dev_dax); } @@ -1313,6 +1327,7 @@ struct dev_dax *devm_create_dev_dax(struct dev_dax_data *data) if (!dev_dax) return ERR_PTR(-ENOMEM); + dev_dax->region = dax_region; if (is_static(dax_region)) { if (dev_WARN_ONCE(parent, data->id < 0, "dynamic id specified to static region\n")) { @@ -1328,13 +1343,11 @@ struct dev_dax *devm_create_dev_dax(struct dev_dax_data *data) goto err_id; } - rc = ida_alloc(&dax_region->ida, GFP_KERNEL); + rc = alloc_dev_dax_id(dev_dax); if (rc < 0) goto err_id; - dev_dax->id = rc; } - dev_dax->region = dax_region; dev = &dev_dax->dev; device_initialize(dev); dev_set_name(dev, "dax%d.%d", dax_region->id, dev_dax->id); @@ -1375,7 +1388,6 @@ struct dev_dax *devm_create_dev_dax(struct dev_dax_data *data) dev_dax->target_node = dax_region->target_node; dev_dax->align = dax_region->align; ida_init(&dev_dax->ida); - kref_get(&dax_region->kref); inode = dax_inode(dax_dev); dev->devt = inode->i_rdev; diff --git a/drivers/dax/dax-private.h b/drivers/dax/dax-private.h index 1c974b7caae6e..afcada6fd2eda 100644 --- a/drivers/dax/dax-private.h +++ b/drivers/dax/dax-private.h @@ -52,7 +52,8 @@ struct dax_mapping { * @region - parent region * @dax_dev - core dax functionality * @target_node: effective numa node if dev_dax memory range is onlined - * @id: ida allocated id + * @dyn_id: is this a dynamic or statically created instance + * @id: ida allocated id when the dax_region is not static * @ida: mapping id allocator * @dev - device core * @pgmap - pgmap for memmap setup / lifetime (driver owned) @@ -64,6 +65,7 @@ struct dev_dax { struct dax_device *dax_dev; unsigned int align; int target_node; + bool dyn_id; int id; struct ida ida; struct device dev; -- 2.39.2