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

[Ira Weiny <ira.weiny@intel.com>]

Jonathan Cameron wrote:
> 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]
> > 

[snip]

> > 
> > 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.

I disagree...  See below.

> 
> 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.

I put it here to group it with the changing of pl_index.  It is extra work.

Since I'm resending I'll make the quick change.

>          
> >   
> >                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...

Sure but lots of indentation on the common path which I have grown
to avoid...  :-/

Looking at this fresh...  A helper function works best.



static int send_one_response(struct cxl_mailbox *cxl_mbox,
                             struct cxl_mbox_dc_response *response,
                             int opcode, u32 extent_list_size, u8 flags)
{
        struct cxl_mbox_cmd mbox_cmd = (struct cxl_mbox_cmd) {
                .opcode = opcode,
                .size_in = struct_size(response, extent_list, extent_list_size),
                .payload_in = response,
        };

        response->extent_list_size = cpu_to_le32(extent_list_size);
        response->flags = flags;
        return cxl_internal_send_cmd(cxl_mbox, &mbox_cmd);
}

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_extent *extent;
        unsigned long index;
        u32 pl_index;

        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;

        if (cnt == 0)
                return send_one_response(cxl_mbox, response, opcode, 0, 0);

        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++;

                if (pl_index == max_extents) {
                        u8 flags = 0;
                        int rc;

                        if (pl_index < cnt)
                                flags &= CXL_DCD_EVENT_MORE;
                        rc = send_one_response(cxl_mbox, response, opcode,
                                               pl_index, flags);
                        if (rc) 
                                return rc;
                        cnt -= pl_index;
                        pl_index = 0;
                }
        }

        if (!pl_index) /* nothing more to do */
                return 0;
        return send_one_response(cxl_mbox, response, opcode, pl_index, 0);
}