02/02/2023 21:26, Tyler Retzlaff: > On Thu, Feb 02, 2023 at 02:23:39PM -0500, Ben Magistro wrote: > > Hello, > > > > While making some updates to our code base for 22.11.1 that were missed in > > our first pass through, we hit the numa node change[1]. In the process of > > updating our code, we noticed that a couple functions (rx/tx_queue_setup, > > maybe more that we aren't using) state they accept `SOCKET_ID_ANY` but the > > function signature then asks for an unsigned integer while `SOCKET_ID_ANY` > > is `-1`. Following it through the redirect to the "real" function it also > > asks for an unsigned integer which is then passed on to one or more > > functions asking for an integer. As an example using the the i40e driver > > -- we would call `rte_eth_tx_queue_setup` [2] which ultimately calls > > `i40e_dev_tx_queue_setup`[3] which finally calls `rte_zmalloc_socket`[4] > > and `rte_eth_dma_zone_reserve`[5]. > > > > I guess what I am looking for is clarification on if this is intentional or > > if this is additional cleanup that may need to be completed/be desirable so > > that signs are maintained through the call paths and avoid potentially > > producing sign-conversion warnings. From the very quick glance I took at > > the i40e driver, it seems these are just passed through to other functions > > and no direct use/manipulation occurs (at least in the mentioned functions). > > i believe this is just sloppyness with sign in our api surface. i too > find it frustrating that use of these api force either explicit > casts or suffer having to suppress warnings. > > in the past examples of this have been cleaned up without full deprecation > notices but there are a lot of instances. i also feel (unpopular opinion) > that for some integer types like this that have constrained range / number > spaces it would be of value to introduce a typedef that can be used > consistently. > > for now you'll just have to add the casts and hopefully in the future we > will fix the api making them unnecessary. of course feel free to submit > patches too, it would be great to have these cleaned up.
I agree it should be cleaned up. Those IDs should accept negative values. Not sure which type we should choose (int, int32_t, or a typedef). Another thing to check is the name of the variable. It should be a socket ID when talking about CPU, and a NUMA node ID when talking about memory. And last but not the least, how can we keep ABI compatibility? I hope we can use function versioning to avoid deprecation and breaking. Trials and suggestions are welcome.
