On 2021/10/11 18:40, Bruce Richardson wrote:

On Sat, Oct 09, 2021 at 05:33:37PM +0800, Chengwen Feng wrote:

quoted

This patch add data plane API for dmadev.

A few initial comments inline. I'll work on rebasing my follow-up patchset
to this, and let you know if I have any more feedback based on that.

/Bruce
 

quoted

diff --git a/lib/dmadev/rte_dmadev.c b/lib/dmadev/rte_dmadev.c
index a6a5680d2b..891ceeb988 100644
--- a/lib/dmadev/rte_dmadev.c
+++ b/lib/dmadev/rte_dmadev.c

@@ -17,6 +17,7 @@
 
 static int16_t dma_devices_max;
 
+struct rte_dma_fp_object *rte_dma_fp_objs;

While I think I like this approach of making more of the dmadev hidden, I
think we need a better name for this. While there is the dev_private
pointer in it, the struct is pretty much the datapath functions, so how
about "rte_dma_funcs" as a name?

en, I notice ethdev and eventdev both use rte_xxx_fp_ops, but this structure
has other fileds(e.g. data pointers) in addition to ops, it's inappropriate to
use ops suffix. So I use the 'object' which is widely used in object-oriented.

It's better to use uniform naming in ethdev/eventdev/dmadev and so on, would
be happy to hear more.

quoted

 struct rte_dma_dev *rte_dma_devices;
 

<snip>

quoted

+/**
+ * @internal
+ * Fast-path dmadev functions and related data are hold in a flat array.
+ * One entry per dmadev.
+ *
+ * On 64-bit systems contents of this structure occupy exactly two 64B lines.
+ * On 32-bit systems contents of this structure fits into one 64B line.
+ *
+ * The 'dev_private' field was placed in the first cache line to optimize
+ * performance because the PMD driver mainly depends on this field.
+ */
+struct rte_dma_fp_object {
+	void *dev_private; /**< PMD-specific private data. */
+	rte_dma_copy_t             copy;
+	rte_dma_copy_sg_t          copy_sg;
+	rte_dma_fill_t             fill;
+	rte_dma_submit_t           submit;
+	rte_dma_completed_t        completed;
+	rte_dma_completed_status_t completed_status;
+	void *reserved_cl0;
+	/** Reserve space for future IO functions, while keeping data and
+	 * dev_ops pointers on the second cacheline.
+	 */

This comment is out of date.

quoted

+	void *reserved_cl1[6];
+} __rte_cache_aligned;

Small suggestion: since there is no data at the end of the structure,
rather than adding in padding arrays which need to be adjusted as we add
fields into the struct, let's just change the "__rte_cache_aligned" macro
to "__rte_aligned(128)". This will explicitly set the size to 128-bytes and
allow us to remove the reserved fields - making it easier to add new
pointers.

Agree

quoted

+
+extern struct rte_dma_fp_object *rte_dma_fp_objs;
+
+#endif /* RTE_DMADEV_CORE_H */

.

Thanks