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