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

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

On Thu, 17 Oct 2024 16:15:03 -0500
Ira Weiny <ira.weiny@intel.com> wrote:

> Jonathan Cameron wrote:
> > On Wed, 9 Oct 2024 14:49:09 -0500
> > Ira Weiny <ira.weiny@intel.com> wrote:
> >   
> > > Li, Ming4 wrote:  
> > > > On 10/8/2024 7:16 AM, ira.weiny@intel.com wrote:    
> > > > > From: Navneet Singh <navneet.singh@intel.com>
> > > > >    
> 
> [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;
> > >                 }
> > >         }
> > >  
> > > -       if (cnt == 0 || pl_index) {  
> > 
> > I thought this cnt == 0 check was to deal with the no valid
> > extents case where an empty reply is needed.  
> 
> Yes but the bug found by Ming needs to be handled too.  I see Fan is also
> questioning this code.
> 
> So...  for clarity among all of us here is the new function.  I'm not thrilled
> with the use of a goto but I think it is ok here.

Easy enough to avoid and I don't think it hurts readability much to do so.

Your code should work though.

> 
> Ira
> 
> 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;                                                           
>        if (cnt == 0)                                                           
>                goto send_zero_accepted;
>        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);    

Why set this here - to me makes more sense to set it only once but I can
see the logic either way.
         
>   
>                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;          
>                                                                                
>                        rc = cxl_internal_send_cmd(cxl_mbox, &mbox_cmd);        
>                        if (rc)                                                 
>                                return rc;                                      
>                        cnt -= pl_index;                                        
>                        pl_index = 0;                                          
>                }                                                               
>        }                                                                       
>                                                                                
>        if (!pl_index)                                                          
>                return 0;                                                       
>                                                                                
> send_zero_accepted:                                                            
>        mbox_cmd = (struct cxl_mbox_cmd) {                                      
>                .opcode = opcode,                                               
>                .size_in = struct_size(response, extent_list,                   
>                                       pl_index),                               
>                .payload_in = response,                                         
>        };                                                                      
>                                                                                
>        response->flags = 0;                                                    
>        return cxl_internal_send_cmd(cxl_mbox, &mbox_cmd);                      
> }                


Alternative form for what you have...
                                                              
	if (cnt != 0) { /* Something to send */
                 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) /* Space for more? */
				continue;

			/* Send what we have */
			response->flags = 0; 
			if (pl_index < cnt)                                     
				response->flags &= CXL_DCD_EVENT_MORE;
			
			mbox_cmd = (struct cxl_mbox_cmd) {                      
				.opcode = opcode,                               
				.size_in = struct_size(response, extent_list,   
						       pl_index),               
				.payload_in = response,                         
			};

			rc = cxl_internal_send_cmd(cxl_mbox, &mbox_cmd);        
			if (rc)                                                 
				return rc;                                      
			cnt -= pl_index;                                        
			pl_index = 0;
		}
		if (!pl_index)
			return 0;
	}

	/* Catch left overs + send if zero length */
	response->flags = 0;
	mbox_cmd = (struct cxl_mbox_cmd) {                                      
		.opcode = opcode,                                               
		.size_in = struct_size(response, extent_list, pl_index),                               
		.payload_in = response,                                         
	};                                                                      
                                                                                                                                    
	return cxl_internal_send_cmd(cxl_mbox, &mbox_cmd);                      
}

                                                

>