Re: [PATCH 5/5] RDMA/srp: Make struct scsi_cmnd and struct srp_request adjacent

[Leon Romanovsky <leonro@mellanox.com>]

On Wed, May 19, 2021 at 08:13:09AM -0700, Bart Van Assche wrote:
> On 5/19/21 2:09 AM, Leon Romanovsky wrote:
> > On Tue, May 11, 2021 at 08:27:52PM -0700, Bart Van Assche wrote:
> >> -static void srp_free_req_data(struct srp_target_port *target,
> >> -			      struct srp_rdma_ch *ch)
> >> +static int srp_exit_cmd_priv(struct Scsi_Host *shost, struct scsi_cmnd *cmd)
> >>  {
> >> +	struct srp_target_port *target = host_to_target(shost);
> >>  	struct srp_device *dev = target->srp_host->srp_dev;
> >>  	struct ib_device *ibdev = dev->dev;
> >> -	struct srp_request *req;
> >> -	int i;
> >> +	struct srp_request *req = scsi_cmd_priv(cmd);
> >>  
> >> -	if (!ch->req_ring)
> >> -		return;
> >> -
> >> -	for (i = 0; i < target->req_ring_size; ++i) {
> >> -		req = &ch->req_ring[i];
> >> -		if (dev->use_fast_reg)
> >> -			kfree(req->fr_list);
> >> -		if (req->indirect_dma_addr) {
> >> -			ib_dma_unmap_single(ibdev, req->indirect_dma_addr,
> >> -					    target->indirect_size,
> >> -					    DMA_TO_DEVICE);
> >> -		}
> >> -		kfree(req->indirect_desc);
> >> +	if (dev->use_fast_reg)
> >> +		kfree(req->fr_list);
> > 
> > Isn't cleaner will be to ensure that fr_list is NULL for !dev->use_fast_reg path?
> > In patch #4 https://lore.kernel.org/linux-rdma/20210512032752.16611-5-bvanassche@acm.org
> 
> Hi Leon,
> 
> I think that per-request private data is zero-initialized and hence that
> it is not necessary to clear req->fr_list explicitly. blk_mq_alloc_rqs()
>  passes __GFP_ZERO to alloc_pages_node(). blk_mq_alloc_rqs() does not
> only allocate block layer requests (struct request) but also per-request
> private data (set->cmd_size).

So you don't need this "if (dev->use_fast_reg)" check.

Thanks

> 
> Thanks,
> 
> Bart.
> 
>