Received-SPF: pass (google.com: domain of linux-kernel+bounces-157175-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Wed, 24 Apr 2024 11:16:01 -0400
From: "Liam R. Howlett" <Liam.Howlett@oracle.com>
To: Luis Chamberlain <mcgrof@kernel.org>
Cc: akpm@linux-foundation.org, willy@infradead.org,
        linux-fsdevel@vger.kernel.org, linux-mm@kvack.org,
        linux-kernel@vger.kernel.org, djwong@kernel.org, david@fromorbit.com,
        gost.dev@samsung.com, p.raghav@samsung.com, da.gomez@samsung.com
Subject: Re: [PATCH 2/2] lib/test_xarray.c: fix error assumptions on
 check_xa_multi_store_adv_add()
Message-ID: <dodldgslvisjrqfnkki2ypt3qejn6gb6l25eewv2euywho4unh@jlbkllb6l3ct>
Mail-Followup-To: "Liam R. Howlett" <Liam.Howlett@oracle.com>, 
	Luis Chamberlain <mcgrof@kernel.org>, akpm@linux-foundation.org, willy@infradead.org, 
	linux-fsdevel@vger.kernel.org, linux-mm@kvack.org, linux-kernel@vger.kernel.org, 
	djwong@kernel.org, david@fromorbit.com, gost.dev@samsung.com, p.raghav@samsung.com, 
	da.gomez@samsung.com
References: <20240423180517.256812-1-mcgrof@kernel.org>
 <20240423180517.256812-3-mcgrof@kernel.org>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240423180517.256812-3-mcgrof@kernel.org>
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?WYpI59ZYB3Ef4b4tG2CIlOHvWzOpHnuzibMoVo3VmvMnJGx+InsG8aGZJTe9?=
 =?us-ascii?Q?fQnsBM32HLu+DbN6JkCH42N8WAYEfraj+b1QGdlZSm1uck8/5JaVUAbC7qXz?=
 =?us-ascii?Q?cS0bp8CmtvHF/W2QfA8bcYzjnoZrqSL33yaV5tY9NtklGcsdFHj6LOjHLNaB?=
 =?us-ascii?Q?LTIlONIoCVVWILXyk49unMOUsChb1HhO9DiVa5y1eIG71hzEsTrGxxdANGLl?=
 =?us-ascii?Q?ncZl5KK1TeFtB4S+K8YTIx8yICPSmZCI0ay4mWdUyPYZ3K8+JYmBTFtOA7Jt?=
 =?us-ascii?Q?ygkgrgGv7v8YySXretjPAucb4FsV8jKCXiMvFs7HZzfF05liY5rNlbmSPjuB?=
 =?us-ascii?Q?vmnKLtOj33QQjFtFRvmpZhflZ6ZmexZO+vIoQatmYCckDQimOTXNZrZS69kT?=
 =?us-ascii?Q?HRI1SnApHsIjJGzKeYNtRN/5q2QKRH3pfeU8lPVWzzG0cQkJQWQrEHPZ22IW?=
 =?us-ascii?Q?qz4KSctjAy7vQaNOo7pogVyreURumwFpSSKBNe0gRgQjd5YrmDYlAPP/tJdx?=
 =?us-ascii?Q?f3/Grv+Dl83N6N871KczYIbyAUfzGwmqWT6C9hPwganhLIU2SstMVIIhm8TP?=
 =?us-ascii?Q?yoh9GT2czFsEIDS3b+s+njgIH2ZbWMMqb/zHM2v6liUAy+ov9kkzj3rDUvHp?=
 =?us-ascii?Q?PJiZ8n2QZSdonbAEbEl+5QLR+FFpofil0+D5Q9+/1EIQcpz8jptB1aGISn/6?=
 =?us-ascii?Q?vmzjeL3Ts1pnvjKCevck10jTDTqMy4qJfyYQiTgs0vA1H2qgnZ6E8TK+Uumr?=
 =?us-ascii?Q?LCWYtcDhPsuLPVk+GddwOBov60iRX2gmuZ3ZhMVx9BgMyehMN/xvvq1yO8Ql?=
 =?us-ascii?Q?VTkyvnAT8CFAR3EGxmIfBGYaz2QLx5PaUbgrVCIkQjwVdgIfI5oybUBZkacg?=
 =?us-ascii?Q?ITg9SX+RFuomYwWZvl4uY1OE8ODNIn8HvOkzkdAzJ7coxQOKS3OONx6Yl1nK?=
 =?us-ascii?Q?rTE0T0K6j3z9aqrykTLy4ogjSg8Dj7VX5+mWVPECaJBFqpLWEkyCxTqOXtyB?=
 =?us-ascii?Q?hnvrl1u9shWmp3pNy2SEKLOSRh3QeB6xYRzs1WWZKafC44Wj3CxLwIl3GRlc?=
 =?us-ascii?Q?gqCvEAsbEGCWEqDfmiLUEvQQlmY4tRcZC2hWBtjPMYI0neVLp1QU+mVfAiBP?=
 =?us-ascii?Q?vVo/8rRj9o84Hf9HxaDCKA04z6i7A7Q2aADpYwGVhq6ONCxEXcWDozkTsS8w?=
 =?us-ascii?Q?c18rzzbmIm+CpEKJlBdFTgw7pvMFdJvQ7j2JrfI/9fMTShA9sCP1XtLxWQyI?=
 =?us-ascii?Q?NRz/dEgCgVrZNBQfcyvr40F35sBhxm+rkHguJMWzZe5af9h3r2+S7AZrA5Er?=
 =?us-ascii?Q?gDACgp2tUk7wicfezjYkxQDR1loZXpL+tR9vI7KMQaiD7ElwOeS75koz+g1z?=
 =?us-ascii?Q?IAmt7iCh4IAuD/jc9+DT7x4i7/9u6ND/WkTHRYYsCLHBhJjH8ahBvrlc8AD3?=
 =?us-ascii?Q?WHmQkcjaKbXgogvBkg0TgTcPuh7P4LiCcI93x/qynEbBCfrqEfA1t8IWk30w?=
 =?us-ascii?Q?BGzKXvrbBVKHBzvI9vnY7b27KBrTbYXcf+01QD6I0mTAFVN7ilsBH8oTnxJy?=
 =?us-ascii?Q?ri0yyJ+lmDXlVtC6hlOLILTvmlQHY8cBh4xwTZRtsddCdmJWoCz+9WP0G3TM?=
 =?us-ascii?Q?Ig=3D=3D?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	U3gNkXT3pJ++0ShUwXHeGqQOJvcxjR8uuhR9qHo9Fygckn3QkTqeU1FDLHJ0fzMS0xQm3XMfogv8m7GtoS55LsPkIyYvA+7/4DYNqeJ34cUEpzZMfOvpZEItF1DXye83dXYb0kM4TO6ENJi7fTtk4LgQ374Q8pjVxxAGnFFNFGbLpDHTcBL3HAJP3n95jvXJWPkI6new2n5IZ9GJPb4oiNdybRZdCuA8Ck3cBwKwbMBLLwliEpAjZ+OYpcWQkR0rZxJ1tlB/dbhke69iJpbXjwAI4Y314Q0ntwQNNe7N0Cm6qAWQ+MJy4NxKVzE++6fiqgPycRA8LySg3CLSLRbcSJYU5FTaGfZzFa+PO5V+5qlj3sIi5s7ZYUzRZxJtsIJtGB3bPOX/Hofp41Zgy6Tn50UmyMUB5QeoBDT+TgGQV3uaQl48WePYH+ZXpIV+iZ/YqMRuPn15SosO8LVCCY6BEcZJ3Kk5ayJF35ttARQpb3eE8J/Dn8y9hN9CGnVJ0ShJCgz6y1FpAPtGV0381nm/KzvvuwtXzPEaAwk14ObeMiC4oPCcNMQal7tWeeiexcVK6WgHhbPTEouLvj78ChqBIZuUs36y1tJ8sC9K3U0W3Fg=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: dcfa3248-faba-4c0a-a9a1-08dc647174ca
X-MS-Exchange-CrossTenant-AuthSource: DS0PR10MB7933.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 24 Apr 2024 15:16:03.4642
 (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: FM8A/yS7rBCnkjLSmjO9XQ8byXQoe2XkvFEqA7kpIUeakhZ9Kis930XVU7LgVt+9Bx0l18zKNoEW5lF3eEz7CQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH3PR10MB7260
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.293,Aquarius:18.0.1011,Hydra:6.0.650,FMLib:17.11.176.26
 definitions=2024-04-24_12,2024-04-24_01,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 phishscore=0 suspectscore=0 bulkscore=0
 mlxlogscore=999 spamscore=0 malwarescore=0 mlxscore=0 adultscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2404010000
 definitions=main-2404240059
X-Proofpoint-GUID: m1B2XnkyjOeJO-6jqu-Wge8H3k0D5T4T
X-Proofpoint-ORIG-GUID: m1B2XnkyjOeJO-6jqu-Wge8H3k0D5T4T

* Luis Chamberlain <mcgrof@kernel.org> [240423 14:05]:
> While testing lib/test_xarray in userspace I've noticed we can fail with:
> 
> make -C tools/testing/radix-tree
> ./tools/testing/radix-tree/xarray
> 
> BUG at check_xa_multi_store_adv_add:749
> xarray: 0x55905fb21a00x head 0x55905fa1d8e0x flags 0 marks 0 0 0
> 0: 0x55905fa1d8e0x
> xarray: ../../../lib/test_xarray.c:749: check_xa_multi_store_adv_add: Assertion `0' failed.
> Aborted
> 
> We get a failure with a BUG_ON(), and that is because we actually can
> fail due to -ENOMEM, the check in xas_nomem() will fix this for us so
> it makes no sense to expect no failure inside the loop. So modify the
> check and since this is also useful for instructional purposes clarify
> the situation.

The default behaviour in the testing framework is to test the error
path, which is what you are seeing with the less likely return of
-ENOMEM.

> 
> The check for XA_BUG_ON(xa, xa_load(xa, index) != p) is already done
> at the end of the loop so just remove the bogus on inside the loop.
> 
> With this we now pass the test in both kernel and userspace:
> 
> In userspace:
> 
> ./tools/testing/radix-tree/xarray
> XArray: 149092856 of 149092856 tests passed
> 
> In kernel space:
> 
> XArray: 148257077 of 148257077 tests passed
> 
> Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>

Reviewed-by: Liam R. Howlett <Liam.Howlett@oracle.com>

> ---
>  lib/test_xarray.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/lib/test_xarray.c b/lib/test_xarray.c
> index ebe2af2e072d..5ab35190aae3 100644
> --- a/lib/test_xarray.c
> +++ b/lib/test_xarray.c
> @@ -744,15 +744,20 @@ static noinline void check_xa_multi_store_adv_add(struct xarray *xa,
>  
>  	do {
>  		xas_lock_irq(&xas);
> -
>  		xas_store(&xas, p);
> -		XA_BUG_ON(xa, xas_error(&xas));
> -		XA_BUG_ON(xa, xa_load(xa, index) != p);
> -
>  		xas_unlock_irq(&xas);
> +		/*
> +		 * In our selftest case the only failure we can expect is for
> +		 * there not to be enough memory as we're not mimicking the
> +		 * entire page cache, so verify that's the only error we can run
> +		 * into here. The xas_nomem() which follows will ensure to fix
> +		 * that condition for us so to chug on on the loop.
> +		 */
> +		XA_BUG_ON(xa, xas_error(&xas) && xas_error(&xas) != -ENOMEM);
>  	} while (xas_nomem(&xas, GFP_KERNEL));
>  
>  	XA_BUG_ON(xa, xas_error(&xas));
> +	XA_BUG_ON(xa, xa_load(xa, index) != p);
>  }
>  
>  /* mimics page_cache_delete() */
> -- 
> 2.43.0
>