[EXT] Re: [PATCH v3 1/2] dmadev: offload to free source buffer

[Amit Prakash Shukla]

Hi Chengwen,

Thanks for the review and feedback. Please find my reply in-line.

Thanks,
Amit Shukla

> -----Original Message-----
> From: fengchengwen <fengchengwen at huawei.com>
> Sent: Saturday, October 7, 2023 2:31 PM
> To: Amit Prakash Shukla <amitprakashs at marvell.com>; Kevin Laatz
> <kevin.laatz at intel.com>; Bruce Richardson <bruce.richardson at intel.com>
> Cc: dev at dpdk.org; Jerin Jacob Kollanukkaran <jerinj at marvell.com>;
> mb at smartsharesystems.com; conor.walsh at intel.com; Vamsi Krishna
> Attunuru <vattunuru at marvell.com>; g.singh at nxp.com;
> sachin.saxena at oss.nxp.com; hemant.agrawal at nxp.com;
> cheng1.jiang at intel.com; Nithin Kumar Dabilpuram
> <ndabilpuram at marvell.com>; Anoob Joseph <anoobj at marvell.com>
> Subject: [EXT] Re: [PATCH v3 1/2] dmadev: offload to free source buffer
> 
> External Email
> 
> ----------------------------------------------------------------------
> Hi Amit,
> 
> On 2023/9/28 19:50, Amit Prakash Shukla wrote:
> > This changeset adds support in DMA library to free source DMA buffer
> > by hardware. On a supported hardware, application can pass on the
> > mempool information as part of vchan config when the DMA transfer
> > direction is configured as RTE_DMA_DIR_MEM_TO_DEV.
> >
> > Signed-off-by: Amit Prakash Shukla <amitprakashs at marvell.com>
> > Acked-by: Morten Brørup <mb at smartsharesystems.com>
> > Acked-by: Anoob Joseph <anoobj at marvell.com>
> > ---
> >  lib/dmadev/rte_dmadev.h | 27 +++++++++++++++++++++++++++
> >  1 file changed, 27 insertions(+)
> >
> > diff --git a/lib/dmadev/rte_dmadev.h b/lib/dmadev/rte_dmadev.h index
> > b157ab7600..f7a6af2528 100644
> > --- a/lib/dmadev/rte_dmadev.h
> > +++ b/lib/dmadev/rte_dmadev.h
> > @@ -278,6 +278,13 @@ int16_t rte_dma_next_dev(int16_t start_dev_id);
> >  #define RTE_DMA_CAPA_OPS_COPY_SG	RTE_BIT64(33)
> >  /** Support fill operation. */
> >  #define RTE_DMA_CAPA_OPS_FILL		RTE_BIT64(34)
> > +/** Support for source buffer free for mem to dev transfer.
> 
> Support auto free source buffer once the M2D (memory-to-device) transfer
> completed.

Ack

> 
> > + *
> > + * @note Even though the DMA driver has this capability, it may not
> > + support all
> > + * mempool drivers. If the mempool is not supported by the DMA
> > + driver,
> > + * rte_dma_vchan_setup() will fail.
> 
> In addition to hardware support, there are requirements for buffer attribute
> (e.g. only specific mempool support it).
> 
> > + **/
> > +#define RTE_DMA_CAPA_MEM_TO_DEV_SOURCE_BUFFER_FREE
> 	RTE_BIT64(35)
> 
> 1) this should follow RTE_DMA_CAPA_HANDLES_ERRORS, because it not
> new OPS.
> 2) the name is too long. how abort
> RTE_DMA_CAPA_AUTO_FREE_M2D_SBUF? I am not sure, suggest get better
> name.

Ack.

> 
> >  /**@}*/
> >
> >  /**
> > @@ -581,6 +588,19 @@ struct rte_dma_vchan_conf {
> >  	 * @see struct rte_dma_port_param
> >  	 */
> >  	struct rte_dma_port_param dst_port;
> > +	/** Mempool from which source buffer is allocated. Mempool info is
> used
> > +	 * for freeing source buffer by hardware when configured direction is
> > +	 * RTE_DMA_DIR_MEM_TO_DEV. To free the source buffer by
> hardware,
> > +	 * RTE_DMA_OP_FLAG_FREE_SBUF must be set while calling
> rte_dma_copy and
> > +	 * rte_dma_copy_sg().
> > +	 *
> > +	 * @note If the mempool is not supported by the DMA device,
> > +	 * rte_dma_vchan_setup() will fail.
> > +	 *
> > +	 * @see RTE_DMA_OP_FLAG_FREE_SBUF
> > +	 */
> > +	struct rte_mempool *mem_to_dev_src_buf_pool;
> > +
> >  };
> 
> Suggest add one extra struct e.g.
> struct rte_dma_auto_free_param {
>     union {
>         struct rte_mempool *pool;
>     }
>     uint64_t reserved[2]; /**< Reserved for future fields. */ };
> 
> In the above conf, we could add a new field: struct
> rte_dma_auto_free_param  m2d_buf

Ack, will add new struct.
struct rte_dma_auto_free_buf_param {
        struct rte_mempool *pool;
        uint64_t reserved[2]; /**< Reserved for future fields. */ };
};

struct rte_dma_auto_free_buf_param  m2d_sbuf;

> 
> >
> >  /**
> > @@ -818,6 +838,13 @@ struct rte_dma_sge {
> >   * capability bit for this, driver should not return error if this flag was set.
> >   */
> >  #define RTE_DMA_OP_FLAG_LLC     RTE_BIT64(2)
> > +/** Mem to dev source buffer free flag.
> > + * Used for freeing source DMA buffer by hardware when the transfer
> > +direction is
> > + * configured as RTE_DMA_DIR_MEM_TO_DEV.
> > + *
> > + * @see struct rte_dma_vchan_conf::mem_to_dev_src_buf_pool
> > + */
> > +#define RTE_DMA_OP_FLAG_FREE_SBUF	RTE_BIT64(3)
> 
> Suggest RTE_DMA_OP_FLAG_AUTO_FREE_SBUF

Ack

> 
> >  /**@}*/
> >
> >  /**
> >
> 
> The S in SBUF seem useless, because it should not auto free dstbuf in
> logically.
> 
> Maybe we should direct use auto-free (just like silent)