Received-SPF: pass (google.com: domain of linux-kernel+bounces-201279-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Tue, 4 Jun 2024 15:21:58 -0400
From: "Liam R. Howlett" <Liam.Howlett@oracle.com>
To: Sidhartha Kumar <sidhartha.kumar@oracle.com>
Cc: linux-kernel@vger.kernel.org, maple-tree@lists.infradead.org,
        linux-mm@kvack.org, akpm@linux-foundation.org,
        zhangpeng.00@bytedance.com, willy@infradead.org
Subject: Re: [PATCH 06/18] maple_tree: remove mas_destroy() from mas_nomem()
Message-ID: <nwy2dqzviquqxsqjkcihsuhtz7vfzszow2kxamjgdk2s52jyzl@xesgos2ltjen>
Mail-Followup-To: "Liam R. Howlett" <Liam.Howlett@oracle.com>, 
	Sidhartha Kumar <sidhartha.kumar@oracle.com>, linux-kernel@vger.kernel.org, maple-tree@lists.infradead.org, 
	linux-mm@kvack.org, akpm@linux-foundation.org, zhangpeng.00@bytedance.com, 
	willy@infradead.org
References: <20240604174145.563900-1-sidhartha.kumar@oracle.com>
 <20240604174145.563900-7-sidhartha.kumar@oracle.com>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240604174145.563900-7-sidhartha.kumar@oracle.com>
User-Agent: NeoMutt/20231103
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: 
	=?us-ascii?Q?xz6WAQZul7Gg2H8rTywkhDuERhKqB5MtO4FpNMeC1MnxoZ8ixgUp85tv9KPb?=
 =?us-ascii?Q?n/+9oa3KtzPws2jvTCDk6Q0J9PwgXkAKBjVVK5W1kE8j7NBZ0TdnJYYWj5EP?=
 =?us-ascii?Q?kHxgmL2Qq2h9jerWxHcalylD/qYFEiw9Ksc/noqf03slkLpat0MZLw+6DbSS?=
 =?us-ascii?Q?ybAKs7HU04HlBkVFx2HvTpgsLgYSM6UIGUEXmIiLRZ2/Wu4af34Pa2whDb02?=
 =?us-ascii?Q?XnUHsk/awfXzAOAzNlzFYKdSNkalu4E+0zugGOAVH0QhelkOdwQEtBwlAnAW?=
 =?us-ascii?Q?B2FBQ27HVxhAPjFtyl3BVa7CLtXtu45p29tzLL7PqgoTpl6c7N2mue83JKEO?=
 =?us-ascii?Q?v0ZLE/31KA7KjOQb7h/Kuq6r8aMHKXHLFPcK2ad/lPUFYQtTqUoIcfx4SeiY?=
 =?us-ascii?Q?kc1DFhBpaGQVXu4EokjPAn2Zyh8wVDLGRT9X6yueP6crHbFjS1MIKFHl2p1J?=
 =?us-ascii?Q?vr3691Oz6UwlbGssWWPxjQnkL1QsMlXZ6efO6SJWMaTGaWJf2DPahmutO2dT?=
 =?us-ascii?Q?nVJCGmF2a4LhPtFOJVgewf/FuCosOkpqe6kfrrw0dS513bMDoX0c71kwHQB2?=
 =?us-ascii?Q?4TVsp/HM/N+2dasgVU9WonWZS+4awT3xD/yDjhjMQLWhgju/20C+VZrBZ7oX?=
 =?us-ascii?Q?O2MerzYpAb34+nLS0sObEUSAH0hFqV2hAuhlNd0ghTl1tuyCoVsGD+10Sj1x?=
 =?us-ascii?Q?UWCs2lcg5TIb57l85ziKmwsTYnStqJGMqsFfcfquq/93IcSlAEsg+Ogg2bB7?=
 =?us-ascii?Q?8NeShl1lAyGfYUvRn/lKT48moppiveS1MEx1+nykd8gB35hJ3fdDydRxCzkn?=
 =?us-ascii?Q?D8ySKvf4D/bKsbsnskzWMNtF0O+qMRzdYENce+mbKrDvNh5Wlj3HOIREgSnV?=
 =?us-ascii?Q?IeHovtCNitGDPGULwxaSoGuKUchSO0ZFPqxbwjh8Yf1efQI9UbSoiASjf1iS?=
 =?us-ascii?Q?ZPBfkS58qaYakUX0mfT1672tSSbUjT5rpAiEAEA35B3x+5LAis3hjzPBtWX7?=
 =?us-ascii?Q?s1R0u/LSLsOztMBIYBtCPrnS+9hIsd7SjUF3hMindl0dl2dhEM3lh3qSoJdH?=
 =?us-ascii?Q?xnd86PR9n4E/g6zg5B8AxYHjST0GA7SX6ApELnvkJ8gykK2iRrQuhbGtJJ22?=
 =?us-ascii?Q?j5+c1SkQPSlZyNtDraDLL9SPcHpsZ907MxnaToXliGJGKEq+QMXpkfzuDCSj?=
 =?us-ascii?Q?mMcE1Q1EN0DURNjseugj8fepeZvwVqZKkQV9kPs2Hd00hGdDFvEU9PAEMGGA?=
 =?us-ascii?Q?4bJd/1cYYVtOhvo1G7uCRiFzHQuxaPptl7Oxc/3LrsSosScJyHSuTeBvBqR2?=
 =?us-ascii?Q?meNbqTpWWk40FLJ5aPev9A+4pcSJDtU/0Zf9RGamG3bzLRFjQVbkt4mLPNj9?=
 =?us-ascii?Q?ayRmBu89E+iITJrcMJzVgECfTVvu9eqld2/aBxjv26Zwo7PB2SAcT42nI6dY?=
 =?us-ascii?Q?Y6TZbz3czc0gFQet9ib/lEedYo28AAJ4K1womiorryFT4Uwz5EnOZ4ehqV64?=
 =?us-ascii?Q?jHpQCg4PMnFwJqR7kPVmIR8xv3+6/Hks5yYJ4CHHdU61Bfc29gM08V45tEBb?=
 =?us-ascii?Q?M79ut1TfSQQWHeiGjRiCd53tBQuQcmuOJjm+woKR?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	CA0OqWoR4OViJTyGTD/5HdpfskvVkHBYENvn8RrZ5tBB8ElP70MkoSHNBdz5MHS7HlQXSVUWzGU4gQFMHl1oDAq16sTuoluWm8ZOqjAZmvBvBwM3lts2Wvg6vkgqIkEEtkETP/2vxfECupJUEqapAef59N/rctP3epWxQ3zhefq32+0AsZkYoh15QvkTGy39Kl6mqhH/G2HhzDoJ+SJRV/43/MMCa4zIWmo3JqLpgbi5koDsqPEG7oFLAoTDiwDAuCrVJOLG+D/p3oVTsJ3zJeD4gTj3Sd/y4Z5L5MjDDemd21S96Bjy3pxHWatrSQ4Oo6+vYkE9w6gPxA7ng/PDGlk+hxsVi4GCOXSrDcB9sWZlEFIYYfnV8Iu9vSjsNIC8XtzQe1ip0XetOva5xkz5hB14U85ZtsxiIQht2ptiquVncFIlXqf0PfHZ2J+EDP8/zOwB024y7pLve2YO2m6zj8brphIAXm+WM8naZ0q3CM0c7t3LV2TQ4Q1AWOsctPNMxeR7ZeokRSzCWwBxNhAwmIUOAaIdp5dd3UHCW7nxcL1vU8TGZfCdPfyiq4j5VIOGhALjPKCyzl0PmfZ8M5wb44yOJ0K9xxfEdhmcakibsU8=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 39e6f776-d9f8-4a88-5a52-08dc84cb9c2c
X-MS-Exchange-CrossTenant-AuthSource: DS0PR10MB7933.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 04 Jun 2024 19:22:01.4204
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: f8j/6sGWGk5c3JZ2k77Vup8GhR+2uH9uKkrjzl5F2NrvoR65Rq8aJqZah6jJBUC5EduqFiWbVZ6az5HvSXZX2g==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS0PR10MB7202
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.293,Aquarius:18.0.1039,Hydra:6.0.680,FMLib:17.12.28.16
 definitions=2024-06-04_09,2024-06-04_02,2024-05-17_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 mlxscore=0 phishscore=0 bulkscore=0
 adultscore=0 spamscore=0 mlxlogscore=999 suspectscore=0 malwarescore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2405010000
 definitions=main-2406040155
X-Proofpoint-GUID: 6K0clSlF8s1v-B2NvW6dlEbAYvuirXtv
X-Proofpoint-ORIG-GUID: 6K0clSlF8s1v-B2NvW6dlEbAYvuirXtv

* Sidhartha Kumar <sidhartha.kumar@oracle.com> [240604 13:42]:
> Separate call to mas_destroy() from mas_nomem() so we can check for no
> memory errors without destroying the current maple state in
> mas_store_gfp(). We then add calls to mas_destroy() to callers of
> mas_nomem().
> 
> Signed-off-by: Sidhartha Kumar <sidhartha.kumar@oracle.com>
> ---
>  lib/maple_tree.c                 | 39 ++++++++++++++++++++------------
>  tools/testing/radix-tree/maple.c | 11 +++++----
>  2 files changed, 31 insertions(+), 19 deletions(-)
> 
> diff --git a/lib/maple_tree.c b/lib/maple_tree.c
> index 3780d4bb0415..f1496817e52a 100644
> --- a/lib/maple_tree.c
> +++ b/lib/maple_tree.c
> @@ -4526,6 +4526,7 @@ int mas_alloc_cyclic(struct ma_state *mas, unsigned long *startp,
>  	if (*next == 0)
>  		mas->tree->ma_flags |= MT_FLAGS_ALLOC_WRAPPED;
>  
> +	mas_destroy(mas);
>  	return ret;
>  }
>  EXPORT_SYMBOL(mas_alloc_cyclic);
> @@ -5606,18 +5607,22 @@ EXPORT_SYMBOL_GPL(mas_store);
>  int mas_store_gfp(struct ma_state *mas, void *entry, gfp_t gfp)
>  {
>  	MA_WR_STATE(wr_mas, mas, entry);
> +	int ret;
>  
> -	mas_wr_store_setup(&wr_mas);
> -	trace_ma_write(__func__, mas, 0, entry);
>  retry:
> -	mas_wr_store_entry(&wr_mas);
> +	mas_wr_preallocate(&wr_mas, entry, gfp);
> +	WARN_ON_ONCE(mas->store_type == wr_invalid);
> +
>  	if (unlikely(mas_nomem(mas, gfp)))
>  		goto retry;

Nit: missing new line

> +	if (mas_is_err(mas))
> +		goto out;
>  
> -	if (unlikely(mas_is_err(mas)))
> -		return xa_err(mas->node);
> -
> -	return 0;
> +	mas_wr_store_entry(&wr_mas);
> +out:
> +	ret = xa_err(mas->node);

Looking at what xa_err() does, it would probably be wise to only assign
the ret to xa_err() on mas_is_err(), which you do elsewhere so I'm not
sure why this is special.

> +	mas_destroy(mas);
> +	return ret;
>  }
>  EXPORT_SYMBOL_GPL(mas_store_gfp);
>  
> @@ -6365,6 +6370,7 @@ void *mas_erase(struct ma_state *mas)
>  	if (mas_nomem(mas, GFP_KERNEL))
>  		goto write_retry;
>  
> +	mas_destroy(mas);
>  	return entry;
>  }
>  EXPORT_SYMBOL_GPL(mas_erase);
> @@ -6379,10 +6385,8 @@ EXPORT_SYMBOL_GPL(mas_erase);
>  bool mas_nomem(struct ma_state *mas, gfp_t gfp)
>  	__must_hold(mas->tree->ma_lock)
>  {
> -	if (likely(mas->node != MA_ERROR(-ENOMEM))) {
> -		mas_destroy(mas);
> +	if (likely(mas->node != MA_ERROR(-ENOMEM)))
>  		return false;
> -	}
>  
>  	if (gfpflags_allow_blocking(gfp) && !mt_external_lock(mas->tree)) {
>  		mtree_unlock(mas->tree);
> @@ -6460,6 +6464,7 @@ int mtree_store_range(struct maple_tree *mt, unsigned long index,
>  {
>  	MA_STATE(mas, mt, index, last);
>  	MA_WR_STATE(wr_mas, &mas, entry);
> +	int ret = 0;
>  
>  	trace_ma_write(__func__, &mas, 0, entry);
>  	if (WARN_ON_ONCE(xa_is_advanced(entry)))
> @@ -6475,10 +6480,12 @@ int mtree_store_range(struct maple_tree *mt, unsigned long index,
>  		goto retry;
>  
>  	mtree_unlock(mt);
> +
>  	if (mas_is_err(&mas))
> -		return xa_err(mas.node);
> +		ret = xa_err(mas.node);
>  
> -	return 0;
> +	mas_destroy(&mas);
> +	return ret;
>  }
>  EXPORT_SYMBOL(mtree_store_range);
>  
> @@ -6514,6 +6521,7 @@ int mtree_insert_range(struct maple_tree *mt, unsigned long first,
>  		unsigned long last, void *entry, gfp_t gfp)
>  {
>  	MA_STATE(ms, mt, first, last);
> +	int ret = 0;
>  
>  	if (WARN_ON_ONCE(xa_is_advanced(entry)))
>  		return -EINVAL;
> @@ -6529,9 +6537,10 @@ int mtree_insert_range(struct maple_tree *mt, unsigned long first,
>  
>  	mtree_unlock(mt);
>  	if (mas_is_err(&ms))
> -		return xa_err(ms.node);
> +		ret = xa_err(ms.node);
>  
> -	return 0;
> +	mas_destroy(&ms);
> +	return ret;
>  }
>  EXPORT_SYMBOL(mtree_insert_range);
>  
> @@ -6586,6 +6595,7 @@ int mtree_alloc_range(struct maple_tree *mt, unsigned long *startp,
>  
>  unlock:
>  	mtree_unlock(mt);
> +	mas_destroy(&mas);
>  	return ret;
>  }
>  EXPORT_SYMBOL(mtree_alloc_range);
> @@ -6667,6 +6677,7 @@ int mtree_alloc_rrange(struct maple_tree *mt, unsigned long *startp,
>  
>  unlock:
>  	mtree_unlock(mt);
> +	mas_destroy(&mas);
>  	return ret;
>  }
>  EXPORT_SYMBOL(mtree_alloc_rrange);
> diff --git a/tools/testing/radix-tree/maple.c b/tools/testing/radix-tree/maple.c
> index c57979de1576..c834e91e6810 100644
> --- a/tools/testing/radix-tree/maple.c
> +++ b/tools/testing/radix-tree/maple.c
> @@ -119,7 +119,7 @@ static noinline void __init check_new_node(struct maple_tree *mt)
>  	MT_BUG_ON(mt, mas.alloc->slot[0] == NULL);
>  	mas_push_node(&mas, mn);
>  	mas_reset(&mas);
> -	mas_nomem(&mas, GFP_KERNEL); /* free */
> +	mas_destroy(&mas);
>  	mtree_unlock(mt);
>  
>  
> @@ -143,7 +143,7 @@ static noinline void __init check_new_node(struct maple_tree *mt)
>  	mn->parent = ma_parent_ptr(mn);
>  	ma_free_rcu(mn);
>  	mas.status = ma_start;
> -	mas_nomem(&mas, GFP_KERNEL);
> +	mas_destroy(&mas);
>  	/* Allocate 3 nodes, will fail. */
>  	mas_node_count(&mas, 3);
>  	/* Drop the lock and allocate 3 nodes. */
> @@ -160,7 +160,7 @@ static noinline void __init check_new_node(struct maple_tree *mt)
>  	MT_BUG_ON(mt, mas_allocated(&mas) != 3);
>  	/* Free. */
>  	mas_reset(&mas);
> -	mas_nomem(&mas, GFP_KERNEL);
> +	mas_destroy(&mas);
>  
>  	/* Set allocation request to 1. */
>  	mas_set_alloc_req(&mas, 1);
> @@ -275,7 +275,7 @@ static noinline void __init check_new_node(struct maple_tree *mt)
>  			MT_BUG_ON(mt, mas_allocated(&mas) != i - j - 1);
>  		}
>  		mas_reset(&mas);
> -		MT_BUG_ON(mt, mas_nomem(&mas, GFP_KERNEL));
> +		mas_destroy(&mas);

This was checking something with an MT_BUG_ON() that was dropped.  Can
we check it another way?

>  
>  	}
>  
> @@ -298,7 +298,7 @@ static noinline void __init check_new_node(struct maple_tree *mt)
>  	}
>  	MT_BUG_ON(mt, mas_allocated(&mas) != total);
>  	mas_reset(&mas);
> -	mas_nomem(&mas, GFP_KERNEL); /* Free. */
> +	mas_destroy(&mas); /* Free. */
>  
>  	MT_BUG_ON(mt, mas_allocated(&mas) != 0);
>  	for (i = 1; i < 128; i++) {
> @@ -35846,6 +35846,7 @@ static noinline void __init check_nomem(struct maple_tree *mt)
>  	mas_store(&ms, &ms); /* insert 1 -> &ms */
>  	mas_nomem(&ms, GFP_KERNEL); /* Node allocated in here. */
>  	mtree_unlock(mt);
> +	mas_destroy(&ms);
>  	mtree_destroy(mt);
>  }
>  
> -- 
> 2.45.1
>