Re: [dpdk-dev] [PATCH v21 1/7] dmadev: introduce DMA device library public APIs

[Bruce Richardson <bruce.richardson@intel.com>]

On Thu, Sep 09, 2021 at 04:26:40PM +0200, Thomas Monjalon wrote:
> 09/09/2021 15:54, fengchengwen:
> > 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.
> 
> Not sure to understand what you mean.
> Please could you explain what does not work and what is the perf impact?
> I guess you want unsigned index for rings, then OK.

Yes, that is it.

> For device ID however, I believe signed integer is useful.

No objection to that.

> 
> [...]
> > >>>>> +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_*
> 
> Quite often, we skip the eal_ prefix, that's why I was thinking about
> a possible namespace conflict. Anyway it could be managed.
> 
> > > 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).
> 
> Yes OK for rte_dma_ prefix everywhere.
> 
> > Also should the filename(e.g. rte_dmadev.h) and directory-name(lib/dmadev) also change ?
> 
> I believe it's better to keep dmadev as name of the lib and filename,
> so it's consistent with other device classes.
> What are the other opinions?

Definitely keep. It's one thing to have additional characters in the header
name, another to have them in the APIs.

/Bruce