Specify queue id as a configuration parameter #2009
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #2009 +/- ##
========================================
Coverage 83.46% 83.46%
========================================
Files 311 312 +1
Lines 54556 54606 +50
Branches 11491 11821 +330
========================================
+ Hits 45534 45576 +42
- Misses 7795 7798 +3
- Partials 1227 1232 +5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Pcap++/header/XdpDevice.h
Outdated
| XdpDeviceStats getStatistics(); | ||
|
|
||
| /// @return Return queue identifier for underlying socket | ||
| uint32_t getQueueId() |
There was a problem hiding this comment.
nit: I think this method can be marked as const?
Pcap++/src/XdpDevice.cpp
Outdated
| uint32_t XdpDevice::getNumQueues(const std::string& iface, bool tx) | ||
| { | ||
| // returns number of hardware queues associated with the device | ||
| uint32_t rxtxqueues = 0; |
There was a problem hiding this comment.
nit: can we rename this to numQueues?
| { | ||
| std::regex rxtx_regex("^" + prefix + "[0-9]+$"); | ||
|
|
||
| struct dirent* entry; |
There was a problem hiding this comment.
nit: Curious why the struct dirent*? Can't we just do dirent*?
|
@JasMetzger, please notice that clang-format fails in CI |
|
Sorry for the delay--- I was out of town. Where is this test failing? |
@JasMetzger you can see the pre-commit build that failed: https://github.com/seladb/PcapPlusPlus/actions/runs/19214326473/job/54921310413?pr=2009 There are trailing white-spaces and formatting issues. To fix formatting, you should run clang-format as mentioned in https://github.com/seladb/PcapPlusPlus/blob/master/CONTRIBUTING.md |
|
When I run the pre-commit, there are errors in files that I did not modify. Shall I address those? |
@JasMetzger there shouldn't be errors in files you did not modify, please avoid updating them in this PR |
|
@JasMetzger should we move this PR to DRAFT until it's ready for review? |
|
When I do a clean make, there are warnings in files I haven't touched, so not sure what to do about that code. The first warning is freebsd running bash but I don't understand how it is failing BSD or what it is doing. Do I need to create a FreeBSD environment? Is it compiling it? I think I need some help with the CI. I got it to pass clang. |
|
There might be issues with the FreeBSD tests, I need to look into it... 😕 |
Pcap++/header/XdpDevice.h
Outdated
| /// @ | ||
| namespace pcpp | ||
| { | ||
| #define XDP_MAX_RXTX_QUEUES 16 |
There was a problem hiding this comment.
This const isn't used anywhere, should we remove it?
There was a problem hiding this comment.
I will remove. I think at first I was simply setting a hard limit before I created a function that can detect it. If the function cannot detect it, it returns 0 which may not be desirable. Maybe it should return 1 if nothing else. That will be the identical behavior in the release version.
There was a problem hiding this comment.
I made the default number 1. So a qid of zero, the default, and as things worked before, will pass the test.
| xskConfig.rx_size = m_Config->rxSize; | ||
| xskConfig.tx_size = m_Config->txSize; |
There was a problem hiding this comment.
Will that be a problem if I leave it like this? A merge issue?
Pcap++/header/XdpDevice.h
Outdated
| /// @param[in] interfaceName The interface name to use to detect hardware queues | ||
| /// @param[in] tx If true, return TX queues, otherwise RX. Default is false | ||
| /// @return The number of hardware queues associated with the device. | ||
| static uint32_t numQueues(const std::string& interfaceName, bool tx = false); |
There was a problem hiding this comment.
Why is this a public method? Shouldn't it be private?
There was a problem hiding this comment.
It may be useful for configuring the user space, such as limiting the number of threads and resources.
Pcap++/src/XdpDevice.cpp
Outdated
| return false; | ||
| } | ||
|
|
||
| unsigned int nhwqueues = numQueues(m_InterfaceName); |
There was a problem hiding this comment.
nit: let's rename to numOfHardwareQueues?
|
Redirecting a packet to a particular rxqueue and associated socket requires a "map" between kernel and userspace. Entries in the map are created on the user space side using the socket's file descriptor. Unless there is a different way, I am considering a function in XdpDevice that passes in a pointer to int for the file descriptor to be written. The function returns a bool, true if the fd was set ok, false otherwise. |
@JasMetzger do you have a code example somewhere that shows how it should be done in AF_XDP? |
3 participants
The queue id can be specified as an additional configuration parameter. It is checked against the maximum queues available by the device using a utility provided as a static function. In the open() function, the configured queue id is used in place of the hardwired zero value that was used before. Added a convenience function to obtain the queue id from device.
Community may consider renaming the XdpDevice to XdpSocketDevice as each instance of the XdpDevice class pertains to a single queue id / socket.