* [PATCH 0/2] Add af_inet mode besides af_unix and pipe
@ 2022-04-28 10:37 Yihao Wu
2022-04-28 10:37 ` [PATCH 1/2] hackbench: Fix negativity opt checking Yihao Wu
2022-04-28 10:37 ` [PATCH 2/2] hackbench: Add af_inet mode besides af_unix and pipe Yihao Wu
0 siblings, 2 replies; 10+ messages in thread
From: Yihao Wu @ 2022-04-28 10:37 UTC (permalink / raw)
To: Clark Williams, John Kacur; +Cc: linux-rt-users
Yihao Wu (2):
hackbench: Fix negativity opt checking
hackbench: Add af_inet mode besides af_unix and pipe
src/hackbench/hackbench.c | 73 ++++++++++++++++++++++++++++++++++-----
1 file changed, 64 insertions(+), 9 deletions(-)
--
2.18.2
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 1/2] hackbench: Fix negativity opt checking
2022-04-28 10:37 [PATCH 0/2] Add af_inet mode besides af_unix and pipe Yihao Wu
@ 2022-04-28 10:37 ` Yihao Wu
2022-05-09 18:59 ` John Kacur
2022-04-28 10:37 ` [PATCH 2/2] hackbench: Add af_inet mode besides af_unix and pipe Yihao Wu
1 sibling, 1 reply; 10+ messages in thread
From: Yihao Wu @ 2022-04-28 10:37 UTC (permalink / raw)
To: Clark Williams, John Kacur; +Cc: linux-rt-users
It was easy to escape the checking. For example, run
./hackbench --datasize 4096 -g -1 -l -1
This patch fixes the checking.
Signed-off-by: Yihao Wu <wuyihao@linux.alibaba.com>
---
src/hackbench/hackbench.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/src/hackbench/hackbench.c b/src/hackbench/hackbench.c
index 268c232..b9b0af6 100644
--- a/src/hackbench/hackbench.c
+++ b/src/hackbench/hackbench.c
@@ -31,10 +31,10 @@
#include <setjmp.h>
#include <sched.h>
-static unsigned int datasize = 100;
-static unsigned int loops = 100;
-static unsigned int num_groups = 10;
-static unsigned int num_fds = 20;
+static int datasize = 100;
+static int loops = 100;
+static int num_groups = 10;
+static int num_fds = 20;
static unsigned int fifo = 0;
/*
@@ -377,7 +377,7 @@ static void process_options(int argc, char *argv[])
}
switch (c) {
case 'f':
- if (!(argv[optind] && (num_fds = atoi(optarg)) > 0)) {
+ if ((num_fds = atoi(optarg)) <= 0) {
fprintf(stderr, "%s: --fds|-f requires an integer > 0\n", argv[0]);
print_usage_exit(1);
}
@@ -386,7 +386,7 @@ static void process_options(int argc, char *argv[])
fifo = 1;
break;
case 'g':
- if (!(argv[optind] && (num_groups = atoi(optarg)) > 0)) {
+ if ((num_groups = atoi(optarg)) <= 0) {
fprintf(stderr, "%s: --groups|-g requires an integer > 0\n", argv[0]);
print_usage_exit(1);
}
@@ -394,7 +394,7 @@ static void process_options(int argc, char *argv[])
case 'h':
print_usage_exit(0);
case 'l':
- if (!(argv[optind] && (loops = atoi(optarg)) > 0)) {
+ if ((loops = atoi(optarg)) <= 0) {
fprintf(stderr, "%s: --loops|-l requires an integer > 0\n", argv[0]);
print_usage_exit(1);
}
@@ -403,7 +403,7 @@ static void process_options(int argc, char *argv[])
use_pipes = 1;
break;
case 's':
- if (!(argv[optind] && (datasize = atoi(optarg)) > 0)) {
+ if ((datasize = atoi(optarg)) <= 0) {
fprintf(stderr, "%s: --datasize|-s requires an integer > 0\n", argv[0]);
print_usage_exit(1);
}
--
2.18.2
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 2/2] hackbench: Add af_inet mode besides af_unix and pipe
2022-04-28 10:37 [PATCH 0/2] Add af_inet mode besides af_unix and pipe Yihao Wu
2022-04-28 10:37 ` [PATCH 1/2] hackbench: Fix negativity opt checking Yihao Wu
@ 2022-04-28 10:37 ` Yihao Wu
2022-05-09 19:05 ` John Kacur
2022-05-23 15:44 ` [PATCH 2/2] " Yihao Wu
1 sibling, 2 replies; 10+ messages in thread
From: Yihao Wu @ 2022-04-28 10:37 UTC (permalink / raw)
To: Clark Williams, John Kacur; +Cc: linux-rt-users
We observerved strange performance regression with hackbench, but only with
its default mode instead of pipe mode. It turned out it was unix domain
socket to blame rather than the scheduler. Because on older kernels, there
was a big lock. And modifications to the scheduler made the contension
worse.
However pipe mode is much different from the default af_unix mode. So it's
hard to prove it's really the case. So we think it might be a good idea to
have af_inet mode for hackbench.
After implementing it, we compare these three modes. And the result shows
as what we expect, that only af_unix mode has regression, the others don't.
This proves adding af_inet mode is valuable.
Signed-off-by: Yihao Wu <wuyihao@linux.alibaba.com>
---
src/hackbench/hackbench.c | 57 ++++++++++++++++++++++++++++++++++++++-
1 file changed, 56 insertions(+), 1 deletion(-)
diff --git a/src/hackbench/hackbench.c b/src/hackbench/hackbench.c
index b9b0af6..030342f 100644
--- a/src/hackbench/hackbench.c
+++ b/src/hackbench/hackbench.c
@@ -20,11 +20,14 @@
#include <string.h>
#include <errno.h>
#include <unistd.h>
+#include <sys/ioctl.h>
#include <sys/types.h>
#include <sys/socket.h>
#include <sys/wait.h>
#include <sys/time.h>
#include <sys/poll.h>
+#include <netinet/in.h>
+#include <arpa/inet.h>
#include <limits.h>
#include <getopt.h>
#include <signal.h>
@@ -46,6 +49,7 @@ static unsigned int fifo = 0;
static unsigned int process_mode = PROCESS_MODE;
static int use_pipes = 0;
+static int use_inet = 0;
struct sender_context {
unsigned int num_fds;
@@ -99,6 +103,7 @@ static void print_usage_exit(int error)
"-h --help print this message\n"
"-l --loops=LOOPS how many message should be send\n"
"-p --pipe send data via a pipe\n"
+ "-i --inet send data via a inet tcp connection\n"
"-s --datasize=SIZE message size\n"
"-T --threads use POSIX threads\n"
"-P --process use fork (default)\n"
@@ -106,11 +111,52 @@ static void print_usage_exit(int error)
exit(error);
}
+static int inet_socketpair(int fds[2])
+{
+ int s1, s2;
+ struct sockaddr_in sin;
+ unsigned long ul = 1;
+
+ if ((s1 = socket(AF_INET, SOCK_STREAM, 0)) < 0)
+ barf("socket");
+ if ((s2 = socket(AF_INET, SOCK_STREAM, 0)) < 0)
+ barf("socket");
+
+ socklen_t len = sizeof(sin);
+ bzero(&sin, len);
+ sin.sin_family = AF_INET;
+ sin.sin_port = 0;
+ sin.sin_addr.s_addr = inet_addr("127.0.0.1");
+
+ if (bind(s1, &sin, len) < 0)
+ barf("bind");
+ getsockname(s1, &sin, &len);
+ if (listen(s1, 10) < 0)
+ barf("listen");
+ if (ioctl(s2, FIONBIO, &ul) < 0)
+ barf("ioctl");
+ if (ioctl(s1, FIONBIO, &ul) < 0)
+ barf("ioctl");
+ connect(s2, &sin, len);
+ if ((fds[0] = accept(s1, &sin, &len)) < 0)
+ barf("accept");
+ ul = 0;
+ if (ioctl(s2, FIONBIO, &ul) < 0)
+ barf("ioctl");
+ fds[1] = s2;
+ close(s1);
+
+ return 0;
+}
+
static void fdpair(int fds[2])
{
if (use_pipes) {
if (pipe(fds) == 0)
return;
+ } else if (use_inet) {
+ if (inet_socketpair(fds) == 0)
+ return;
} else {
if (socketpair(AF_UNIX, SOCK_STREAM, 0, fds) == 0)
return;
@@ -364,13 +410,14 @@ static void process_options(int argc, char *argv[])
{"help", no_argument, NULL, 'h'},
{"loops", required_argument, NULL, 'l'},
{"pipe", no_argument, NULL, 'p'},
+ {"inet", no_argument, NULL, 'i'},
{"datasize", required_argument, NULL, 's'},
{"threads", no_argument, NULL, 'T'},
{"processes", no_argument, NULL, 'P'},
{NULL, 0, NULL, 0}
};
- int c = getopt_long(argc, argv, "f:Fg:hl:ps:TP",
+ int c = getopt_long(argc, argv, "f:Fg:hl:pis:TP",
longopts, &optind);
if (c == -1) {
break;
@@ -402,6 +449,9 @@ static void process_options(int argc, char *argv[])
case 'p':
use_pipes = 1;
break;
+ case 'i':
+ use_inet = 1;
+ break;
case 's':
if ((datasize = atoi(optarg)) <= 0) {
fprintf(stderr, "%s: --datasize|-s requires an integer > 0\n", argv[0]);
@@ -418,6 +468,11 @@ static void process_options(int argc, char *argv[])
print_usage_exit(1);
}
}
+
+ if (use_pipes && use_inet) {
+ fprintf(stderr, "%s: --pipe|-p and --inet|-i cannot be used together\n", argv[0]);
+ print_usage_exit(1);
+ }
}
void sigcatcher(int sig) {
--
2.18.2
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] hackbench: Fix negativity opt checking
2022-04-28 10:37 ` [PATCH 1/2] hackbench: Fix negativity opt checking Yihao Wu
@ 2022-05-09 18:59 ` John Kacur
2022-05-23 15:39 ` Yihao Wu
0 siblings, 1 reply; 10+ messages in thread
From: John Kacur @ 2022-05-09 18:59 UTC (permalink / raw)
To: Yihao Wu; +Cc: Clark Williams, linux-rt-users
On Thu, 28 Apr 2022, Yihao Wu wrote:
> It was easy to escape the checking. For example, run
>
> ./hackbench --datasize 4096 -g -1 -l -1
>
> This patch fixes the checking.
>
> Signed-off-by: Yihao Wu <wuyihao@linux.alibaba.com>
> ---
> src/hackbench/hackbench.c | 16 ++++++++--------
> 1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/src/hackbench/hackbench.c b/src/hackbench/hackbench.c
> index 268c232..b9b0af6 100644
> --- a/src/hackbench/hackbench.c
> +++ b/src/hackbench/hackbench.c
> @@ -31,10 +31,10 @@
> #include <setjmp.h>
> #include <sched.h>
>
> -static unsigned int datasize = 100;
> -static unsigned int loops = 100;
> -static unsigned int num_groups = 10;
> -static unsigned int num_fds = 20;
> +static int datasize = 100;
> +static int loops = 100;
> +static int num_groups = 10;
> +static int num_fds = 20;
> static unsigned int fifo = 0;
>
> /*
> @@ -377,7 +377,7 @@ static void process_options(int argc, char *argv[])
> }
> switch (c) {
> case 'f':
> - if (!(argv[optind] && (num_fds = atoi(optarg)) > 0)) {
> + if ((num_fds = atoi(optarg)) <= 0) {
> fprintf(stderr, "%s: --fds|-f requires an integer > 0\n", argv[0]);
> print_usage_exit(1);
> }
> @@ -386,7 +386,7 @@ static void process_options(int argc, char *argv[])
> fifo = 1;
> break;
> case 'g':
> - if (!(argv[optind] && (num_groups = atoi(optarg)) > 0)) {
> + if ((num_groups = atoi(optarg)) <= 0) {
> fprintf(stderr, "%s: --groups|-g requires an integer > 0\n", argv[0]);
> print_usage_exit(1);
> }
> @@ -394,7 +394,7 @@ static void process_options(int argc, char *argv[])
> case 'h':
> print_usage_exit(0);
> case 'l':
> - if (!(argv[optind] && (loops = atoi(optarg)) > 0)) {
> + if ((loops = atoi(optarg)) <= 0) {
> fprintf(stderr, "%s: --loops|-l requires an integer > 0\n", argv[0]);
> print_usage_exit(1);
> }
> @@ -403,7 +403,7 @@ static void process_options(int argc, char *argv[])
> use_pipes = 1;
> break;
> case 's':
> - if (!(argv[optind] && (datasize = atoi(optarg)) > 0)) {
> + if ((datasize = atoi(optarg)) <= 0) {
> fprintf(stderr, "%s: --datasize|-s requires an integer > 0\n", argv[0]);
> print_usage_exit(1);
> }
> --
> 2.18.2
>
>
Well, the code doesn't work the way the author intended, that's for sure.
As your patch indicates, you realize the problem is that an unsigned
variable can never be less than zero, so the check is meaningless.
However, is changing the data type the right solution, just to make a
check against something stupid a user might input the correct thing to do?
It decreases the size of valid input. Maybe that doesn't matter, but then
could it introduce a new bug somewhere? You've dropped the argv[optind]
too which also doesn't work the way the author intended, but it was
probably meant as a check of whether the user provided an argument or not.
What I would rather see if you want to fix this, is a check against the
user input that doesn't change the datatype. For example, if you treat the
input as a char, if (optarg[0] == '-') then we can tell the difference
between a really big number or user input that is stupid or malicious.
If you look in cyclictest, (there could be bugs lurking there too), there
is an attempt to parse this kind of thing and to check whether there are
arguments too.
Thanks
John
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] hackbench: Add af_inet mode besides af_unix and pipe
2022-04-28 10:37 ` [PATCH 2/2] hackbench: Add af_inet mode besides af_unix and pipe Yihao Wu
@ 2022-05-09 19:05 ` John Kacur
2022-05-23 15:40 ` [PATCH v2] " Yihao Wu
2022-05-23 15:44 ` [PATCH 2/2] " Yihao Wu
1 sibling, 1 reply; 10+ messages in thread
From: John Kacur @ 2022-05-09 19:05 UTC (permalink / raw)
To: Yihao Wu; +Cc: Clark Williams, linux-rt-users
On Thu, 28 Apr 2022, Yihao Wu wrote:
> We observerved strange performance regression with hackbench, but only with
> its default mode instead of pipe mode. It turned out it was unix domain
> socket to blame rather than the scheduler. Because on older kernels, there
> was a big lock. And modifications to the scheduler made the contension
> worse.
>
> However pipe mode is much different from the default af_unix mode. So it's
> hard to prove it's really the case. So we think it might be a good idea to
> have af_inet mode for hackbench.
>
> After implementing it, we compare these three modes. And the result shows
> as what we expect, that only af_unix mode has regression, the others don't.
> This proves adding af_inet mode is valuable.
>
> Signed-off-by: Yihao Wu <wuyihao@linux.alibaba.com>
> ---
> src/hackbench/hackbench.c | 57 ++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 56 insertions(+), 1 deletion(-)
>
> diff --git a/src/hackbench/hackbench.c b/src/hackbench/hackbench.c
> index b9b0af6..030342f 100644
> --- a/src/hackbench/hackbench.c
> +++ b/src/hackbench/hackbench.c
> @@ -20,11 +20,14 @@
> #include <string.h>
> #include <errno.h>
> #include <unistd.h>
> +#include <sys/ioctl.h>
> #include <sys/types.h>
> #include <sys/socket.h>
> #include <sys/wait.h>
> #include <sys/time.h>
> #include <sys/poll.h>
> +#include <netinet/in.h>
> +#include <arpa/inet.h>
> #include <limits.h>
> #include <getopt.h>
> #include <signal.h>
> @@ -46,6 +49,7 @@ static unsigned int fifo = 0;
> static unsigned int process_mode = PROCESS_MODE;
>
> static int use_pipes = 0;
> +static int use_inet = 0;
>
> struct sender_context {
> unsigned int num_fds;
> @@ -99,6 +103,7 @@ static void print_usage_exit(int error)
> "-h --help print this message\n"
> "-l --loops=LOOPS how many message should be send\n"
> "-p --pipe send data via a pipe\n"
> + "-i --inet send data via a inet tcp connection\n"
> "-s --datasize=SIZE message size\n"
> "-T --threads use POSIX threads\n"
> "-P --process use fork (default)\n"
> @@ -106,11 +111,52 @@ static void print_usage_exit(int error)
> exit(error);
> }
>
> +static int inet_socketpair(int fds[2])
> +{
> + int s1, s2;
> + struct sockaddr_in sin;
> + unsigned long ul = 1;
> +
> + if ((s1 = socket(AF_INET, SOCK_STREAM, 0)) < 0)
> + barf("socket");
> + if ((s2 = socket(AF_INET, SOCK_STREAM, 0)) < 0)
> + barf("socket");
> +
> + socklen_t len = sizeof(sin);
> + bzero(&sin, len);
> + sin.sin_family = AF_INET;
> + sin.sin_port = 0;
> + sin.sin_addr.s_addr = inet_addr("127.0.0.1");
> +
> + if (bind(s1, &sin, len) < 0)
> + barf("bind");
> + getsockname(s1, &sin, &len);
> + if (listen(s1, 10) < 0)
> + barf("listen");
> + if (ioctl(s2, FIONBIO, &ul) < 0)
> + barf("ioctl");
> + if (ioctl(s1, FIONBIO, &ul) < 0)
> + barf("ioctl");
> + connect(s2, &sin, len);
> + if ((fds[0] = accept(s1, &sin, &len)) < 0)
> + barf("accept");
> + ul = 0;
> + if (ioctl(s2, FIONBIO, &ul) < 0)
> + barf("ioctl");
> + fds[1] = s2;
> + close(s1);
> +
> + return 0;
> +}
> +
> static void fdpair(int fds[2])
> {
> if (use_pipes) {
> if (pipe(fds) == 0)
> return;
> + } else if (use_inet) {
> + if (inet_socketpair(fds) == 0)
> + return;
> } else {
> if (socketpair(AF_UNIX, SOCK_STREAM, 0, fds) == 0)
> return;
> @@ -364,13 +410,14 @@ static void process_options(int argc, char *argv[])
> {"help", no_argument, NULL, 'h'},
> {"loops", required_argument, NULL, 'l'},
> {"pipe", no_argument, NULL, 'p'},
> + {"inet", no_argument, NULL, 'i'},
> {"datasize", required_argument, NULL, 's'},
> {"threads", no_argument, NULL, 'T'},
> {"processes", no_argument, NULL, 'P'},
> {NULL, 0, NULL, 0}
> };
>
> - int c = getopt_long(argc, argv, "f:Fg:hl:ps:TP",
> + int c = getopt_long(argc, argv, "f:Fg:hl:pis:TP",
> longopts, &optind);
> if (c == -1) {
> break;
> @@ -402,6 +449,9 @@ static void process_options(int argc, char *argv[])
> case 'p':
> use_pipes = 1;
> break;
> + case 'i':
> + use_inet = 1;
> + break;
> case 's':
> if ((datasize = atoi(optarg)) <= 0) {
> fprintf(stderr, "%s: --datasize|-s requires an integer > 0\n", argv[0]);
> @@ -418,6 +468,11 @@ static void process_options(int argc, char *argv[])
> print_usage_exit(1);
> }
> }
> +
> + if (use_pipes && use_inet) {
> + fprintf(stderr, "%s: --pipe|-p and --inet|-i cannot be used together\n", argv[0]);
> + print_usage_exit(1);
> + }
> }
>
> void sigcatcher(int sig) {
> --
> 2.18.2
>
>
Sure, this seems like a good idea. If I understand correctly, it doesn't
change the default way that hackbench is run today without the option.
I'll take this, but it doesn't apply correctly today, probably
because it assumes that your first patch was applied. Could you regenerate
this patch, against the current code without your first patch, and I'll
apply it then.
Thanks
John
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] hackbench: Fix negativity opt checking
2022-05-09 18:59 ` John Kacur
@ 2022-05-23 15:39 ` Yihao Wu
0 siblings, 0 replies; 10+ messages in thread
From: Yihao Wu @ 2022-05-23 15:39 UTC (permalink / raw)
To: John Kacur; +Cc: Clark Williams, linux-rt-users
I'm sorry! John. In the commit log, I described the problem wrong. It
should be more trivially reproduced like this,
./hackbench --datasize 4096 -g 1 -l 1
This command gives an error "--datasize|-s requires an integer > 0". But
if you append any argument (or nonsense) after this command, like,
./hackbench --datasize 4096 -g 1 -l 1 whatever
This command runs without any error message.
My original intention of this patch was to remove "!(argv[optind]". I
suppose this condition leads to this bug. (Pardon me for I don't know
what's it for. I'd admit I'm not familiar with getopt)
This negative bugfix should only be a piggypack fix. But as you've
pointed out, it merely hides misusage. So it's a bad bugfix.
Thanks,
Yihao
On 2022/5/10 02:59, John Kacur wrote:
> Well, the code doesn't work the way the author intended, that's for sure.
> As your patch indicates, you realize the problem is that an unsigned
> variable can never be less than zero, so the check is meaningless.
> However, is changing the data type the right solution, just to make a
> check against something stupid a user might input the correct thing to do?
> It decreases the size of valid input. Maybe that doesn't matter, but then
> could it introduce a new bug somewhere? You've dropped the argv[optind]
> too which also doesn't work the way the author intended, but it was
> probably meant as a check of whether the user provided an argument or not.
>
> What I would rather see if you want to fix this, is a check against the
> user input that doesn't change the datatype. For example, if you treat the
> input as a char, if (optarg[0] == '-') then we can tell the difference
> between a really big number or user input that is stupid or malicious.
> If you look in cyclictest, (there could be bugs lurking there too), there
> is an attempt to parse this kind of thing and to check whether there are
> arguments too.
>
> Thanks
>
> John
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2] hackbench: Add af_inet mode besides af_unix and pipe
2022-05-09 19:05 ` John Kacur
@ 2022-05-23 15:40 ` Yihao Wu
2022-05-23 15:55 ` [PATCH v3] " Yihao Wu
0 siblings, 1 reply; 10+ messages in thread
From: Yihao Wu @ 2022-05-23 15:40 UTC (permalink / raw)
To: John Kacur; +Cc: Clark Williams, linux-rt-users
We observerved strange performance regression with hackbench, but only
with its default mode instead of pipe mode. It turned out it was unix
domain socket to blame rather than the scheduler. Because on older
kernels, there was a big lock. And modifications to the scheduler made
the contension worse.
However pipe mode is much different from the default af_unix mode. So
it's hard to prove it's really the case. So we think it might be a good
idea to have af_inet mode for hackbench.
After implementing it, we compare these three modes. And the result
shows as what we expect, that only af_unix mode has regression, the
others don't. This proves adding af_inet mode is valuable.
Signed-off-by: Yihao Wu <wuyihao@linux.alibaba.com>
---
src/hackbench/hackbench.c | 57 ++++++++++++++++++++++++++++++++++++++-
1 file changed, 56 insertions(+), 1 deletion(-)
diff --git a/src/hackbench/hackbench.c b/src/hackbench/hackbench.c
index 268c232..18c9284 100644
--- a/src/hackbench/hackbench.c
+++ b/src/hackbench/hackbench.c
@@ -20,11 +20,14 @@
#include <string.h>
#include <errno.h>
#include <unistd.h>
+#include <sys/ioctl.h>
#include <sys/types.h>
#include <sys/socket.h>
#include <sys/wait.h>
#include <sys/time.h>
#include <sys/poll.h>
+#include <netinet/in.h>
+#include <arpa/inet.h>
#include <limits.h>
#include <getopt.h>
#include <signal.h>
@@ -46,6 +49,7 @@ static unsigned int fifo = 0;
static unsigned int process_mode = PROCESS_MODE;
static int use_pipes = 0;
+static int use_inet = 0;
struct sender_context {
unsigned int num_fds;
@@ -99,6 +103,7 @@ static void print_usage_exit(int error)
"-h --help print this message\n"
"-l --loops=LOOPS how many message should be send\n"
"-p --pipe send data via a pipe\n"
+ "-i --inet send data via a inet tcp connection\n"
"-s --datasize=SIZE message size\n"
"-T --threads use POSIX threads\n"
"-P --process use fork (default)\n"
@@ -106,11 +111,52 @@ static void print_usage_exit(int error)
exit(error);
}
+static int inet_socketpair(int fds[2])
+{
+ int s1, s2;
+ struct sockaddr_in sin;
+ unsigned long ul = 1;
+
+ if ((s1 = socket(AF_INET, SOCK_STREAM, 0)) < 0)
+ barf("socket");
+ if ((s2 = socket(AF_INET, SOCK_STREAM, 0)) < 0)
+ barf("socket");
+
+ socklen_t len = sizeof(sin);
+ bzero(&sin, len);
+ sin.sin_family = AF_INET;
+ sin.sin_port = 0;
+ sin.sin_addr.s_addr = inet_addr("127.0.0.1");
+
+ if (bind(s1, &sin, len) < 0)
+ barf("bind");
+ getsockname(s1, &sin, &len);
+ if (listen(s1, 10) < 0)
+ barf("listen");
+ if (ioctl(s2, FIONBIO, &ul) < 0)
+ barf("ioctl");
+ if (ioctl(s1, FIONBIO, &ul) < 0)
+ barf("ioctl");
+ connect(s2, &sin, len);
+ if ((fds[0] = accept(s1, &sin, &len)) < 0)
+ barf("accept");
+ ul = 0;
+ if (ioctl(s2, FIONBIO, &ul) < 0)
+ barf("ioctl");
+ fds[1] = s2;
+ close(s1);
+
+ return 0;
+}
+
static void fdpair(int fds[2])
{
if (use_pipes) {
if (pipe(fds) == 0)
return;
+ } else if (use_inet) {
+ if (inet_socketpair(fds) == 0)
+ return;
} else {
if (socketpair(AF_UNIX, SOCK_STREAM, 0, fds) == 0)
return;
@@ -364,13 +410,14 @@ static void process_options(int argc, char *argv[])
{"help", no_argument, NULL, 'h'},
{"loops", required_argument, NULL, 'l'},
{"pipe", no_argument, NULL, 'p'},
+ {"inet", no_argument, NULL, 'i'},
{"datasize", required_argument, NULL, 's'},
{"threads", no_argument, NULL, 'T'},
{"processes", no_argument, NULL, 'P'},
{NULL, 0, NULL, 0}
};
- int c = getopt_long(argc, argv, "f:Fg:hl:ps:TP",
+ int c = getopt_long(argc, argv, "f:Fg:hl:pis:TP",
longopts, &optind);
if (c == -1) {
break;
@@ -402,6 +449,9 @@ static void process_options(int argc, char *argv[])
case 'p':
use_pipes = 1;
break;
+ case 'i':
+ use_inet = 1;
+ break;
case 's':
if (!(argv[optind] && (datasize = atoi(optarg)) > 0)) {
fprintf(stderr, "%s: --datasize|-s requires an integer > 0\n",
argv[0]);
@@ -418,6 +468,11 @@ static void process_options(int argc, char *argv[])
print_usage_exit(1);
}
}
+
+ if (use_pipes && use_inet) {
+ fprintf(stderr, "%s: --pipe|-p and --inet|-i cannot be used
together\n", argv[0]);
+ print_usage_exit(1);
+ }
}
void sigcatcher(int sig) {
--
2.32.0.604.gb1f3e1269
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] hackbench: Add af_inet mode besides af_unix and pipe
2022-04-28 10:37 ` [PATCH 2/2] hackbench: Add af_inet mode besides af_unix and pipe Yihao Wu
2022-05-09 19:05 ` John Kacur
@ 2022-05-23 15:44 ` Yihao Wu
1 sibling, 0 replies; 10+ messages in thread
From: Yihao Wu @ 2022-05-23 15:44 UTC (permalink / raw)
To: Clark Williams, John Kacur; +Cc: linux-rt-users
Hi John,
I regenerated the second patch and sent it to you. Now it should be
applied successfully without the first patch.
Thanks a lot for your patient reviewing and comments!
Yihao
On 2022/4/28 18:37, Yihao Wu wrote:
> We observerved strange performance regression with hackbench, but only with
> its default mode instead of pipe mode. It turned out it was unix domain
> socket to blame rather than the scheduler. Because on older kernels, there
> was a big lock. And modifications to the scheduler made the contension
> worse.
>
> However pipe mode is much different from the default af_unix mode. So it's
> hard to prove it's really the case. So we think it might be a good idea to
> have af_inet mode for hackbench.
>
> After implementing it, we compare these three modes. And the result shows
> as what we expect, that only af_unix mode has regression, the others don't.
> This proves adding af_inet mode is valuable.
>
> Signed-off-by: Yihao Wu <wuyihao@linux.alibaba.com>
> ---
> src/hackbench/hackbench.c | 57 ++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 56 insertions(+), 1 deletion(-)
>
> diff --git a/src/hackbench/hackbench.c b/src/hackbench/hackbench.c
> index b9b0af6..030342f 100644
> --- a/src/hackbench/hackbench.c
> +++ b/src/hackbench/hackbench.c
> @@ -20,11 +20,14 @@
> #include <string.h>
> #include <errno.h>
> #include <unistd.h>
> +#include <sys/ioctl.h>
> #include <sys/types.h>
> #include <sys/socket.h>
> #include <sys/wait.h>
> #include <sys/time.h>
> #include <sys/poll.h>
> +#include <netinet/in.h>
> +#include <arpa/inet.h>
> #include <limits.h>
> #include <getopt.h>
> #include <signal.h>
> @@ -46,6 +49,7 @@ static unsigned int fifo = 0;
> static unsigned int process_mode = PROCESS_MODE;
>
> static int use_pipes = 0;
> +static int use_inet = 0;
>
> struct sender_context {
> unsigned int num_fds;
> @@ -99,6 +103,7 @@ static void print_usage_exit(int error)
> "-h --help print this message\n"
> "-l --loops=LOOPS how many message should be send\n"
> "-p --pipe send data via a pipe\n"
> + "-i --inet send data via a inet tcp connection\n"
> "-s --datasize=SIZE message size\n"
> "-T --threads use POSIX threads\n"
> "-P --process use fork (default)\n"
> @@ -106,11 +111,52 @@ static void print_usage_exit(int error)
> exit(error);
> }
>
> +static int inet_socketpair(int fds[2])
> +{
> + int s1, s2;
> + struct sockaddr_in sin;
> + unsigned long ul = 1;
> +
> + if ((s1 = socket(AF_INET, SOCK_STREAM, 0)) < 0)
> + barf("socket");
> + if ((s2 = socket(AF_INET, SOCK_STREAM, 0)) < 0)
> + barf("socket");
> +
> + socklen_t len = sizeof(sin);
> + bzero(&sin, len);
> + sin.sin_family = AF_INET;
> + sin.sin_port = 0;
> + sin.sin_addr.s_addr = inet_addr("127.0.0.1");
> +
> + if (bind(s1, &sin, len) < 0)
> + barf("bind");
> + getsockname(s1, &sin, &len);
> + if (listen(s1, 10) < 0)
> + barf("listen");
> + if (ioctl(s2, FIONBIO, &ul) < 0)
> + barf("ioctl");
> + if (ioctl(s1, FIONBIO, &ul) < 0)
> + barf("ioctl");
> + connect(s2, &sin, len);
> + if ((fds[0] = accept(s1, &sin, &len)) < 0)
> + barf("accept");
> + ul = 0;
> + if (ioctl(s2, FIONBIO, &ul) < 0)
> + barf("ioctl");
> + fds[1] = s2;
> + close(s1);
> +
> + return 0;
> +}
> +
> static void fdpair(int fds[2])
> {
> if (use_pipes) {
> if (pipe(fds) == 0)
> return;
> + } else if (use_inet) {
> + if (inet_socketpair(fds) == 0)
> + return;
> } else {
> if (socketpair(AF_UNIX, SOCK_STREAM, 0, fds) == 0)
> return;
> @@ -364,13 +410,14 @@ static void process_options(int argc, char *argv[])
> {"help", no_argument, NULL, 'h'},
> {"loops", required_argument, NULL, 'l'},
> {"pipe", no_argument, NULL, 'p'},
> + {"inet", no_argument, NULL, 'i'},
> {"datasize", required_argument, NULL, 's'},
> {"threads", no_argument, NULL, 'T'},
> {"processes", no_argument, NULL, 'P'},
> {NULL, 0, NULL, 0}
> };
>
> - int c = getopt_long(argc, argv, "f:Fg:hl:ps:TP",
> + int c = getopt_long(argc, argv, "f:Fg:hl:pis:TP",
> longopts, &optind);
> if (c == -1) {
> break;
> @@ -402,6 +449,9 @@ static void process_options(int argc, char *argv[])
> case 'p':
> use_pipes = 1;
> break;
> + case 'i':
> + use_inet = 1;
> + break;
> case 's':
> if ((datasize = atoi(optarg)) <= 0) {
> fprintf(stderr, "%s: --datasize|-s requires an integer > 0\n", argv[0]);
> @@ -418,6 +468,11 @@ static void process_options(int argc, char *argv[])
> print_usage_exit(1);
> }
> }
> +
> + if (use_pipes && use_inet) {
> + fprintf(stderr, "%s: --pipe|-p and --inet|-i cannot be used together\n", argv[0]);
> + print_usage_exit(1);
> + }
> }
>
> void sigcatcher(int sig) {
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v3] hackbench: Add af_inet mode besides af_unix and pipe
2022-05-23 15:40 ` [PATCH v2] " Yihao Wu
@ 2022-05-23 15:55 ` Yihao Wu
2022-06-24 16:28 ` John Kacur
0 siblings, 1 reply; 10+ messages in thread
From: Yihao Wu @ 2022-05-23 15:55 UTC (permalink / raw)
To: Clark Williams, John Kacur; +Cc: linux-rt-users
We observerved strange performance regression with hackbench, but only with
its default mode instead of pipe mode. It turned out it was unix domain
socket to blame rather than the scheduler. Because on older kernels, there
was a big lock. And modifications to the scheduler made the contension
worse.
However pipe mode is much different from the default af_unix mode. So it's
hard to prove it's really the case. So we think it might be a good idea to
have af_inet mode for hackbench.
After implementing it, we compare these three modes. And the result shows
as what we expect, that only af_unix mode has regression, the others don't.
This proves adding af_inet mode is valuable.
Signed-off-by: Yihao Wu <wuyihao@linux.alibaba.com>
---
v1->v2: Drop the first patch
v2->v3: Fix EOLs in the commit log
src/hackbench/hackbench.c | 57 ++++++++++++++++++++++++++++++++++++++-
1 file changed, 56 insertions(+), 1 deletion(-)
diff --git a/src/hackbench/hackbench.c b/src/hackbench/hackbench.c
index 268c232..18c9284 100644
--- a/src/hackbench/hackbench.c
+++ b/src/hackbench/hackbench.c
@@ -20,11 +20,14 @@
#include <string.h>
#include <errno.h>
#include <unistd.h>
+#include <sys/ioctl.h>
#include <sys/types.h>
#include <sys/socket.h>
#include <sys/wait.h>
#include <sys/time.h>
#include <sys/poll.h>
+#include <netinet/in.h>
+#include <arpa/inet.h>
#include <limits.h>
#include <getopt.h>
#include <signal.h>
@@ -46,6 +49,7 @@ static unsigned int fifo = 0;
static unsigned int process_mode = PROCESS_MODE;
static int use_pipes = 0;
+static int use_inet = 0;
struct sender_context {
unsigned int num_fds;
@@ -99,6 +103,7 @@ static void print_usage_exit(int error)
"-h --help print this message\n"
"-l --loops=LOOPS how many message should be send\n"
"-p --pipe send data via a pipe\n"
+ "-i --inet send data via a inet tcp connection\n"
"-s --datasize=SIZE message size\n"
"-T --threads use POSIX threads\n"
"-P --process use fork (default)\n"
@@ -106,11 +111,52 @@ static void print_usage_exit(int error)
exit(error);
}
+static int inet_socketpair(int fds[2])
+{
+ int s1, s2;
+ struct sockaddr_in sin;
+ unsigned long ul = 1;
+
+ if ((s1 = socket(AF_INET, SOCK_STREAM, 0)) < 0)
+ barf("socket");
+ if ((s2 = socket(AF_INET, SOCK_STREAM, 0)) < 0)
+ barf("socket");
+
+ socklen_t len = sizeof(sin);
+ bzero(&sin, len);
+ sin.sin_family = AF_INET;
+ sin.sin_port = 0;
+ sin.sin_addr.s_addr = inet_addr("127.0.0.1");
+
+ if (bind(s1, &sin, len) < 0)
+ barf("bind");
+ getsockname(s1, &sin, &len);
+ if (listen(s1, 10) < 0)
+ barf("listen");
+ if (ioctl(s2, FIONBIO, &ul) < 0)
+ barf("ioctl");
+ if (ioctl(s1, FIONBIO, &ul) < 0)
+ barf("ioctl");
+ connect(s2, &sin, len);
+ if ((fds[0] = accept(s1, &sin, &len)) < 0)
+ barf("accept");
+ ul = 0;
+ if (ioctl(s2, FIONBIO, &ul) < 0)
+ barf("ioctl");
+ fds[1] = s2;
+ close(s1);
+
+ return 0;
+}
+
static void fdpair(int fds[2])
{
if (use_pipes) {
if (pipe(fds) == 0)
return;
+ } else if (use_inet) {
+ if (inet_socketpair(fds) == 0)
+ return;
} else {
if (socketpair(AF_UNIX, SOCK_STREAM, 0, fds) == 0)
return;
@@ -364,13 +410,14 @@ static void process_options(int argc, char *argv[])
{"help", no_argument, NULL, 'h'},
{"loops", required_argument, NULL, 'l'},
{"pipe", no_argument, NULL, 'p'},
+ {"inet", no_argument, NULL, 'i'},
{"datasize", required_argument, NULL, 's'},
{"threads", no_argument, NULL, 'T'},
{"processes", no_argument, NULL, 'P'},
{NULL, 0, NULL, 0}
};
- int c = getopt_long(argc, argv, "f:Fg:hl:ps:TP",
+ int c = getopt_long(argc, argv, "f:Fg:hl:pis:TP",
longopts, &optind);
if (c == -1) {
break;
@@ -402,6 +449,9 @@ static void process_options(int argc, char *argv[])
case 'p':
use_pipes = 1;
break;
+ case 'i':
+ use_inet = 1;
+ break;
case 's':
if (!(argv[optind] && (datasize = atoi(optarg)) > 0)) {
fprintf(stderr, "%s: --datasize|-s requires an integer > 0\n", argv[0]);
@@ -418,6 +468,11 @@ static void process_options(int argc, char *argv[])
print_usage_exit(1);
}
}
+
+ if (use_pipes && use_inet) {
+ fprintf(stderr, "%s: --pipe|-p and --inet|-i cannot be used together\n", argv[0]);
+ print_usage_exit(1);
+ }
}
void sigcatcher(int sig) {
--
2.32.0.604.gb1f3e1269
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v3] hackbench: Add af_inet mode besides af_unix and pipe
2022-05-23 15:55 ` [PATCH v3] " Yihao Wu
@ 2022-06-24 16:28 ` John Kacur
0 siblings, 0 replies; 10+ messages in thread
From: John Kacur @ 2022-06-24 16:28 UTC (permalink / raw)
To: Yihao Wu; +Cc: Clark Williams, linux-rt-users
On Mon, 23 May 2022, Yihao Wu wrote:
> We observerved strange performance regression with hackbench, but only with
> its default mode instead of pipe mode. It turned out it was unix domain
> socket to blame rather than the scheduler. Because on older kernels, there
> was a big lock. And modifications to the scheduler made the contension
> worse.
>
> However pipe mode is much different from the default af_unix mode. So it's
> hard to prove it's really the case. So we think it might be a good idea to
> have af_inet mode for hackbench.
>
> After implementing it, we compare these three modes. And the result shows
> as what we expect, that only af_unix mode has regression, the others don't.
> This proves adding af_inet mode is valuable.
>
> Signed-off-by: Yihao Wu <wuyihao@linux.alibaba.com>
> ---
> v1->v2: Drop the first patch
> v2->v3: Fix EOLs in the commit log
>
> src/hackbench/hackbench.c | 57 ++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 56 insertions(+), 1 deletion(-)
>
> diff --git a/src/hackbench/hackbench.c b/src/hackbench/hackbench.c
> index 268c232..18c9284 100644
> --- a/src/hackbench/hackbench.c
> +++ b/src/hackbench/hackbench.c
> @@ -20,11 +20,14 @@
> #include <string.h>
> #include <errno.h>
> #include <unistd.h>
> +#include <sys/ioctl.h>
> #include <sys/types.h>
> #include <sys/socket.h>
> #include <sys/wait.h>
> #include <sys/time.h>
> #include <sys/poll.h>
> +#include <netinet/in.h>
> +#include <arpa/inet.h>
> #include <limits.h>
> #include <getopt.h>
> #include <signal.h>
> @@ -46,6 +49,7 @@ static unsigned int fifo = 0;
> static unsigned int process_mode = PROCESS_MODE;
>
> static int use_pipes = 0;
> +static int use_inet = 0;
>
> struct sender_context {
> unsigned int num_fds;
> @@ -99,6 +103,7 @@ static void print_usage_exit(int error)
> "-h --help print this message\n"
> "-l --loops=LOOPS how many message should be send\n"
> "-p --pipe send data via a pipe\n"
> + "-i --inet send data via a inet tcp connection\n"
> "-s --datasize=SIZE message size\n"
> "-T --threads use POSIX threads\n"
> "-P --process use fork (default)\n"
> @@ -106,11 +111,52 @@ static void print_usage_exit(int error)
> exit(error);
> }
>
> +static int inet_socketpair(int fds[2])
> +{
> + int s1, s2;
> + struct sockaddr_in sin;
> + unsigned long ul = 1;
> +
> + if ((s1 = socket(AF_INET, SOCK_STREAM, 0)) < 0)
> + barf("socket");
> + if ((s2 = socket(AF_INET, SOCK_STREAM, 0)) < 0)
> + barf("socket");
> +
> + socklen_t len = sizeof(sin);
> + bzero(&sin, len);
> + sin.sin_family = AF_INET;
> + sin.sin_port = 0;
> + sin.sin_addr.s_addr = inet_addr("127.0.0.1");
> +
> + if (bind(s1, &sin, len) < 0)
> + barf("bind");
> + getsockname(s1, &sin, &len);
> + if (listen(s1, 10) < 0)
> + barf("listen");
> + if (ioctl(s2, FIONBIO, &ul) < 0)
> + barf("ioctl");
> + if (ioctl(s1, FIONBIO, &ul) < 0)
> + barf("ioctl");
> + connect(s2, &sin, len);
> + if ((fds[0] = accept(s1, &sin, &len)) < 0)
> + barf("accept");
> + ul = 0;
> + if (ioctl(s2, FIONBIO, &ul) < 0)
> + barf("ioctl");
> + fds[1] = s2;
> + close(s1);
> +
> + return 0;
> +}
> +
> static void fdpair(int fds[2])
> {
> if (use_pipes) {
> if (pipe(fds) == 0)
> return;
> + } else if (use_inet) {
> + if (inet_socketpair(fds) == 0)
> + return;
> } else {
> if (socketpair(AF_UNIX, SOCK_STREAM, 0, fds) == 0)
> return;
> @@ -364,13 +410,14 @@ static void process_options(int argc, char *argv[])
> {"help", no_argument, NULL, 'h'},
> {"loops", required_argument, NULL, 'l'},
> {"pipe", no_argument, NULL, 'p'},
> + {"inet", no_argument, NULL, 'i'},
> {"datasize", required_argument, NULL, 's'},
> {"threads", no_argument, NULL, 'T'},
> {"processes", no_argument, NULL, 'P'},
> {NULL, 0, NULL, 0}
> };
>
> - int c = getopt_long(argc, argv, "f:Fg:hl:ps:TP",
> + int c = getopt_long(argc, argv, "f:Fg:hl:pis:TP",
> longopts, &optind);
> if (c == -1) {
> break;
> @@ -402,6 +449,9 @@ static void process_options(int argc, char *argv[])
> case 'p':
> use_pipes = 1;
> break;
> + case 'i':
> + use_inet = 1;
> + break;
> case 's':
> if (!(argv[optind] && (datasize = atoi(optarg)) > 0)) {
> fprintf(stderr, "%s: --datasize|-s requires an integer > 0\n", argv[0]);
> @@ -418,6 +468,11 @@ static void process_options(int argc, char *argv[])
> print_usage_exit(1);
> }
> }
> +
> + if (use_pipes && use_inet) {
> + fprintf(stderr, "%s: --pipe|-p and --inet|-i cannot be used together\n", argv[0]);
> + print_usage_exit(1);
> + }
> }
>
> void sigcatcher(int sig) {
> --
> 2.32.0.604.gb1f3e1269
>
>
Signed-off-by: John Kacur <jkacur@redhat.com>
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2022-06-24 16:28 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-04-28 10:37 [PATCH 0/2] Add af_inet mode besides af_unix and pipe Yihao Wu
2022-04-28 10:37 ` [PATCH 1/2] hackbench: Fix negativity opt checking Yihao Wu
2022-05-09 18:59 ` John Kacur
2022-05-23 15:39 ` Yihao Wu
2022-04-28 10:37 ` [PATCH 2/2] hackbench: Add af_inet mode besides af_unix and pipe Yihao Wu
2022-05-09 19:05 ` John Kacur
2022-05-23 15:40 ` [PATCH v2] " Yihao Wu
2022-05-23 15:55 ` [PATCH v3] " Yihao Wu
2022-06-24 16:28 ` John Kacur
2022-05-23 15:44 ` [PATCH 2/2] " Yihao Wu
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.