Already fix in V21, thanks

On 2021/9/6 21:35, Bruce Richardson wrote:

On Sat, Sep 04, 2021 at 06:10:22PM +0800, Chengwen Feng wrote:

quoted

This patch introduce DMA device library internal header, which contains
internal data types that are used by the DMA devices in order to expose
their ops to the class.

Signed-off-by: Chengwen Feng <redacted>
Acked-by: Bruce Richardson <redacted>
Acked-by: Morten Brørup <redacted>
Reviewed-by: Kevin Laatz <redacted>
Reviewed-by: Conor Walsh <redacted>
---

<snip>

quoted

+struct rte_dmadev {
+	void *dev_private;
+	/**< PMD-specific private data.
+	 *
+	 * - If is the primary process, after dmadev allocated by
+	 * rte_dmadev_pmd_allocate(), the PCI/SoC device probing should
+	 * initialize this field, and copy it's value to the 'dev_private'
+	 * field of 'struct rte_dmadev_data' which pointer by 'data' filed.
+	 *
+	 * - If is the secondary process, dmadev framework will initialize this
+	 * field by copy from 'dev_private' field of 'struct rte_dmadev_data'
+	 * which initialized by primary process.
+	 *
+	 * @note It's the primary process responsibility to deinitialize this
+	 * field after invoke rte_dmadev_pmd_release() in the PCI/SoC device
+	 * removing stage.
+	 */
+	rte_dmadev_copy_t             copy;
+	rte_dmadev_copy_sg_t          copy_sg;
+	rte_dmadev_fill_t             fill;
+	rte_dmadev_submit_t           submit;
+	rte_dmadev_completed_t        completed;
+	rte_dmadev_completed_status_t completed_status;
+	void *reserved_ptr[7]; /**< Reserved for future IO function. */

This is new in this set, I think. I assume that 7 was chosen so that we
have the "data" pointer and the "dev_ops" pointers on the second cacheline
(if 64-byte CLs)? If so, I wonder if we can find a good way to express that
in the code or in the comments?

The simplest - and probably as clear as any - is to split this into
"void *__reserved_cl0" and "void *__reserved_cl1[6]" to show that it is
split across the two cachelines, with the latter having comment:
"Reserve space for future IO functions, while keeping data and dev_ops
pointers on the second cacheline"

If we don't mind using a slightly different type the magic "6" could be
changed to a computation:
char __reserved_cl1[RTE_CACHELINE_SZ - sizeof(void *) * 2];

However, for simplicity, I think the magic 6 can be kept, and just split
into reserved_cl0 and reserved_cl1 as I suggest above.

/Bruce

.