RE: [PATCH 1/2] cxl/core: Always hold region_rwsem while reading poison lists

From: Dan Williams <dan.j.williams@intel.com>
To: <alison.schofield@intel.com>, Davidlohr Bueso <dave@stgolabs.net>,
	Jonathan Cameron <jonathan.cameron@huawei.com>,
	Dave Jiang <dave.jiang@intel.com>,
	Vishal Verma <vishal.l.verma@intel.com>,
	Ira Weiny <ira.weiny@intel.com>,
	Dan Williams <dan.j.williams@intel.com>
Cc: <linux-cxl@vger.kernel.org>
Subject: RE: [PATCH 1/2] cxl/core: Always hold region_rwsem while reading poison lists
Date: Wed, 29 Nov 2023 17:21:59 -0800	[thread overview]
Message-ID: <6567e3b7b3f06_c3ae294a7@dwillia2-xfh.jf.intel.com.notmuch> (raw)
In-Reply-To: <08e8e7ec9a3413b91d51de39e385653494b1eed0.1701041440.git.alison.schofield@intel.com>

alison.schofield@ wrote:
> From: Alison Schofield <alison.schofield@intel.com>
> 
> A read of a device poison list is triggered via a sysfs attribute
> and the results are logged as kernel trace events of type cxl_poison.
> The work is managed by either: a) the region driver when one of more
> regions map the device, or by b) the memdev driver when no regions
> map the device.
> 
> In the case of a) the region driver holds the region_rwsem while
> reading the poison by committed endpoint decoder mappings and for
> any unmapped resources. This makes sure that the cxl_poison trace
> event trace reports valid region info. (Region name, HPA, and UUID).
> 
> In the case of b) the memdev driver holds the dpa_rwsem preventing
> new DPA resources from being attached to a region. However, it leaves
> a gap between region attach and decoder commit actions. If a DPA in
> the gap is in the poison list, the cxl_poison trace event will omit
> the region info.
> 
> Close the gap by holding the region_rwsem and the dpa_rwsem when
> reading poison per memdev. Since both methods now hold both locks,
> down_read both from the caller. Doing so also addresses the lockdep
> assert that found this issue:
> Commit 458ba8189cb4 ("cxl: Add cxl_decoders_committed() helper")
> 
> Fixes: 7ff6ad107588 ("cxl/memdev: Add trigger_poison_list sysfs attribute")

I am going to drop this Fixes: tag. Every kernel that has this commit
also has f0832a586396, and the there is nothing left to fix after
backporting this new commit.

> Fixes: f0832a586396 ("cxl/region: Provide region info to the cxl_poison trace event")
> Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> ---
>  drivers/cxl/core/memdev.c | 9 ++++++++-
>  drivers/cxl/core/region.c | 5 -----
>  2 files changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
> index fc5c2b414793..5ad1b13e780a 100644
> --- a/drivers/cxl/core/memdev.c
> +++ b/drivers/cxl/core/memdev.c
> @@ -227,10 +227,16 @@ int cxl_trigger_poison_list(struct cxl_memdev *cxlmd)
>  	if (!port || !is_cxl_endpoint(port))
>  		return -EINVAL;
>  
> -	rc = down_read_interruptible(&cxl_dpa_rwsem);
> +	rc = down_read_interruptible(&cxl_region_rwsem);
>  	if (rc)
>  		return rc;
>  
> +	rc = down_read_interruptible(&cxl_dpa_rwsem);
> +	if (rc) {
> +		up_read(&cxl_region_rwsem);
> +		return rc;

I am going to leave this as is, but for future consideration I think it
is ok reason that cxl_region_rwsem has a higher chance for contention.
Compare how long cxl_region_rwsem is held vs cxl_dpa_rwsem. Once it is
acquired, just commit to being uninterruptible with plain down_read()
for cxl_dpa_rwsem.