Re: [dpdk-dev] [PATCH v4] dmadev: introduce DMA device library

[Jerin Jacob <jerinjacobk@gmail.com>]

On Thu, Jul 15, 2021 at 9:34 PM fengchengwen <fengchengwen@huawei.com> wrote:
>
> @burce, jerin  Some unmodified review comments are returned here:
>
> 1.
> COMMENT: > +                    memset(dmadev_shared_data->data, 0,
> > +                            sizeof(dmadev_shared_data->data));
> I believe all memzones are zero on allocation anyway, so this memset is
> unecessary and can be dropped.
>
> REPLY: I didn't find a comment to clear with this function.
>
> 2.
> COMMENT: > + * @see struct rte_dmadev_info::dev_capa
> > + */
> Drop this flag as unnecessary. All devices either always provide ordering
> guarantee - in which case it's a no-op - or else support the flag.
>
> REPLY: I prefer define it, it could let user know whether support fence.
>
> 3.
> COMMENT: I would suggest v4 to split the patch like (so that we can review and
> ack each patch)
> 1) Only public header file with Doxygen inclusion, (There is a lot of
> Doxygen syntax issue in the patch)
> 2) 1 or more patches for implementation.
>
> REPLY: the V4 still one patch and with doxyen files, It's better now for just
> one patch of header file and implementation.
> Later I will push doxyen file patch and skelton file patch.

Makes sense. Whichever version you would like to have Ack, please
split in that version.

>
> 4.
> COMMENT: > +        * @see RTE_DMA_DEV_TO_MEM
> > +        * @see RTE_DMA_DEV_TO_DEV
> Since we can set of only one direction per vchan . Should be we make
> it as enum to
> make it clear.
>
> REPLY: May some devices support it. I think it's OK for future use.

Sent comment in another thread.

>
> 5.
> COMMENT: > +__rte_experimental
> > +int
> > +rte_dmadev_vchan_release(uint16_t dev_id, uint16_t vchan);
> I would like to remove this to align with other device class in DPDK and use
> configure and start again if there change in vchannel setup/
>
> REPLY: I think we could have dynamic reconfig vchan without stop device ability.

I think, In general, drivers are doing a lot of stuff in _start(),
this will give enough flexibility
to driver(like getting a complete view of HW resources at start()).
IMO, there is no need to deviate from other classes of DPDK devices here.
Also, the release is a slow path operation, so adding more calls for
reconfiguring is OK like
other DPDK device classes.



>
> 6.
> COMMENT: > +
> > +#define RTE_DMADEV_ALL_VCHAN   0xFFFFu
> RTE_DMADEV_VCHAN_ALL ??
>
> REPLY: I don't like reserved a fix length queue stats in 'struct rte_eth_stats',
> which may counter the RTE_ETHDEV_QUEUE_STAT_CNTRS too short problem.
> So here I define the API could get one or ALL vchans stats.

I meant subsystem name can come as the third item like RTE_DMADEV_VCHAN_ALL
vs RTE_DMADEV_ALL_VCHAN where ALL is not a subsystem
i.e RTE_DMADEV_<Sub system in DMA>_.._ACTION/VERB/etc




>
> 7.
> COMMENT: > +rte_dmadev_copy_sg(uint16_t dev_id, uint16_t vchan, const struct rte_dma_sg *sg,
> > +                  uint64_t flags)
> In order to avoid population of rte_dma_sg in stack (as it is
> fastpath), I would like
> to change the API as
> rte_dmadev_copy_sg(uint16_t dev_id, uint16_t vchan, struct rte_dma_sge
> *src,  struct rte_dma_sge *dst,   uint16_t nb_src, uint16_t nb_dst,
> uint64_t flags)
>
> REPLY: Too many (7) parameters if it separates. I prefer define a struct wrap it.

I agree if it is a slow path function. However,

I strongly prefer to not have one more stack indirection in fastpath
and also we will not
be adding new arguments to this function in the future. So that way,
it is better as

rte_dmadev_copy_sg(uint16_t dev_id, uint16_t vchan, struct rte_dma_sge
src,  struct rte_dma_sge *dst,   uint16_t nb_src, uint16_t nb_dst,
uint64_t flags)



>
> 8.
> COMMENT: change RTE_LOG_REGISTER(rte_dmadev_logtype, lib.dmadev, INFO); to
> RTE_LOG_REGISTER_DEFAULT, and change rte_dmadev_logtype to logtype.
>
> REPLY: Our CI version still don't support RTE_LOG_REGISTER_DEFAULT (have not sync newest version),
> I think it could fix in next version or patch.
> and because RTE_LOG_REGISTER define the rte_dmadev_logtype as a un-static variable, if we
> change to logtype, there maybe namespace conflict, so I think it's ok to retain the original
> variables.
>
> thanks.
>
> [snip]
>
> On 2021/7/15 23:41, Chengwen Feng wrote:
> > This patch introduce 'dmadevice' which is a generic type of DMA
> > device.
> >
> > The APIs of dmadev library exposes some generic operations which can
> > enable configuration and I/O with the DMA devices.
> >
> > Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
> > ---
> > v4:
> > * replace xxx_complete_fails with xxx_completed_status.
> > * add SILENT capability, also a silent_mode in rte_dmadev_conf.
> > * add op_flag_llc for performance.
> > * rename dmadev_xxx_t to rte_dmadev_xxx_t to avoid namespace conflict.
> > * delete filed 'enqueued_count' from rte_dmadev_stats.
> > * make rte_dmadev hold 'dev_private' filed.
> > * add RTE_DMA_STATUS_NOT_ATTEMPED status code.
> > * rename RTE_DMA_STATUS_ACTIVE_DROP to RTE_DMA_STATUS_USER_ABORT.
> > * rename rte_dma_sg(e) to rte_dmadev_sg(e) to make sure all struct
> >   prefix with rte_dmadev.
> > * put the comment afterwards.
> > * fix some doxgen problem.
> > * delete macro RTE_DMADEV_VALID_DEV_ID_OR_RET and
> >   RTE_DMADEV_PTR_OR_ERR_RET.
> > * replace strlcpy with rte_strscpy.
> > * other minor modifications from review comment.
> > v3:
>
> [snip]