On 2021/9/9 20:45, Bruce Richardson wrote:

On Thu, Sep 09, 2021 at 01:29:33PM +0200, Thomas Monjalon wrote:

quoted

09/09/2021 13:18, Bruce Richardson:

quoted

On Thu, Sep 09, 2021 at 12:33:00PM +0200, Thomas Monjalon wrote:

quoted

07/09/2021 14:56, Chengwen Feng:

quoted

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

No, it doesn't work as you can't have wrap-around of the IDs once you use
signed values - and that impacts both the end app and the internals of the
drivers. Let's keep it as-is otherwise it will have massive impacts -
including potential perf impacts.

quoted

quoted

quoted

quoted

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

quoted

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?

It can be added, sure.

quoted

quoted

quoted

quoted

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

Well, any DPDK dma functions which are not in dma library should have the
prefix of the library they are in e.g. rte_eal_dma_*, rte_pci_dma_*
Therefore, I don't think name conflicts should be an issue, and I like
having less typing to do in function names (and I believe Morten was
strongly proposing this previously too)

The dmadev is rather short, if change I prefer all public API with rte_dma_ prefix,
and don't have rte_dma_dev_ prefix for such start/stop/close. (ps: the rte_eth_ also
have rte_eth_dev_close which is painful for OCD).

Also should the filename(e.g. rte_dmadev.h) and directory-name(lib/dmadev) also change ?

quoted

quoted

quoted

quoted

+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

quoted

quoted

quoted

+/**
+ * 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.

quoted

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

.