Ticket #2949 (closed enhancement: invalid)

Opened 12 years ago

Last modified 8 years ago

[ 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) 
    340340void 
    341341open_error_pipe (void) 
    342342{ 
    343     if (pipe (error_pipe) < 0) 
     343    if (pipe2(error_pipe, O_DIRECT) < 0) 
    344344    { 
    345345        message (D_NORMAL, _("Warning"), _("Pipe failed")); 
    346346    } 

  • 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) 
    514514    int back_comm[2];           /* back connection */ 
    515515    pid_t pid; 
    516516 
    517     if (pipe (comm) == -1) 
     517    if (pipe2(comm, O_DIRECT) == -1) 
    518518        return -1; 
    519519 
    520     if (pipe (back_comm) == -1) 
     520    if (pipe2(back_comm, O_DIRECT) == -1) 
    521521        return -1; 
    522522 
    523523    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) 
    148148        status = close (pipefd1[1]); 
    149149        status = close (pipefd2[0]); 
    150150        /* 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))) 
    152152        { 
    153153            mc_global.tty.console_flag = '\0'; 
    154154            break; 

  • src/subshell.c 

    diff --git a/src/subshell.c b/src/subshell.c
index 510cf4d..b19117a 100644
    a b init_subshell (void) 
    850850                return; 
    851851            } 
    852852        } 
    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)) 
    854854        { 
    855855            perror (__FILE__ ": couldn't create pipe"); 
    856856            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[]) 
    312312    int fileset1[2], fileset2[2]; 
    313313    int res; 
    314314 
    315     if ((pipe (fileset1) < 0) || (pipe (fileset2) < 0)) 
     315    if ((pipe2(fileset1, O_DIRECT) < 0) || (pipe2(fileset2, O_DIRECT) < 0)) 
    316316        vfs_die ("Cannot pipe(): %m."); 
    317317 
    318318    res = fork (); 

Attachments

PIPE2.patch​ (3.1 KB) - added by pavlinux 12 years ago.

Change History

comment:1 Changed 12 years ago by pavlinux

P.S.

O_DIRECT flag work only from kernel 3.4 and other backports (for ex. 3.2.19+)

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:3 Changed 12 years ago by andrew_b

  • Description modified (diff)

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
Last edited 12 years ago by pavlinux (previous) (diff)

comment:6 Changed 12 years ago by pavlinux

In general, works for me, and you're there yourself somehow ]:->

Version 0, edited 12 years ago by pavlinux (next)

Changed 12 years ago by pavlinux

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) 
    340340void 
    341341open_error_pipe (void) 
    342342{ 
    343     if (pipe (error_pipe) < 0) 
     343    if (mc_pipe(error_pipe) < 0) 
    344344    { 
    345345        message (D_NORMAL, _("Warning"), _("Pipe failed")); 
    346346    } 

  • 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) 
    514514    int back_comm[2];           /* back connection */ 
    515515    pid_t pid; 
    516516 
    517     if (pipe (comm) == -1) 
     517    if (mc_pipe(comm) == -1) 
    518518        return -1; 
    519519 
    520     if (pipe (back_comm) == -1) 
     520    if (mc_pipe(back_comm) == -1) 
    521521        return -1; 
    522522 
    523523    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) 
    148148        status = close (pipefd1[1]); 
    149149        status = close (pipefd2[0]); 
    150150        /* Create two pipes for communication */ 
    151         if (!((pipe (pipefd1) == 0) && ((pipe (pipefd2)) == 0))) 
     151        if (!((mc_pipe(pipefd1) == 0) && ((mc_pipe(pipefd2)) == 0))) 
    152152        { 
    153153            mc_global.tty.console_flag = '\0'; 
    154154            break; 

  • src/subshell.c 

    diff --git a/src/subshell.c b/src/subshell.c
index 510cf4d..00ca85c 100644
    a b init_subshell (void) 
    850850                return; 
    851851            } 
    852852        } 
    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)) 
    854854        { 
    855855            perror (__FILE__ ": couldn't create pipe"); 
    856856            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[]) 
    312312    int fileset1[2], fileset2[2]; 
    313313    int res; 
    314314 
    315     if ((pipe (fileset1) < 0) || (pipe (fileset2) < 0)) 
     315    if ((mc_pipe(fileset1) < 0) || (mc_pipe(fileset2) < 0)) 
    316316        vfs_die ("Cannot pipe(): %m."); 
    317317 
    318318    res = fork (); 

  • lib/global.h 

    diff --git a/lib/global.h b/lib/global.h
index f13dfc0..0655c1e 100644
    a b typedef struct 
    283283    } vfs; 
    284284} mc_global_t; 
    285285 
     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 
    286297/*** global variables defined in .c file *********************************************************/ 
    287298 
    288299extern 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) + if ((pipe2(fileset1, O_DIRECT) < 0)
(pipe (fileset2) < 0))
(pipe2(fileset2, O_DIRECT) < 0))

Care to explain benefits?

The O_DIRECT addition pessimizes pipe I/O by requiring more read()s:

​https://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=commitdiff;h=9883035ae7edef3ec62ad215611cb8e17d6a1a5d

Last edited 12 years ago by slyfox (previous) (diff)

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...
Last edited 12 years ago by pavlinux (previous) (diff)

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>
Last edited 12 years ago by pavlinux (previous) (diff)

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 :)

​http://pavlinux.ru/pipe2bench/pipe2bench2.c

И подсунь ему файл размером 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

Last edited 12 years ago by pavlinux (previous) (diff)

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();

Измерять надо при первом обращении к файлу, дальше оно кэшируется.

Last edited 12 years ago by pavlinux (previous) (diff)

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 :]

​https://github.com/pavlinux/pipe2bench

comment:28 in reply to: ↑ 27 ; follow-up: ↓ 29 Changed 12 years ago by slyfox

Replying to pavlinux:

Лучше на github :]

​https://github.com/pavlinux/pipe2bench

Чудно. Уже собирается, но всё равно нет различий :]

Измерять надо при первом обращении к файлу, дальше оно кэшируется.

То есть бенчмарк _вообще_ ничего не показывает.
Чтобы сделать его самодостаточным нужно перед каждым "тестом":

  • всунуть "sync()"
  • всунуть "echo 3 > /proc/sys/vm/drop_caches"

чтобы хотя-бы замерять одно и то же.

А то есть некоторые подозрения :]

comment:29 in reply to: ↑ 28 ; follow-up: ↓ 30 Changed 12 years ago by pavlinux

Replying to slyfox:

Replying to pavlinux:

Лучше на github :]

​https://github.com/pavlinux/pipe2bench

Чудно. Уже собирается, но всё равно нет различий :]

Измерять надо при первом обращении к файлу, дальше оно кэшируется.

То есть бенчмарк _вообще_ ничего не показывает.

Ты в реальной жизни один и тот же файл тоже по 2 раза копируешь?

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 :]

comment:31 Changed 8 years ago by andrew_b

  • Milestone Future Releases deleted
Note: See TracTickets for help on using tickets.