linux-kernel - Re: [PATCH v2 5/8] cxl/region: Split commit_store() into __commit() and queue

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <c98590aa-1ad8-48f7-8dfb-a343987d769c@intel.com>
Date: Mon, 23 Jun 2025 07:59:34 -0700
From: Dave Jiang <dave.jiang@...el.com>
To: Dan Williams <dan.j.williams@...el.com>, linux-cxl@...r.kernel.org
Cc: linux-kernel@...r.kernel.org, Davidlohr Bueso <dave@...olabs.net>,
 Jonathan Cameron <jonathan.cameron@...wei.com>,
 Alison Schofield <alison.schofield@...el.com>,
 Vishal Verma <vishal.l.verma@...el.com>, Ira Weiny <ira.weiny@...el.com>
Subject: Re: [PATCH v2 5/8] cxl/region: Split commit_store() into __commit()
 and queue_reset() helpers



On 6/18/25 10:04 PM, Dan Williams wrote:
> The complexity of dropping the lock is removed in favor of splitting commit
> operations to a helper, and leaving all the complexities of "decommit" for
> commit_store() to coordinate the different locking contexts.
> 
> The CPU cache-invalidation in the decommit path is solely handled now by
> cxl_region_decode_reset(). Previously the CPU caches were being needlessly
> flushed twice in the decommit path where the first flush had no guarantee
> that the memory would not be immediately re-dirtied.
> 
> Cc: Davidlohr Bueso <dave@...olabs.net>
> Cc: Jonathan Cameron <jonathan.cameron@...wei.com>
> Cc: Dave Jiang <dave.jiang@...el.com>
> Cc: Alison Schofield <alison.schofield@...el.com>
> Cc: Vishal Verma <vishal.l.verma@...el.com>
> Cc: Ira Weiny <ira.weiny@...el.com>
> Signed-off-by: Dan Williams <dan.j.williams@...el.com>

Reviewed-by: Dave Jiang <dave.jiang@...el.com>
> ---
>  drivers/cxl/core/region.c | 99 +++++++++++++++++++++++++++------------
>  1 file changed, 70 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 6e5e1460068d..3a77aec2c447 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -349,30 +349,42 @@ static int cxl_region_decode_commit(struct cxl_region *cxlr)
>  	return rc;
>  }
>  
> -static ssize_t commit_store(struct device *dev, struct device_attribute *attr,
> -			    const char *buf, size_t len)
> +static int queue_reset(struct cxl_region *cxlr)
>  {
> -	struct cxl_region *cxlr = to_cxl_region(dev);
>  	struct cxl_region_params *p = &cxlr->params;
> -	bool commit;
> -	ssize_t rc;
> +	int rc;
>  
> -	rc = kstrtobool(buf, &commit);
> +	rc = down_write_killable(&cxl_region_rwsem);
>  	if (rc)
>  		return rc;
>  
> +	/* Already in the requested state? */
> +	if (p->state < CXL_CONFIG_COMMIT)
> +		goto out;
> +
> +	p->state = CXL_CONFIG_RESET_PENDING;
> +
> +out:
> +	up_write(&cxl_region_rwsem);
> +
> +	return rc;
> +}
> +
> +static int __commit(struct cxl_region *cxlr)
> +{
> +	struct cxl_region_params *p = &cxlr->params;
> +	int rc;
> +
>  	rc = down_write_killable(&cxl_region_rwsem);
>  	if (rc)
>  		return rc;
>  
>  	/* Already in the requested state? */
> -	if (commit && p->state >= CXL_CONFIG_COMMIT)
> -		goto out;
> -	if (!commit && p->state < CXL_CONFIG_COMMIT)
> +	if (p->state >= CXL_CONFIG_COMMIT)
>  		goto out;
>  
>  	/* Not ready to commit? */
> -	if (commit && p->state < CXL_CONFIG_ACTIVE) {
> +	if (p->state < CXL_CONFIG_ACTIVE) {
>  		rc = -ENXIO;
>  		goto out;
>  	}
> @@ -385,31 +397,60 @@ static ssize_t commit_store(struct device *dev, struct device_attribute *attr,
>  	if (rc)
>  		goto out;
>  
> -	if (commit) {
> -		rc = cxl_region_decode_commit(cxlr);
> -		if (rc == 0)
> -			p->state = CXL_CONFIG_COMMIT;
> -	} else {
> -		p->state = CXL_CONFIG_RESET_PENDING;
> -		up_write(&cxl_region_rwsem);
> -		device_release_driver(&cxlr->dev);
> -		down_write(&cxl_region_rwsem);
> -
> -		/*
> -		 * The lock was dropped, so need to revalidate that the reset is
> -		 * still pending.
> -		 */
> -		if (p->state == CXL_CONFIG_RESET_PENDING) {
> -			cxl_region_decode_reset(cxlr, p->interleave_ways);
> -			p->state = CXL_CONFIG_ACTIVE;
> -		}
> -	}
> +	rc = cxl_region_decode_commit(cxlr);
> +	if (rc == 0)
> +		p->state = CXL_CONFIG_COMMIT;
>  
>  out:
>  	up_write(&cxl_region_rwsem);
>  
> +	return rc;
> +}
> +
> +static ssize_t commit_store(struct device *dev, struct device_attribute *attr,
> +			    const char *buf, size_t len)
> +{
> +	struct cxl_region *cxlr = to_cxl_region(dev);
> +	struct cxl_region_params *p = &cxlr->params;
> +	bool commit;
> +	ssize_t rc;
> +
> +	rc = kstrtobool(buf, &commit);
>  	if (rc)
>  		return rc;
> +
> +	if (commit) {
> +		rc = __commit(cxlr);
> +		if (rc)
> +			return rc;
> +		return len;
> +	}
> +
> +	rc = queue_reset(cxlr);
> +	if (rc)
> +		return rc;
> +
> +	/*
> +	 * Unmap the region and depend the reset-pending state to ensure
> +	 * it does not go active again until post reset
> +	 */
> +	device_release_driver(&cxlr->dev);
> +
> +	/*
> +	 * With the reset pending take cxl_region_rwsem unconditionally
> +	 * to ensure the reset gets handled before returning.
> +	 */
> +	guard(rwsem_write)(&cxl_region_rwsem);
> +
> +	/*
> +	 * Revalidate that the reset is still pending in case another
> +	 * thread already handled this reset.
> +	 */
> +	if (p->state == CXL_CONFIG_RESET_PENDING) {
> +		cxl_region_decode_reset(cxlr, p->interleave_ways);
> +		p->state = CXL_CONFIG_ACTIVE;
> +	}
> +
>  	return len;
>  }
>