Ticket #2949 (closed enhancement: invalid)
[ Optimization ]: replace pipe() by pipe2()
Reported by: | pavlinux | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | |
Component: | mc-core | Version: | master |
Keywords: | Cc: | gotar@… | |
Blocked By: | Blocking: | ||
Branch state: | no branch | Votes for changeset: |
Description (last modified by andrew_b) (diff)
Linux only :-P
- The pipe2() was added to Linux in version 2.6.27. It is a variant of the normal pipe syscall, but takes an extra flags argument which can be the ORed value of O_NONBLOCK and O_CLOEXEC. The original pipe syscall is now defined as like the pipe2 syscall, but with the flags variable set to zero.
- The pipe2() glibc support is available starting with version 2.9.
- The pipe2() is Linux-specific.
-
lib/utilunix.c
diff --git a/lib/utilunix.c b/lib/utilunix.c index c3c6704..0127544 100644
a b tilde_expand (const char *directory) 340 340 void 341 341 open_error_pipe (void) 342 342 { 343 if (pipe (error_pipe) < 0)343 if (pipe2(error_pipe, O_DIRECT) < 0) 344 344 { 345 345 message (D_NORMAL, _("Warning"), _("Pipe failed")); 346 346 } -
src/background.c
diff --git a/src/background.c b/src/background.c index a68fe86..affe8c0 100644
a b do_background (struct FileOpContext *ctx, char *info) 514 514 int back_comm[2]; /* back connection */ 515 515 pid_t pid; 516 516 517 if (pipe (comm) == -1)517 if (pipe2(comm, O_DIRECT) == -1) 518 518 return -1; 519 519 520 if (pipe (back_comm) == -1)520 if (pipe2(back_comm, O_DIRECT) == -1) 521 521 return -1; 522 522 523 523 pid = fork (); -
src/cons.handler.c
diff --git a/src/cons.handler.c b/src/cons.handler.c index 610db28..97a09b6 100644
a b handle_console_linux (console_action_t action) 148 148 status = close (pipefd1[1]); 149 149 status = close (pipefd2[0]); 150 150 /* Create two pipes for communication */ 151 if (!((pipe (pipefd1) == 0) && ((pipe (pipefd2)) == 0)))151 if (!((pipe2(pipefd1, O_DIRECT) == 0) && ((pipe2(pipefd2, O_DIRECT)) == 0))) 152 152 { 153 153 mc_global.tty.console_flag = '\0'; 154 154 break; -
src/subshell.c
diff --git a/src/subshell.c b/src/subshell.c index 510cf4d..b19117a 100644
a b init_subshell (void) 850 850 return; 851 851 } 852 852 } 853 else /* subshell_type is BASH or ZSH */ if (pipe (subshell_pipe))853 else /* subshell_type is BASH or ZSH */ if (pipe2(subshell_pipe, O_DIRECT)) 854 854 { 855 855 perror (__FILE__ ": couldn't create pipe"); 856 856 mc_global.tty.use_subshell = FALSE; -
src/vfs/fish/fish.c
diff --git a/src/vfs/fish/fish.c b/src/vfs/fish/fish.c index 5c84175..e02ec77 100644
a b fish_pipeopen (struct vfs_s_super *super, const char *path, const char *argv[]) 312 312 int fileset1[2], fileset2[2]; 313 313 int res; 314 314 315 if ((pipe (fileset1) < 0) || (pipe (fileset2) < 0))315 if ((pipe2(fileset1, O_DIRECT) < 0) || (pipe2(fileset2, O_DIRECT) < 0)) 316 316 vfs_die ("Cannot pipe(): %m."); 317 317 318 318 res = fork ();
Attachments
Change History
comment:2 follow-up: ↓ 5 Changed 12 years ago by andrew_b
- Status changed from new to closed
- Resolution set to wontfix
Could you make your patch not for Linux only?
comment:4 Changed 12 years ago by ossi
regardless of the missing ifdefs, the description also entirely fails to mention what the *point* of this patch is.
comment:5 in reply to: ↑ 2 Changed 12 years ago by pavlinux
Replying to andrew_b:
Could you make your patch not for Linux only?
/* define somewhere */
#ifdef linux #define mc_pipe(fd) pipe2(fd, O_DIRECT) #else #define mc_pipe(fd) pipe(fd) #endif
comment:7 Changed 12 years ago by pavlinux
Fixed patch.
-
lib/utilunix.c
diff --git a/lib/utilunix.c b/lib/utilunix.c index c3c6704..4f53d6e 100644
a b tilde_expand (const char *directory) 340 340 void 341 341 open_error_pipe (void) 342 342 { 343 if ( pipe(error_pipe) < 0)343 if (mc_pipe(error_pipe) < 0) 344 344 { 345 345 message (D_NORMAL, _("Warning"), _("Pipe failed")); 346 346 } -
src/background.c
diff --git a/src/background.c b/src/background.c index a68fe86..a628b90 100644
a b do_background (struct FileOpContext *ctx, char *info) 514 514 int back_comm[2]; /* back connection */ 515 515 pid_t pid; 516 516 517 if ( pipe(comm) == -1)517 if (mc_pipe(comm) == -1) 518 518 return -1; 519 519 520 if ( pipe(back_comm) == -1)520 if (mc_pipe(back_comm) == -1) 521 521 return -1; 522 522 523 523 pid = fork (); -
src/cons.handler.c
diff --git a/src/cons.handler.c b/src/cons.handler.c index 610db28..c2bea27 100644
a b handle_console_linux (console_action_t action) 148 148 status = close (pipefd1[1]); 149 149 status = close (pipefd2[0]); 150 150 /* Create two pipes for communication */ 151 if (!(( pipe (pipefd1) == 0) && ((pipe(pipefd2)) == 0)))151 if (!((mc_pipe(pipefd1) == 0) && ((mc_pipe(pipefd2)) == 0))) 152 152 { 153 153 mc_global.tty.console_flag = '\0'; 154 154 break; -
src/subshell.c
diff --git a/src/subshell.c b/src/subshell.c index 510cf4d..00ca85c 100644
a b init_subshell (void) 850 850 return; 851 851 } 852 852 } 853 else /* subshell_type is BASH or ZSH */ if ( pipe(subshell_pipe))853 else /* subshell_type is BASH or ZSH */ if (mc_pipe(subshell_pipe)) 854 854 { 855 855 perror (__FILE__ ": couldn't create pipe"); 856 856 mc_global.tty.use_subshell = FALSE; -
src/vfs/fish/fish.c
diff --git a/src/vfs/fish/fish.c b/src/vfs/fish/fish.c index 5c84175..b1d2a4c 100644
a b fish_pipeopen (struct vfs_s_super *super, const char *path, const char *argv[]) 312 312 int fileset1[2], fileset2[2]; 313 313 int res; 314 314 315 if (( pipe (fileset1) < 0) || (pipe(fileset2) < 0))315 if ((mc_pipe(fileset1) < 0) || (mc_pipe(fileset2) < 0)) 316 316 vfs_die ("Cannot pipe(): %m."); 317 317 318 318 res = fork (); -
lib/global.h
diff --git a/lib/global.h b/lib/global.h index f13dfc0..0655c1e 100644
a b typedef struct 283 283 } vfs; 284 284 } mc_global_t; 285 285 286 #if defined(linux) /* compiler definition */ 287 #include <linux/version.h> 288 #if LINUX_VERSION_CODE > KERNEL_VERSION(2,6,28) 289 #define mc_pipe(fd) pipe2(fd, O_DIRECT) 290 #else 291 #define mc_pipe(fd) pipe(fd) 292 #endif 293 #else 294 #define mc_pipe(fd) pipe(fd) 295 #endif 296 286 297 /*** global variables defined in .c file *********************************************************/ 287 298 288 299 extern mc_global_t mc_global;
comment:8 Changed 12 years ago by pavlinux
- Status changed from closed to reopened
- Resolution wontfix deleted
comment:9 Changed 12 years ago by ossi
you still didn't say how this is supposed to be an optimization. also, some numbers (or at least plausible reasoning) which suggest that complicating the code is even remotely worth it would be in order.
comment:10 in reply to: ↑ description ; follow-up: ↓ 11 Changed 12 years ago by slyfox
if ((pipe (fileset1) < 0) (pipe (fileset2) < 0)) + if ((pipe2(fileset1, O_DIRECT) < 0) (pipe2(fileset2, O_DIRECT) < 0))
Care to explain benefits?
The O_DIRECT addition pessimizes pipe I/O by requiring more read()s:
comment:11 in reply to: ↑ 10 Changed 12 years ago by pavlinux
The O_DIRECT addition pessimizes pipe I/O by requiring more read()s:
...you can now treat the pipe as a packet interface, where each read() system call will read one packet at a time...
comment:12 follow-up: ↓ 14 Changed 12 years ago by pavlinux
Мужики, кто из нас разработчик миднайта?!
Я предложил фичу, не нравиццо - удаляйте, сомневаетесь - делайте бенчмарки.
На больших потоках данных оно реально даёт прирост, я уже с мая месяца эту фичу
использую в своих проектах.
Google Translate to help you if you do not like the russian :-P
comment:13 follow-up: ↓ 16 Changed 12 years ago by pavlinux
Benchmark:
http://pavlinux.ru/pipe2bench/pipe2bench.c
http://pavlinux.ru/pipe2bench/Makefile
# make
# ./pipe2bench /media/disk/iso/2011Mb.iso
PIPE Time: 35141 ms
PIPE2 Time: 14922 ms
comment:14 in reply to: ↑ 12 Changed 12 years ago by ossi
Replying to pavlinux:
Google Translate to help you if you do not like the russian :-P
yeah, right - as if i'd understand any of that gibberish if i didn't read russian ...
you're just being lazy, which is somewhat consistent with the rest of this contribution. your synthetic benchmark as such doesn't prove anything. for every case where you replaced the pipe() call, you need to identify the expected data flow patterns and make a case that using "packet pipes" improves the performance on average. also, you should make a point that it *makes any practical difference at all* - whether it be higher bandwidth in some particular (realistic) scenario, or just measurably lower cpu usage (and thus power consumption).
comment:15 Changed 12 years ago by andrew_b
- Status changed from reopened to closed
- Resolution set to wontfix
comment:16 in reply to: ↑ 13 Changed 12 years ago by slyfox
Replying to pavlinux:
Benchmark:
http://pavlinux.ru/pipe2bench/pipe2bench.c
http://pavlinux.ru/pipe2bench/Makefile
# make
# ./pipe2bench /media/disk/iso/2011Mb.iso
PIPE Time: 35141 ms
PIPE2 Time: 14922 ms
Benchmarking sync()/fsync() times is very interesting, yes.
Try to explore performance patterns with 'perf' tool'.
It's very simple:
$ ls -lh /big/dist/os/maemo/n800/RX-34_DIABLO_4.2008.bin 136MB $ perf record ./pipe2bench /big/dist/os/maemo/n800/RX-34_DIABLO_4.2008.bin PIPE Time: 635 ms PIPE2 Time: 336 ms perf record ./pipe2bench /big/dist/os/maemo/n800/RX-34_DIABLO_4.2008.bin perf report 29.87% pipe2bench [kernel.kallsyms] [k] copy_user_generic_string 19.97% pipe2bench [kernel.kallsyms] [k] sync_inodes_sb 6.61% pipe2bench [kernel.kallsyms] [k] _raw_spin_lock 3.62% pipe2bench [kernel.kallsyms] [k] find_get_page 3.06% pipe2bench [kernel.kallsyms] [k] btrfs_drop_inode 2.76% pipe2bench [kernel.kallsyms] [k] filemap_fdatawait_range 2.24% pipe2bench [kernel.kallsyms] [k] __srcu_read_lock 1.67% pipe2bench [kernel.kallsyms] [k] radix_tree_lookup_element 1.57% pipe2bench [kernel.kallsyms] [k] filemap_fdatawait
Fun, isn't it? You've benched my cache drop speed.
How about second run?
./pipe2bench /big/dist/os/maemo/n800/RX-34_DIABLO_4.2008.bin PIPE Time: 335 ms PIPE2 Time: 334 ms
I strongly suggest you to read what pipe's O_DIRECT actually does
and who is intended to be the primary user of it.
You might also like to add a loop to your benchmark to compare a set of
runs or just swap pp2() and pp().
Мужики, кто из нас разработчик миднайта?!
Я предложил фичу, не нравиццо - удаляйте, сомневаетесь - делайте бенчмарки.
На больших потоках данных оно реально даёт прирост, я уже с мая месяца эту фичу
использую в своих проектах.
Это хак для autofs, которая имеет нестабильный ABI user<->kernel
(размер структуры плавает в зависимости от битности ядра ядра):
http://lwn.net/Articles/494993/
Можешь в бенчмарке побайтно начать писать в pipe и постранично
пытаться считывать - увидишь разницу.
Эта фича не делает pipe() быстрее (можно убедиться на трейсе, выданном perf).
comment:17 follow-ups: ↓ 20 ↓ 21 Changed 12 years ago by pavlinux
2-nd edition :)
http://pavlinux.ru/pipe2bench/pipe2bench2.c
localhost:/tmp # for ((i=0; i < 5; i++)) do ./pipe2bench2 /media/disk/iso/2011Mb.iso; done; PIPE Time: 2283 ms PIPE2 Time: 1806 ms PIPE Time: 2242 ms PIPE2 Time: 1805 ms PIPE Time: 2227 ms PIPE2 Time: 1795 ms PIPE Time: 2215 ms PIPE2 Time: 1803 ms PIPE Time: 2191 ms PIPE2 Time: 1795 ms
По ассемблерному коду вообще нет различий, так что,
счётчик функций ваааще не показатель!
; pipe() secion jmp 400b05 <main+0x125> nopl 0x0(%rax) mov 0x2015d6(%rip),%edi # 6020cc <pipefd+0x4> lea 0x20(%rsp),%rsi mov $0x1,%edx callq 4008d0 <write@plt> mov 0x2015bd(%rip),%edi # 6020c8 <pipefd> lea 0x20(%rsp),%rsi mov $0x1000,%edx callq 400940 <read@plt> test %rax,%rax jg 400af0 <main+0x110> ; pipe2() secion jmp 400b85 <main+0x1a5> nopl 0x0(%rax,%rax,1) mov 0x20154a(%rip),%edi # 6020c0 <pipe2fd+0x4> lea 0x20(%rsp),%rsi mov $0x1,%edx callq 4008d0 <write@plt> mov 0x201531(%rip),%edi # 6020bc <pipe2fd> lea 0x20(%rsp),%rsi mov $0x1000,%edx callq 400940 <read@plt> test %rax,%rax jg 400b70 <main+0x190>
comment:18 Changed 12 years ago by pavlinux
- Status changed from closed to reopened
- Resolution wontfix deleted
comment:19 follow-up: ↓ 24 Changed 12 years ago by andrew_b
Rewrite your patch to use m4 to test new function instead of using kernel headers in user-space program.
comment:20 in reply to: ↑ 17 Changed 12 years ago by slyfox
Replying to pavlinux:
2-nd edition :)
http://pavlinux.ru/pipe2bench/pipe2bench2.c
localhost:/tmp # for ((i=0; i < 5; i++)) do ./pipe2bench2 /media/disk/iso/2011Mb.iso; done; PIPE Time: 2283 ms PIPE2 Time: 1806 ms PIPE Time: 2242 ms PIPE2 Time: 1805 ms PIPE Time: 2227 ms PIPE2 Time: 1795 ms PIPE Time: 2215 ms PIPE2 Time: 1803 ms PIPE Time: 2191 ms PIPE2 Time: 1795 msПо ассемблерному коду вообще нет различий, так что,
счётчик функций ваааще не показатель!
?
perf показывает реальное число инструкций ядра ([k]),
a не функций. Поставь pp2() первым в тесте :]
Что это за железка?
Обычный десктоп на 1G файле:
[sf] /tmp/y:./a /big/mov/film/The\ Art\ of\ Flight.m4v PIPE Time: 10009 ms PIPE2 Time: 460 ms [sf] /tmp/y:./a /big/mov/film/The\ Art\ of\ Flight.m4v PIPE Time: 444 ms PIPE2 Time: 439 ms [sf] /tmp/y:./a /big/mov/film/The\ Art\ of\ Flight.m4v PIPE Time: 432 ms PIPE2 Time: 441 ms
comment:21 in reply to: ↑ 17 ; follow-up: ↓ 22 Changed 12 years ago by slyfox
Replying to pavlinux:
2-nd edition :)
И подсунь ему файл размером 7 байт и посмотри на трейс:
read(6, "1234567", 4096) = 7 write(7, "1", 1) = 1
hint:
n = read(pipefd[0], &buf, BUFF_SIZE) > 0
возвращает только 0 или 1.
comment:22 in reply to: ↑ 21 ; follow-up: ↓ 25 Changed 12 years ago by pavlinux
Replying to slyfox:
И подсунь ему файл размером 7 байт и посмотри на трейс:
Без тибя нашёл :-P
3-nd edition :)
http://pavlinux.ru/pipe2bench/pipe2bench2.c
http://pavlinux.ru/pipe2bench/Makefile;
http://pavlinux.ru/pipe2bench/MD5SUM;
83489098d5b6f13c324b5b5b972dcc3d Makefile
ddb4f9e4bbbc8d721a297cc66af247a3 pipe2bench2.c
comment:23 Changed 12 years ago by pavlinux
Переставил pp2() и pp() местами
# ./pipe2bench /media/disk/iso/2011Mb.iso PIPE Time: 35948 ms PIPE2 Time: 12696 ms
В 2 раза!!!
# ./pipe2bench /media/disk/pub/Dist/Windows7/Windows7.Ultimate.32bit.iso PIPE Time: 86984 ms PIPE2 Time: 21992 ms
Есть разница, 1.5 минуты или 20 сек.?
./pipe2bench /boot/vmlinuz-3.8.0-rc3+ PIPE Time: 159 ms PIPE2 Time: 113 ms
# ./pipe2bench /media/disk/pub/Distributives/MSVSphere/MSVSphere-Server.rar PIPE Time: 46552 ms PIPE2 Time: 13746 ms
./pipe2bench /media/disk/pub/Distributives/WindowsXP64/WindowsXP_64_SP2.iso PIPE Time: 9053 ms PIPE2 Time: 2698 ms
в последнем тесте поменял местами, сначала pipe(), потом pipe2();
Измерять надо при первом обращении к файлу, дальше оно кэшируется.
comment:24 in reply to: ↑ 19 Changed 12 years ago by gotar
- Cc gotar@… added
Replying to andrew_b:
Rewrite your patch to use m4 to test new function instead of using kernel headers in user-space program.
Compile-time detection is not enough, this must be runtime-detectable in order not to break mc when running on pre-2.6.27 kernels as it "will fail with an EINVAL on kernels that do not support this interface".
comment:25 in reply to: ↑ 22 ; follow-up: ↓ 26 Changed 12 years ago by slyfox
Replying to pavlinux:
Replying to slyfox:
И подсунь ему файл размером 7 байт и посмотри на трейс:
Без тибя нашёл :-P
3-nd edition :)
http://pavlinux.ru/pipe2bench/pipe2bench2.c
http://pavlinux.ru/pipe2bench/Makefile;
http://pavlinux.ru/pipe2bench/MD5SUM;
83489098d5b6f13c324b5b5b972dcc3d Makefile
ddb4f9e4bbbc8d721a297cc66af247a3 pipe2bench2.c
Не собирается :p
comment:26 in reply to: ↑ 25 ; follow-up: ↓ 27 Changed 12 years ago by slyfox
Не собирается :p
Хедера sys/stat не хватает для S_ макросов.
В макросах pp() и pp2() фигурные скобки не сбалансированы.
Makefile вообще укуренный и собирает исходник не с тем именем.
target all должен первым идти, чтобы по умолчанию исполняемый
файл собирался.
Лучше на github :]
comment:27 in reply to: ↑ 26 ; follow-up: ↓ 28 Changed 12 years ago by pavlinux
Лучше на github :]
comment:28 in reply to: ↑ 27 ; follow-up: ↓ 29 Changed 12 years ago by slyfox
Replying to pavlinux:
Лучше на github :]
Чудно. Уже собирается, но всё равно нет различий :]
Измерять надо при первом обращении к файлу, дальше оно кэшируется.
То есть бенчмарк _вообще_ ничего не показывает.
Чтобы сделать его самодостаточным нужно перед каждым "тестом":
- всунуть "sync()"
- всунуть "echo 3 > /proc/sys/vm/drop_caches"
чтобы хотя-бы замерять одно и то же.
А то есть некоторые подозрения :]
comment:29 in reply to: ↑ 28 ; follow-up: ↓ 30 Changed 12 years ago by pavlinux
comment:30 in reply to: ↑ 29 Changed 12 years ago by slyfox
- Status changed from reopened to closed
- Resolution set to invalid
Конструктива 0.
Closing as INVALID as benchmark is Obviously Wrong :]
P.S.
O_DIRECT flag work only from kernel 3.4 and other backports (for ex. 3.2.19+)