On 2021/10/11 18:40, Bruce Richardson wrote:
> On Sat, Oct 09, 2021 at 05:33:37PM +0800, Chengwen Feng wrote:
>> 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
>
>> 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.
>
>> struct rte_dma_dev *rte_dma_devices;
>>
>
> <snip>
>
>> +/**
>> + * @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.
>
>> + 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
>
>> +
>> +extern struct rte_dma_fp_object *rte_dma_fp_objs;
>> +
>> +#endif /* RTE_DMADEV_CORE_H */
>
> .
>
Thanks
