Message-ID: <445fa543-d83a-44a8-b88d-84be8d979c43@amd.com>
Date: Wed, 5 Feb 2025 09:01:01 +0000
From: Alejandro Lucero Palau <alucerop@....com>
To: Dan Williams <dan.j.williams@...el.com>, Ira Weiny <ira.weiny@...el.com>,
 Dave Jiang <dave.jiang@...el.com>, Davidlohr Bueso <dave@...olabs.net>,
 Jonathan Cameron <jonathan.cameron@...wei.com>,
 Alison Schofield <alison.schofield@...el.com>,
 Vishal Verma <vishal.l.verma@...el.com>
Cc: linux-cxl@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH RFC 2/2] cxl/memdev: Remove temporary variables from
 cxl_memdev_state

<snip>
>>> I was hoping this would get further away from new in/out arguments and
>>> look at centralizing all partition enumeration into one routine.
>> I don't understand?  get partition info is only required if the partition align
>> bytes is not 0.  IOW if the device allows for partitions to be changed.
>> cxl_mem_get_partition_info() is only called IFF that extra query is required.
>> So this does centralize byte information queries into one routine.  It leaves
>> creating partitions to the device driver which moves us toward these being
>> mailbox only calls...
> The crux of the concern for me is less about the role of
> cxl_mem_get_partition_info() and more about the introduction of a new 'struct
> cxl_mem_dev_info' in/out parameter which is similar in function to
> 'struct cxl_dpa_info'. If you can find a way to avoid another level of
> indirection or otherwise consolidate all these steps into a straight
> line routine that does "all the DPA enumeration" things.
>

All this discussion after Dan's patches makes me feel miserable.


I have already used those patches for the Type2 support, and I'm using a 
struct to be used by accel drivers without a mbox for setting up the 
current mds information, required for building up the DPA partitions 
used the new functions. In this case, a struct is needed for sure, 
because there are two alternatives which are more painful than using 
such a struct:

  - one alternative is to allow accel drivers to manipulate internal cxl 
structs what would require, to start with, to export them fully for 
accel drivers. Then those drivers doing non trivial work for something 
the current cxl core is already doing and, IMO, which could be reused 
hidden the complexity.


- Another option, if not such new struct is used, is to pass the 
required data one by one, and although it could be fine by now, I think 
it is not as clean and it does not take into account potential changes 
hardcoded by accel drivers which would require further arguments.


What I have now is quite similar to current pci driver, but using a new 
function for accel drivers (where that new struct is used) instead of 
cxl_dev_state_identify, although some accel drivers will just call that 
function if there exists an mbox. Then cxl_mem_dpa_fetch and 
cxl_dpa_setup will make all the work as they do for the pci driver.


The change is simple and the code now cleaner than the previous versions 
where the DPA mess was still there. So, I'm happy with the DPA mess 
being solved, but ...


... what you seem to suggest now, and I mean both, Ira and Dan, is to 
optimize this which will make life harder for an accel driver. Or 
further helper functions will be needed, or the accel driver will need 
to do a lot of the work now performed by the core. I guess it is not a 
surprise if I speak up against any of these two possibilities.


Maybe it is worth if you have a look at v10 that will be sent later 
today, early in the morning for you, where all this can hopefully be 
seen clearly.

Powered by blists - more mailing lists