You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by "brbzull0 (via GitHub)" <gi...@apache.org> on 2023/06/23 14:14:37 UTC
[GitHub] [trafficserver] brbzull0 opened a new pull request, #9905: Add support for recvmmsg and UDP GRO
brbzull0 opened a new pull request, #9905:
URL: https://github.com/apache/trafficserver/pull/9905
**WIP**
I will leave this as draft as I still need to :
- Make another look to the code and improve some stuffs.
- Ammend some stuffs and run some more tests and update the description, I still need to tweak some stuffs but the idea would be this.
- Make sure I have all CI green in the meantime.
**WIP**
### Descrition
This PR adds support for UDP GRO when reading the socket using `recvm(m)gsg`. This PR also includes support for `recvmmsg` when it is available by the OS.
There is now a new configuration variable:
```yaml
# records.yaml
ts:
udp:
enable_gro: 1 # Enabled by default.
```
----
### Notes
All the tests were done using GET only and setting the h2load parameter to `--max-udp-payload-size=2k`.
|Feature |Total # Req | # Clients | # recvmmsg | # recvmsg |
|--------------|------------|-----------|------------|-----------|
| recvmmsg+gro | 100k | 100 | 22834 | 0 |
| recvmmsg | 100k | 100 | 23501 | 0 |
| recvmsg+gro | 100k | 100 | | 247970 |
| recvmmsg | 100k | 100 | | 249097 |
As I didn't see much of a difference between having GRO enabled or disabled I did run a test with debug
enabled and I was able to count how many packets came with the GRO set in the ancillary data.
Then I realized why I wasn't actually seeing much difference, not too many packets were spliced by the kernel.
Packets with (No GRO/GRO) = Number of times that ATS detected a packet with spliced with GRO.
|Feature |Total # Req | # Clients | # recvmmsg | # recvmsg | # Packets with (No GRO/GRO) | GRO % |
|--------------|------------|-----------|------------|-----------|---------------|-------|
| recvmmsg+gro | 100k | 100 | 13929 | 0 | 223080 / **180** | 0.8% |
| recvmsg+gro | 100k | 100 | 0| 278718 | 231760 / **34484** | 14% |
**Using recvmmsg reduced the number of syscall significantly**
This is what I used for this tests:
**bpftrace**
```bash
bpftrace -e 'tracepoint:syscalls:sys_enter_recvm* { @[comm, probe] = count(); }'
```
**h2load**
```bash
/opt/bin/h2load -n 100000 -c 100 --npn-list=h3 URL --max-udp-payload-size=2k
```
I was using only **GET** requests with around 15 header with about 80 bytes each.
I have not tried POST yet which I believe will give us better insights.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] brbzull0 commented on a diff in pull request #9905: Add support for recvmmsg and UDP GRO
Posted by "brbzull0 (via GitHub)" <gi...@apache.org>.
brbzull0 commented on code in PR #9905:
URL: https://github.com/apache/trafficserver/pull/9905#discussion_r1243953670
##########
iocore/net/UnixUDPNet.cc:
##########
@@ -326,61 +415,54 @@ UDPNetProcessorInternal::udp_read_from_net(UDPNetHandler *nh, UDPConnection *xuc
Debug("udp-read", "The UDP packet is truncated");
}
- // fill the IOBufferBlock chain
- int64_t saved = r;
- b = chain.get();
- while (b && saved > 0) {
- if (saved > buffer_size) {
- b->fill(buffer_size);
- saved -= buffer_size;
- b = b->next.get();
- } else {
- b->fill(saved);
- saved = 0;
- next_chain = b->next.get();
- b->next = nullptr;
- }
- }
-
safe_getsockname(xuc->getFd(), reinterpret_cast<struct sockaddr *>(&toaddr), &toaddr_len);
for (auto cmsg = CMSG_FIRSTHDR(&msg); cmsg != nullptr; cmsg = CMSG_NXTHDR(&msg, cmsg)) {
- switch (cmsg->cmsg_type) {
-#ifdef IP_PKTINFO
- case IP_PKTINFO:
- if (cmsg->cmsg_level == IPPROTO_IP) {
- struct in_pktinfo *pktinfo = reinterpret_cast<struct in_pktinfo *>(CMSG_DATA(cmsg));
- reinterpret_cast<sockaddr_in *>(&toaddr)->sin_addr.s_addr = pktinfo->ipi_addr.s_addr;
- }
+ if (get_ip_address_from_cmsg(cmsg, &toaddr)) {
break;
-#endif
-#ifdef IP_RECVDSTADDR
- case IP_RECVDSTADDR:
- if (cmsg->cmsg_level == IPPROTO_IP) {
- struct in_addr *addr = reinterpret_cast<struct in_addr *>(CMSG_DATA(cmsg));
- reinterpret_cast<sockaddr_in *>(&toaddr)->sin_addr.s_addr = addr->s_addr;
+ }
+#ifdef SOL_UDP
+ if (UDP_GRO == cmsg->cmsg_type) {
+ if (nh->is_gro_enabled()) {
+ gso_size = *reinterpret_cast<uint16_t *>(CMSG_DATA(cmsg));
}
break;
+ }
#endif
-#if defined(IPV6_PKTINFO) || defined(IPV6_RECVPKTINFO)
- case IPV6_PKTINFO: // IPV6_RECVPKTINFO uses IPV6_PKTINFO too
- if (cmsg->cmsg_level == IPPROTO_IPV6) {
- struct in6_pktinfo *pktinfo = reinterpret_cast<struct in6_pktinfo *>(CMSG_DATA(cmsg));
- memcpy(toaddr.sin6_addr.s6_addr, &pktinfo->ipi6_addr, 16);
+ }
+
+ // If we got the gso size, then we need to find out in how many parts this was spliced.
+ int const parts = gso_size ? static_cast<int>(ceil(static_cast<double>(r) / static_cast<double>(gso_size))) : 1;
Review Comment:
Not sure how bad would be using `ceil` just right here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt commented on pull request #9905: Add support for recvmmsg and UDP GRO
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on PR #9905:
URL: https://github.com/apache/trafficserver/pull/9905#issuecomment-1613150322
[approve ci clang-format]
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] JosiahWI commented on pull request #9905: Add support for recvmmsg and UDP GRO
Posted by "JosiahWI (via GitHub)" <gi...@apache.org>.
JosiahWI commented on PR #9905:
URL: https://github.com/apache/trafficserver/pull/9905#issuecomment-1607405229
> You mean this: https://github.com/apache/trafficserver/pull/9905/files#diff-49473dca262eeab3b4a43002adb08b4db31020d190caaad1594b47f1d5daa810R1393 ?
Yep. 😃
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] JosiahWI commented on pull request #9905: Add support for recvmmsg and UDP GRO
Posted by "JosiahWI (via GitHub)" <gi...@apache.org>.
JosiahWI commented on PR #9905:
URL: https://github.com/apache/trafficserver/pull/9905#issuecomment-1607435676
I took a closer look; it looks like we don't check this yet in CMake, but we probably should. We might want to make our own function:
```cmake
include(CheckIncludeFile)
function(REQUIRE_SYSTEM_HEADER HEADER)
check_include_file("${HEADER}" HAVE_HEADER)
if(NOT HAVE_HEADER)
message(FATAL_ERROR "Missing system header ${HEADER}")
endif()
endfunction()
# sendmsg, recvmsg, bunch of other stuff
require_system_header("sys/socket.h")
```
That's a little bigger change than I anticipated; if you don't want to do it in this PR I could do it afterwards.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] JosiahWI commented on pull request #9905: Add support for recvmmsg and UDP GRO
Posted by "JosiahWI (via GitHub)" <gi...@apache.org>.
JosiahWI commented on PR #9905:
URL: https://github.com/apache/trafficserver/pull/9905#issuecomment-1607449837
Sorry, I didn't read carefully, that only works for sendmsg and recvmsg. I think for recvmmsg and sendmmsg we need to check for the function, which I have not figured out how to do (autotools has its pros...). I would ignore this for now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] brbzull0 commented on a diff in pull request #9905: Add support for recvmmsg and UDP GRO
Posted by "brbzull0 (via GitHub)" <gi...@apache.org>.
brbzull0 commented on code in PR #9905:
URL: https://github.com/apache/trafficserver/pull/9905#discussion_r1246633794
##########
iocore/net/UnixUDPNet.cc:
##########
@@ -326,61 +415,54 @@ UDPNetProcessorInternal::udp_read_from_net(UDPNetHandler *nh, UDPConnection *xuc
Debug("udp-read", "The UDP packet is truncated");
}
- // fill the IOBufferBlock chain
- int64_t saved = r;
- b = chain.get();
- while (b && saved > 0) {
- if (saved > buffer_size) {
- b->fill(buffer_size);
- saved -= buffer_size;
- b = b->next.get();
- } else {
- b->fill(saved);
- saved = 0;
- next_chain = b->next.get();
- b->next = nullptr;
- }
- }
-
safe_getsockname(xuc->getFd(), reinterpret_cast<struct sockaddr *>(&toaddr), &toaddr_len);
for (auto cmsg = CMSG_FIRSTHDR(&msg); cmsg != nullptr; cmsg = CMSG_NXTHDR(&msg, cmsg)) {
- switch (cmsg->cmsg_type) {
-#ifdef IP_PKTINFO
- case IP_PKTINFO:
- if (cmsg->cmsg_level == IPPROTO_IP) {
- struct in_pktinfo *pktinfo = reinterpret_cast<struct in_pktinfo *>(CMSG_DATA(cmsg));
- reinterpret_cast<sockaddr_in *>(&toaddr)->sin_addr.s_addr = pktinfo->ipi_addr.s_addr;
- }
+ if (get_ip_address_from_cmsg(cmsg, &toaddr)) {
break;
-#endif
-#ifdef IP_RECVDSTADDR
- case IP_RECVDSTADDR:
- if (cmsg->cmsg_level == IPPROTO_IP) {
- struct in_addr *addr = reinterpret_cast<struct in_addr *>(CMSG_DATA(cmsg));
- reinterpret_cast<sockaddr_in *>(&toaddr)->sin_addr.s_addr = addr->s_addr;
+ }
+#ifdef SOL_UDP
+ if (UDP_GRO == cmsg->cmsg_type) {
+ if (nh->is_gro_enabled()) {
+ gso_size = *reinterpret_cast<uint16_t *>(CMSG_DATA(cmsg));
}
break;
+ }
#endif
-#if defined(IPV6_PKTINFO) || defined(IPV6_RECVPKTINFO)
- case IPV6_PKTINFO: // IPV6_RECVPKTINFO uses IPV6_PKTINFO too
- if (cmsg->cmsg_level == IPPROTO_IPV6) {
- struct in6_pktinfo *pktinfo = reinterpret_cast<struct in6_pktinfo *>(CMSG_DATA(cmsg));
- memcpy(toaddr.sin6_addr.s6_addr, &pktinfo->ipi6_addr, 16);
+ }
+
+ // If we got the gso size, then we need to find out in how many parts this was spliced.
+ int const parts = gso_size ? static_cast<int>(ceil(static_cast<double>(r) / static_cast<double>(gso_size))) : 1;
Review Comment:
We can have a remainder bigger than 0.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] brbzull0 commented on pull request #9905: Add support for recvmmsg and UDP GRO
Posted by "brbzull0 (via GitHub)" <gi...@apache.org>.
brbzull0 commented on PR #9905:
URL: https://github.com/apache/trafficserver/pull/9905#issuecomment-1623296265
[approve ci autest]
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on a diff in pull request #9905: Add support for recvmmsg and UDP GRO
Posted by "maskit (via GitHub)" <gi...@apache.org>.
maskit commented on code in PR #9905:
URL: https://github.com/apache/trafficserver/pull/9905#discussion_r1254586174
##########
iocore/net/UnixUDPNet.cc:
##########
@@ -37,20 +37,32 @@
#include "P_DNSConnection.h"
#include "P_Net.h"
#include "P_UDPNet.h"
+#include "tscore/ink_inet.h"
#include "tscore/ink_sock.h"
-
-#include "netinet/udp.h"
+#include <math.h>
Review Comment:
Do we still need math.h?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on a diff in pull request #9905: Add support for recvmmsg and UDP GRO
Posted by "maskit (via GitHub)" <gi...@apache.org>.
maskit commented on code in PR #9905:
URL: https://github.com/apache/trafficserver/pull/9905#discussion_r1245486241
##########
iocore/net/UnixUDPNet.cc:
##########
@@ -1452,13 +1709,22 @@ net_signal_hook_callback(EThread *thread)
#endif
}
-UDPNetHandler::UDPNetHandler(bool enable_gso) : udpOutQueue(enable_gso)
+UDPNetHandler::UDPNetHandler(Cfg &&cfg) : udpOutQueue(cfg.enable_gso), _cfg{std::move(cfg)}
{
nextCheck = Thread::get_hrtime_updated() + HRTIME_MSECONDS(1000);
lastCheck = 0;
SET_HANDLER(&UDPNetHandler::startNetEvent);
}
+bool
+UDPNetHandler::is_gro_enabled() const
+{
+#ifndef SOL_UDP
+ return false;
+#endif
+ return this->_cfg.enable_gro;
Review Comment:
It may be ok, but some analyzer may say this line is unreachable. I'd have this line in an else clause.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on a diff in pull request #9905: Add support for recvmmsg and UDP GRO
Posted by "maskit (via GitHub)" <gi...@apache.org>.
maskit commented on code in PR #9905:
URL: https://github.com/apache/trafficserver/pull/9905#discussion_r1245481534
##########
iocore/net/UnixUDPNet.cc:
##########
@@ -326,61 +415,54 @@ UDPNetProcessorInternal::udp_read_from_net(UDPNetHandler *nh, UDPConnection *xuc
Debug("udp-read", "The UDP packet is truncated");
}
- // fill the IOBufferBlock chain
- int64_t saved = r;
- b = chain.get();
- while (b && saved > 0) {
- if (saved > buffer_size) {
- b->fill(buffer_size);
- saved -= buffer_size;
- b = b->next.get();
- } else {
- b->fill(saved);
- saved = 0;
- next_chain = b->next.get();
- b->next = nullptr;
- }
- }
-
safe_getsockname(xuc->getFd(), reinterpret_cast<struct sockaddr *>(&toaddr), &toaddr_len);
for (auto cmsg = CMSG_FIRSTHDR(&msg); cmsg != nullptr; cmsg = CMSG_NXTHDR(&msg, cmsg)) {
- switch (cmsg->cmsg_type) {
-#ifdef IP_PKTINFO
- case IP_PKTINFO:
- if (cmsg->cmsg_level == IPPROTO_IP) {
- struct in_pktinfo *pktinfo = reinterpret_cast<struct in_pktinfo *>(CMSG_DATA(cmsg));
- reinterpret_cast<sockaddr_in *>(&toaddr)->sin_addr.s_addr = pktinfo->ipi_addr.s_addr;
- }
+ if (get_ip_address_from_cmsg(cmsg, &toaddr)) {
break;
-#endif
-#ifdef IP_RECVDSTADDR
- case IP_RECVDSTADDR:
- if (cmsg->cmsg_level == IPPROTO_IP) {
- struct in_addr *addr = reinterpret_cast<struct in_addr *>(CMSG_DATA(cmsg));
- reinterpret_cast<sockaddr_in *>(&toaddr)->sin_addr.s_addr = addr->s_addr;
+ }
+#ifdef SOL_UDP
+ if (UDP_GRO == cmsg->cmsg_type) {
+ if (nh->is_gro_enabled()) {
+ gso_size = *reinterpret_cast<uint16_t *>(CMSG_DATA(cmsg));
}
break;
+ }
#endif
-#if defined(IPV6_PKTINFO) || defined(IPV6_RECVPKTINFO)
- case IPV6_PKTINFO: // IPV6_RECVPKTINFO uses IPV6_PKTINFO too
- if (cmsg->cmsg_level == IPPROTO_IPV6) {
- struct in6_pktinfo *pktinfo = reinterpret_cast<struct in6_pktinfo *>(CMSG_DATA(cmsg));
- memcpy(toaddr.sin6_addr.s6_addr, &pktinfo->ipi6_addr, 16);
+ }
+
+ // If we got the gso size, then we need to find out in how many parts this was spliced.
+ int const parts = gso_size ? static_cast<int>(ceil(static_cast<double>(r) / static_cast<double>(gso_size))) : 1;
Review Comment:
If it's segmented it should always be divisible, right? Maybe `ink_assert(r % gso_size == 0)` instead?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] brbzull0 commented on pull request #9905: Add support for recvmmsg and UDP GRO
Posted by "brbzull0 (via GitHub)" <gi...@apache.org>.
brbzull0 commented on PR #9905:
URL: https://github.com/apache/trafficserver/pull/9905#issuecomment-1621670862
[approve ci autest]
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] JosiahWI commented on pull request #9905: Add support for recvmmsg and UDP GRO
Posted by "JosiahWI (via GitHub)" <gi...@apache.org>.
JosiahWI commented on PR #9905:
URL: https://github.com/apache/trafficserver/pull/9905#issuecomment-1604434062
The CMake build will need to be updated as well.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] brbzull0 commented on a diff in pull request #9905: Add support for recvmmsg and UDP GRO
Posted by "brbzull0 (via GitHub)" <gi...@apache.org>.
brbzull0 commented on code in PR #9905:
URL: https://github.com/apache/trafficserver/pull/9905#discussion_r1247880607
##########
iocore/net/UnixUDPNet.cc:
##########
@@ -326,61 +415,54 @@ UDPNetProcessorInternal::udp_read_from_net(UDPNetHandler *nh, UDPConnection *xuc
Debug("udp-read", "The UDP packet is truncated");
}
- // fill the IOBufferBlock chain
- int64_t saved = r;
- b = chain.get();
- while (b && saved > 0) {
- if (saved > buffer_size) {
- b->fill(buffer_size);
- saved -= buffer_size;
- b = b->next.get();
- } else {
- b->fill(saved);
- saved = 0;
- next_chain = b->next.get();
- b->next = nullptr;
- }
- }
-
safe_getsockname(xuc->getFd(), reinterpret_cast<struct sockaddr *>(&toaddr), &toaddr_len);
for (auto cmsg = CMSG_FIRSTHDR(&msg); cmsg != nullptr; cmsg = CMSG_NXTHDR(&msg, cmsg)) {
- switch (cmsg->cmsg_type) {
-#ifdef IP_PKTINFO
- case IP_PKTINFO:
- if (cmsg->cmsg_level == IPPROTO_IP) {
- struct in_pktinfo *pktinfo = reinterpret_cast<struct in_pktinfo *>(CMSG_DATA(cmsg));
- reinterpret_cast<sockaddr_in *>(&toaddr)->sin_addr.s_addr = pktinfo->ipi_addr.s_addr;
- }
+ if (get_ip_address_from_cmsg(cmsg, &toaddr)) {
break;
-#endif
-#ifdef IP_RECVDSTADDR
- case IP_RECVDSTADDR:
- if (cmsg->cmsg_level == IPPROTO_IP) {
- struct in_addr *addr = reinterpret_cast<struct in_addr *>(CMSG_DATA(cmsg));
- reinterpret_cast<sockaddr_in *>(&toaddr)->sin_addr.s_addr = addr->s_addr;
+ }
+#ifdef SOL_UDP
+ if (UDP_GRO == cmsg->cmsg_type) {
+ if (nh->is_gro_enabled()) {
+ gso_size = *reinterpret_cast<uint16_t *>(CMSG_DATA(cmsg));
}
break;
+ }
#endif
-#if defined(IPV6_PKTINFO) || defined(IPV6_RECVPKTINFO)
- case IPV6_PKTINFO: // IPV6_RECVPKTINFO uses IPV6_PKTINFO too
- if (cmsg->cmsg_level == IPPROTO_IPV6) {
- struct in6_pktinfo *pktinfo = reinterpret_cast<struct in6_pktinfo *>(CMSG_DATA(cmsg));
- memcpy(toaddr.sin6_addr.s6_addr, &pktinfo->ipi6_addr, 16);
+ }
+
+ // If we got the gso size, then we need to find out in how many parts this was spliced.
+ int const parts = gso_size ? static_cast<int>(ceil(static_cast<double>(r) / static_cast<double>(gso_size))) : 1;
Review Comment:
> Oh, you mean `r` can be 2500 (1000 + 1000 + 500) where `gso_size` is 1000?
>
Exactly.
> I don't have any better idea to calculate parts faster.
> https://quick-bench.com/q/IC9D3TNEcLCwWgEfoh_Ll3igs2k
does not seems to change much I think.
> Another approach would be get rid of parts. Maybe we can subtract gso_size from r on every iteration instead (gso_size > would need to be set to r if gso_size is 0)?
I could probably do that I think.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] brbzull0 commented on pull request #9905: Add support for recvmmsg and UDP GRO
Posted by "brbzull0 (via GitHub)" <gi...@apache.org>.
brbzull0 commented on PR #9905:
URL: https://github.com/apache/trafficserver/pull/9905#issuecomment-1613279718
[approve ci autest]
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on a diff in pull request #9905: Add support for recvmmsg and UDP GRO
Posted by "maskit (via GitHub)" <gi...@apache.org>.
maskit commented on code in PR #9905:
URL: https://github.com/apache/trafficserver/pull/9905#discussion_r1246902688
##########
iocore/net/UnixUDPNet.cc:
##########
@@ -326,61 +415,54 @@ UDPNetProcessorInternal::udp_read_from_net(UDPNetHandler *nh, UDPConnection *xuc
Debug("udp-read", "The UDP packet is truncated");
}
- // fill the IOBufferBlock chain
- int64_t saved = r;
- b = chain.get();
- while (b && saved > 0) {
- if (saved > buffer_size) {
- b->fill(buffer_size);
- saved -= buffer_size;
- b = b->next.get();
- } else {
- b->fill(saved);
- saved = 0;
- next_chain = b->next.get();
- b->next = nullptr;
- }
- }
-
safe_getsockname(xuc->getFd(), reinterpret_cast<struct sockaddr *>(&toaddr), &toaddr_len);
for (auto cmsg = CMSG_FIRSTHDR(&msg); cmsg != nullptr; cmsg = CMSG_NXTHDR(&msg, cmsg)) {
- switch (cmsg->cmsg_type) {
-#ifdef IP_PKTINFO
- case IP_PKTINFO:
- if (cmsg->cmsg_level == IPPROTO_IP) {
- struct in_pktinfo *pktinfo = reinterpret_cast<struct in_pktinfo *>(CMSG_DATA(cmsg));
- reinterpret_cast<sockaddr_in *>(&toaddr)->sin_addr.s_addr = pktinfo->ipi_addr.s_addr;
- }
+ if (get_ip_address_from_cmsg(cmsg, &toaddr)) {
break;
-#endif
-#ifdef IP_RECVDSTADDR
- case IP_RECVDSTADDR:
- if (cmsg->cmsg_level == IPPROTO_IP) {
- struct in_addr *addr = reinterpret_cast<struct in_addr *>(CMSG_DATA(cmsg));
- reinterpret_cast<sockaddr_in *>(&toaddr)->sin_addr.s_addr = addr->s_addr;
+ }
+#ifdef SOL_UDP
+ if (UDP_GRO == cmsg->cmsg_type) {
+ if (nh->is_gro_enabled()) {
+ gso_size = *reinterpret_cast<uint16_t *>(CMSG_DATA(cmsg));
}
break;
+ }
#endif
-#if defined(IPV6_PKTINFO) || defined(IPV6_RECVPKTINFO)
- case IPV6_PKTINFO: // IPV6_RECVPKTINFO uses IPV6_PKTINFO too
- if (cmsg->cmsg_level == IPPROTO_IPV6) {
- struct in6_pktinfo *pktinfo = reinterpret_cast<struct in6_pktinfo *>(CMSG_DATA(cmsg));
- memcpy(toaddr.sin6_addr.s6_addr, &pktinfo->ipi6_addr, 16);
+ }
+
+ // If we got the gso size, then we need to find out in how many parts this was spliced.
+ int const parts = gso_size ? static_cast<int>(ceil(static_cast<double>(r) / static_cast<double>(gso_size))) : 1;
Review Comment:
Oh, you mean `r` can be 2500 (1000 + 1000 + 500) where `gso_size` is 1000?
I don't have any better idea to calculate `parts` faster.
https://quick-bench.com/q/IC9D3TNEcLCwWgEfoh_Ll3igs2k
Another approach would be get rid of `parts`. Maybe we can subtract `gso_size` from `r` on every iteration instead (gso_size would need to be set to `r` if `gso_size` is 0)?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] brbzull0 merged pull request #9905: Add support for recvmmsg and UDP GRO
Posted by "brbzull0 (via GitHub)" <gi...@apache.org>.
brbzull0 merged PR #9905:
URL: https://github.com/apache/trafficserver/pull/9905
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] brbzull0 commented on pull request #9905: Add support for recvmmsg and UDP GRO
Posted by "brbzull0 (via GitHub)" <gi...@apache.org>.
brbzull0 commented on PR #9905:
URL: https://github.com/apache/trafficserver/pull/9905#issuecomment-1607266424
> The CMake build will need to be updated as well.
Thanks for the comment.
You mean this: https://github.com/apache/trafficserver/pull/9905/files#diff-49473dca262eeab3b4a43002adb08b4db31020d190caaad1594b47f1d5daa810R1393 ?
Cheers.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] brbzull0 commented on a diff in pull request #9905: Add support for recvmmsg and UDP GRO
Posted by "brbzull0 (via GitHub)" <gi...@apache.org>.
brbzull0 commented on code in PR #9905:
URL: https://github.com/apache/trafficserver/pull/9905#discussion_r1245596903
##########
iocore/net/UnixUDPNet.cc:
##########
@@ -1452,13 +1709,22 @@ net_signal_hook_callback(EThread *thread)
#endif
}
-UDPNetHandler::UDPNetHandler(bool enable_gso) : udpOutQueue(enable_gso)
+UDPNetHandler::UDPNetHandler(Cfg &&cfg) : udpOutQueue(cfg.enable_gso), _cfg{std::move(cfg)}
{
nextCheck = Thread::get_hrtime_updated() + HRTIME_MSECONDS(1000);
lastCheck = 0;
SET_HANDLER(&UDPNetHandler::startNetEvent);
}
+bool
+UDPNetHandler::is_gro_enabled() const
+{
+#ifndef SOL_UDP
+ return false;
+#endif
+ return this->_cfg.enable_gro;
Review Comment:
This is ugly, I meant to have an `else` here. Thanks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] brbzull0 commented on pull request #9905: Add support for recvmmsg and UDP GRO
Posted by "brbzull0 (via GitHub)" <gi...@apache.org>.
brbzull0 commented on PR #9905:
URL: https://github.com/apache/trafficserver/pull/9905#issuecomment-1623321846
removed the `ceil` function and the parts handling, it now relies on the received size and work out with the remining size based on each part/packet size (if gro).
cc @maskit
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] brbzull0 commented on a diff in pull request #9905: Add support for recvmmsg and UDP GRO
Posted by "brbzull0 (via GitHub)" <gi...@apache.org>.
brbzull0 commented on code in PR #9905:
URL: https://github.com/apache/trafficserver/pull/9905#discussion_r1254587819
##########
iocore/net/UnixUDPNet.cc:
##########
@@ -37,20 +37,32 @@
#include "P_DNSConnection.h"
#include "P_Net.h"
#include "P_UDPNet.h"
+#include "tscore/ink_inet.h"
#include "tscore/ink_sock.h"
-
-#include "netinet/udp.h"
+#include <math.h>
Review Comment:
defintelly not. Thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org