On 2021/9/9 20:45, Bruce Richardson wrote:
> On Thu, Sep 09, 2021 at 01:29:33PM +0200, Thomas Monjalon wrote:
>> 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.
>>
>
> 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.
>
>>>>> + *
>>>>> + * 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?
>>
>
> It can be added, sure.
>
>>>>> +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 ?
>
>>>>> +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.
>>
> .
>
