Re: [PATCH v4 21/28] cxl/extent: Process DCD events and realize region extents

[Li, Ming4]

On 10/10/2024 3:49 AM, Ira Weiny wrote:
> Li, Ming4 wrote:
>> On 10/8/2024 7:16 AM, ira.weiny@xxxxxxxxx wrote:
>>> From: Navneet Singh <navneet.singh@xxxxxxxxx>
>>>
[snip]
>>> +static int cxl_send_dc_response(struct cxl_memdev_state *mds, int opcode,
>>> +				struct xarray *extent_array, int cnt)
>>> +{
>>> +	struct cxl_mailbox *cxl_mbox = &mds->cxlds.cxl_mbox;
>>> +	struct cxl_mbox_dc_response *p;
>>> +	struct cxl_mbox_cmd mbox_cmd;
>>> +	struct cxl_extent *extent;
>>> +	unsigned long index;
>>> +	u32 pl_index;
>>> +	int rc;
>>> +
>>> +	size_t pl_size = struct_size(p, extent_list, cnt);
>>> +	u32 max_extents = cnt;
>>> +
>>> +	/* May have to use more bit on response. */
>>> +	if (pl_size > cxl_mbox->payload_size) {
>>> +		max_extents = (cxl_mbox->payload_size - sizeof(*p)) /
>>> +			      sizeof(struct updated_extent_list);
>>> +		pl_size = struct_size(p, extent_list, max_extents);
>>> +	}
>>> +
>>> +	struct cxl_mbox_dc_response *response __free(kfree) =
>>> +						kzalloc(pl_size, GFP_KERNEL);
>>> +	if (!response)
>>> +		return -ENOMEM;
>>> +
>>> +	pl_index = 0;
>>> +	xa_for_each(extent_array, index, extent) {
>>> +
>>> +		response->extent_list[pl_index].dpa_start = extent->start_dpa;
>>> +		response->extent_list[pl_index].length = extent->length;
>>> +		pl_index++;
>>> +		response->extent_list_size = cpu_to_le32(pl_index);
>>> +
>>> +		if (pl_index == max_extents) {
>>> +			mbox_cmd = (struct cxl_mbox_cmd) {
>>> +				.opcode = opcode,
>>> +				.size_in = struct_size(response, extent_list,
>>> +						       pl_index),
>>> +				.payload_in = response,
>>> +			};
>>> +
>>> +			response->flags = 0;
>>> +			if (pl_index < cnt)
>>> +				response->flags &= CXL_DCD_EVENT_MORE;
>> It should be 'response->flags |= CXL_DCD_EVENT_MORE' here.
> Ah yea.  Good catch.
>
>> Another issue is if 'cnt' is N times bigger than 'max_extents'(e,g. cnt=20, max_extents=10). all responses will be sent in this xa_for_each(), and CXL_DCD_EVENT_MORE will be set in the last response but it should not be set in these cases.
>>
> Ah yes.  cnt must be decremented.  As I looked at the patch just now the
>
> 	if (cnt == 0 || pl_index)
>
> ... seemed very wrong to me.  That change masked this bug.
>
> This should fix it:
>
> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> index d66beec687a0..99200274dea8 100644
> --- a/drivers/cxl/core/mbox.c
> +++ b/drivers/cxl/core/mbox.c
> @@ -1119,10 +1119,11 @@ static int cxl_send_dc_response(struct cxl_memdev_state *mds, int opcode,
>                         if (rc)
>                                 return rc;
>                         pl_index = 0;
> +                       cnt -= pl_index;

should update cnt before pl_index is reset to 0.

the cnt is a one of parameters of cxl_send_dc_response(), that means the caller gives the value of cnt, is that possible if the size of extent_array is larger than cnt? Should skip remain extents in extent_array when cnt is equal to 0?

>                 }
>         }
>  
> -       if (cnt == 0 || pl_index) {
> +       if (pl_index) {
>                 mbox_cmd = (struct cxl_mbox_cmd) {
>                         .opcode = opcode,
>                         .size_in = struct_size(response, extent_list,
>
>
> Thank you, and sorry again for missing your feedback.
>
> Ira
>
> [snip]