Re: [dpdk-dev] [PATCH v21 1/7] dmadev: introduce DMA device library public APIs

[Thomas Monjalon <thomas@monjalon.net>]

09/09/2021 13:18, Bruce Richardson:
> On Thu, Sep 09, 2021 at 12:33:00PM +0200, Thomas Monjalon wrote:
> > 07/09/2021 14:56, Chengwen Feng:
> > > + * The first three APIs are used to submit the operation request to the virtual
> > > + * DMA channel, if the submission is successful, an uint16_t ring_idx is
> > > + * returned, otherwise a negative number is returned.
> > 
> > unsigned or negative? looks weird.
> 
> May be, but it works well. We could perhaps rephase to make it less weird
> though:
> "if the submission is successful, a positive ring_idx <= UINT16_MAX is
>  returned, otherwise a negative number is returned."

I am advocating for int16_t,
it makes a lot of things simpler.

> > > + *
> > > + * The last API was used to issue doorbell to hardware, and also there are flags
> > > + * (@see RTE_DMA_OP_FLAG_SUBMIT) parameter of the first three APIs could do the
> > > + * same work.
> > 
> > I don't understand this sentence.
> > You mean rte_dmadev_submit function?
> > Why past tense "was"?
> > Why having a redundant function?
> > 
> 
> Just because there are two ways to do something does not mean that one of
> them is redundant, as both may be more suitable for different situations.

I agree.

> When enqueuing a set of jobs to the device, having a separate submit
> outside a loop makes for clearer code than having a check for the last
> iteration inside the loop to set a special submit flag.  However, for cases
> where one item alone is to be submitted or there is a small set of jobs to
> be submitted sequentially, having a submit flag provides a lower-overhead
> way of doing the submission while still keeping the code clean.

This kind of explanation may be missing in doxygen?

> > > +bool
> > > +rte_dmadev_is_valid_dev(uint16_t dev_id);
> > 
> > I would suggest dropping the final "_dev" in the function name.
> > 
> 
> The alternative, which I would support, is replacing "rte_dmadev" with
> "rte_dma" across the API. This would then become "rte_dma_is_valid_dev"
> which is clearer, since the dev is not part of the standard prefix. It also
> would fit in with a possible future function of "rte_dma_is_valid_vchan"
> for instance.

Yes
The question is whether it would make sense to reserver rte_dma_ prefix
for some DMA functions which would be outside of dmadev lib?
If you think that all DMA functions will be in dmadev,
then yes we can shorten the prefix to rte_dma_.

> > > +uint16_t
> > > +rte_dmadev_count(void);
> > 
> > It would be safer to name it rte_dmadev_count_avail
> > in case we need new kind of device count later.
> > 
> 
> If we change "dmadev" to "dma" this could then be
> "rte_dma_count_avail_dev".

Yes

> > > +/**
> > > + * A structure used to retrieve the information of a DMA device.
> > > + */
> > > +struct rte_dmadev_info {
> > > +	struct rte_device *device; /**< Generic Device information. */
> > 
> > Please do not expose this.
> > 
> > > +	uint64_t dev_capa; /**< Device capabilities (RTE_DMADEV_CAPA_*). */
> > > +	uint16_t max_vchans;
> > > +	/**< Maximum number of virtual DMA channels supported. */
> > > +	uint16_t max_desc;
> > > +	/**< Maximum allowed number of virtual DMA channel descriptors. */
> > > +	uint16_t min_desc;
> > > +	/**< Minimum allowed number of virtual DMA channel descriptors. */
> > > +	uint16_t max_sges;
> > > +	/**< Maximum number of source or destination scatter-gather entry
> > > +	 * supported.
> > > +	 * If the device does not support COPY_SG capability, this value can be
> > > +	 * zero.
> > > +	 * If the device supports COPY_SG capability, then rte_dmadev_copy_sg()
> > > +	 * parameter nb_src/nb_dst should not exceed this value.
> > > +	 */
> > > +	uint16_t nb_vchans; /**< Number of virtual DMA channel configured. */
> > 
> > What about adding NUMA node?
> > 
> >     /* Local NUMA memory ID. -1 if unknown. */
> >     int16_t numa_node;
> > 
> 
> That was omitted as it could be got through the device structure. If device
> is removed, we need to ensure all fields needed from device, such as numa
> node, are made available here.

Ah yes, forgot about rte_device :)
Yes please remove rte_device from this struct.