RE: [PATCH v2 1/3] cxl: Change 'struct cxl_memdev_state' *_perf_list to single 'struct cxl_dpa_perf'

[Dan Williams <dan.j.williams@intel.com>]

Dave Jiang wrote:
> In order to address the issue with being able to expose qos_class sysfs
> attributes under 'ram' and 'pmem' sub-directories, the attributes must
> be defined as static attributes rather than under driver->dev_groups.
> To avoid implementing locking for accessing the 'struct cxl_dpa_perf`
> lists, convert the list to a single 'struct cxl_dpa_perf' entry in
> preparation to move the attributes to statically defined.
> 
> Link: https://lore.kernel.org/linux-cxl/65b200ba228f_2d43c29468@dwillia2-mobl3.amr.corp.intel.com.notmuch/
> Suggested-by: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> ---
>  drivers/cxl/core/cdat.c | 90 +++++++++++++----------------------------
>  drivers/cxl/core/mbox.c |  4 +-
>  drivers/cxl/cxlmem.h    | 10 ++---
>  drivers/cxl/mem.c       | 25 ++++--------
>  4 files changed, 42 insertions(+), 87 deletions(-)

Oh, wow, this looks wonderful!

I was expecting the lists to still be there, just moved out of 'struct
cxl_dev_state'. Am I reading this right, the work to select and validate
the "best" performance per partition can be done without list walking?
If so, great!

[..]
> diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
> index c5c9d8e0d88d..a62099e47d71 100644
> --- a/drivers/cxl/mem.c
> +++ b/drivers/cxl/mem.c
> @@ -221,16 +221,13 @@ static ssize_t ram_qos_class_show(struct device *dev,
>  	struct cxl_memdev *cxlmd = to_cxl_memdev(dev);
>  	struct cxl_dev_state *cxlds = cxlmd->cxlds;
>  	struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlds);
> -	struct cxl_dpa_perf *dpa_perf;
> +	struct cxl_dpa_perf *dpa_perf = &mds->ram_perf;
>  
>  	if (!dev->driver)
>  		return -ENOENT;

This can be deleted since it is racy being referenced without the
device_lock(), and nothing in this routine requires the device to be
locked.

>  
> -	if (list_empty(&mds->ram_perf_list))
> -		return -ENOENT;
> -
> -	dpa_perf = list_first_entry(&mds->ram_perf_list, struct cxl_dpa_perf,
> -				    list);
> +	if (dpa_perf->qos_class == CXL_QOS_CLASS_INVALID)
> +		return -ENODATA;

As long as is_visible() checks for CXL_QOS_CLASS_INVALID there is no
need to add error handling in this _show() routine.

>  
>  	return sysfs_emit(buf, "%d\n", dpa_perf->qos_class);
>  }
> @@ -244,16 +241,10 @@ static ssize_t pmem_qos_class_show(struct device *dev,
>  	struct cxl_memdev *cxlmd = to_cxl_memdev(dev);
>  	struct cxl_dev_state *cxlds = cxlmd->cxlds;
>  	struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlds);
> -	struct cxl_dpa_perf *dpa_perf;
> +	struct cxl_dpa_perf *dpa_perf = &mds->pmem_perf;
>  
> -	if (!dev->driver)
> -		return -ENOENT;

Ah, good, you deleted it this time.

> -
> -	if (list_empty(&mds->pmem_perf_list))
> -		return -ENOENT;
> -
> -	dpa_perf = list_first_entry(&mds->pmem_perf_list, struct cxl_dpa_perf,
> -				    list);
> +	if (dpa_perf->qos_class == CXL_QOS_CLASS_INVALID)
> +		return -ENODATA;

This can go.