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
