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

[Dave Jiang <dave.jiang@intel.com>]

On 2/1/24 21:21, Wonjae Lee wrote:
> On Thu, Feb 01, 2024 at 02:47:29PM -0700, 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.
>>
>> While theoretically a partition may have multiple qos_class via CDAT, this
>> has not been encountered with testing on available hardware. The code is
>> simplified for now to not support the complex case until a use case is
>> needed to support that.
>>
>> 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>
>> ---
>> v3:
>> - Add to commit log about simplification (Dan)
>> - Remove check for dev->driver (Dan)
>> - Remove check for invalid qos_class (Dan)
>> ---
>>  drivers/cxl/core/cdat.c | 81 ++++++++++++-----------------------------
>>  drivers/cxl/core/mbox.c |  4 +-
>>  drivers/cxl/cxlmem.h    | 10 ++---
>>  drivers/cxl/mem.c       | 28 ++------------
>>  4 files changed, 33 insertions(+), 90 deletions(-)
>>
>> diff --git a/drivers/cxl/core/cdat.c b/drivers/cxl/core/cdat.c
>> index 6fe11546889f..55b82dfd794b 100644
>> --- a/drivers/cxl/core/cdat.c
>> +++ b/drivers/cxl/core/cdat.c
>> @@ -210,19 +210,12 @@ static int cxl_port_perf_data_calculate(struct cxl_port *port,
>>   return 0;
>>  }
>>
>> -static void add_perf_entry(struct device *dev, struct dsmas_entry *dent,
>> -            struct list_head *list)
>> +static void update_perf_entry(struct device *dev, struct dsmas_entry *dent,
>> +               struct cxl_dpa_perf *dpa_perf)
>>  {
>> - struct cxl_dpa_perf *dpa_perf;
>> -
>> - dpa_perf = kzalloc(sizeof(*dpa_perf), GFP_KERNEL);
>> - if (!dpa_perf)
>> -     return;
>> -
>>   dpa_perf->dpa_range = dent->dpa_range;
>>   dpa_perf->coord = dent->coord;
>>   dpa_perf->qos_class = dent->qos_class;
>> - list_add_tail(&dpa_perf->list, list);
>>   dev_dbg(dev,
>>       "DSMAS: dpa: %#llx qos: %d read_bw: %d write_bw %d read_lat: %d write_lat: %d\n",
>>       dent->dpa_range.start, dpa_perf->qos_class,
>> @@ -230,20 +223,6 @@ static void add_perf_entry(struct device *dev, struct dsmas_entry *dent,
>>       dent->coord.read_latency, dent->coord.write_latency);
>>  }
>>
>> -static void free_perf_ents(void *data)
>> -{
>> - struct cxl_memdev_state *mds = data;
>> - struct cxl_dpa_perf *dpa_perf, *n;
>> - LIST_HEAD(discard);
>> -
>> - list_splice_tail_init(&mds->ram_perf_list, &discard);
>> - list_splice_tail_init(&mds->pmem_perf_list, &discard);
>> - list_for_each_entry_safe(dpa_perf, n, &discard, list) {
>> -     list_del(&dpa_perf->list);
>> -     kfree(dpa_perf);
>> - }
>> -}
>> -
>>  static void cxl_memdev_set_qos_class(struct cxl_dev_state *cxlds,
>>                    struct xarray *dsmas_xa)
>>  {
>> @@ -263,16 +242,14 @@ static void cxl_memdev_set_qos_class(struct cxl_dev_state *cxlds,
>>   xa_for_each(dsmas_xa, index, dent) {
>>       if (resource_size(&cxlds->ram_res) &&
>>           range_contains(&ram_range, &dent->dpa_range))
>> -         add_perf_entry(dev, dent, &mds->ram_perf_list);
>> +         update_perf_entry(dev, dent, &mds->ram_perf);
>>       else if (resource_size(&cxlds->pmem_res) &&
>>            range_contains(&pmem_range, &dent->dpa_range))
>> -         add_perf_entry(dev, dent, &mds->pmem_perf_list);
>> +         update_perf_entry(dev, dent, &mds->pmem_perf);
>>       else
>>           dev_dbg(dev, "no partition for dsmas dpa: %#llx\n",
>>               dent->dpa_range.start);
>>   }
>> -
>> - devm_add_action_or_reset(&cxlds->cxlmd->dev, free_perf_ents, mds);
>>  }
>>
>>  static int match_cxlrd_qos_class(struct device *dev, void *data)
>> @@ -293,24 +270,25 @@ static int match_cxlrd_qos_class(struct device *dev, void *data)
>>   return 0;
>>  }
>>
>> +static void reset_dpa_perf(struct cxl_dpa_perf *dpa_perf)
>> +{
>> + memset(&dpa_perf, 0, sizeof(*dpa_perf));
> 
> Hello,
> 
> I think you meant dpa_perf instead of &dpa_perf, right?

Yes thank you!

> 
> diff --git a/drivers/cxl/core/cdat.c b/drivers/cxl/core/cdat.c
> index 5c93bf9d5253..7091619f12a9 100644
> --- a/drivers/cxl/core/cdat.c
> +++ b/drivers/cxl/core/cdat.c
> @@ -272,7 +272,7 @@ static int match_cxlrd_qos_class(struct device *dev, void *data)
> 
>  static void reset_dpa_perf(struct cxl_dpa_perf *dpa_perf)
>  {
> -       memset(&dpa_perf, 0, sizeof(*dpa_perf));
> +       memset(dpa_perf, 0, sizeof(*dpa_perf));
>         dpa_perf->qos_class = CXL_QOS_CLASS_INVALID;
>  }
> 
> Thanks,
> Wonjae