performance optimization: split all the members of dpvs connection into two groups for its initialization. #598

andrewhit · 2020-06-26T10:28:32Z

TCP CPS can be increased by 10% for the single core with TCP flood traffic
sent by hping3.

ywc689 · 2020-07-19T14:51:35Z

src/ipvs/ip_vs_conn.c

@@ -98,7 +98,6 @@ static struct dp_vs_conn *dp_vs_conn_alloc(enum dpvs_fwd_mode fwdmode,
        return NULL;
    }

-    memset(conn, 0, sizeof(struct dp_vs_conn));


Is it safe to use uninitialized dp_vs_conn? Did you examine every field of dp_vs_conn and check whether it may be used uninitialized?

It has been tested and no issue is found.

I don't think uninitialized dp_vs_conn is good idea. Someday add some field in dp_vs_conn, it may lead problem.

For uninitialized dp_vs_conn maybe we can do something in dp_vs_conn_free before invoke rte_mempool_put?

fnat_seq needs to be initialized as 0, otherwise it brings the fault to tcp connections. Additionally, double assignment for the same field of dpvs connection is not good to CPS performance.

ywc689 · 2020-07-28T02:33:33Z

DPVS crashed after a 2-hours stress test using this patch. Traces show mbuf is tainted.

#0  0x00007ffff6b315f7 in raise () from /lib64/libc.so.6
#1  0x00007ffff6b32ce8 in abort () from /lib64/libc.so.6
#2  0x00007ffff6b2a566 in __assert_fail_base () from /lib64/libc.so.6
#3  0x00007ffff6b2a612 in __assert_fail () from /lib64/libc.so.6
#4  0x0000000000689b51 in netif_port_get (id=65535) at /home/wencyu/github/dpvs/src/netif.c:3173
#5  0x0000000000687258 in lcore_process_packets (qconf=0x14d8a80 <lcore_conf+3551552>, mbufs=0x14d8a90 <lcore_conf+3551568>, cid=4 '\004', count=3, pkts_from_ring=false) at /home/wencyu/github/dpvs/src/netif.c:2325
#6  0x0000000000687cd7 in lcore_job_recv_fwd (arg=0x0) at /home/wencyu/github/dpvs/src/netif.c:2442
#7  0x00000000006961c3 in do_lcore_job (job=0xc61040 <netif_jobs>) at /home/wencyu/github/dpvs/src/scheduler.c:153
#8  0x00000000006962f7 in dpvs_job_loop (arg=0x0) at /home/wencyu/github/dpvs/src/scheduler.c:201
#9  0x0000000000548894 in eal_thread_loop ()
#10 0x00007ffff70d0dc5 in start_thread () from /lib64/libpthread.so.0
#11 0x00007ffff6bf221d in clone () from /lib64/libc.so.6

andrewhit · 2020-07-28T06:39:28Z

DPVS crashed after a 2-hours stress test using this patch. Traces show mbuf is tainted.

#0  0x00007ffff6b315f7 in raise () from /lib64/libc.so.6
#1  0x00007ffff6b32ce8 in abort () from /lib64/libc.so.6
#2  0x00007ffff6b2a566 in __assert_fail_base () from /lib64/libc.so.6
#3  0x00007ffff6b2a612 in __assert_fail () from /lib64/libc.so.6
#4  0x0000000000689b51 in netif_port_get (id=65535) at /home/wencyu/github/dpvs/src/netif.c:3173
#5  0x0000000000687258 in lcore_process_packets (qconf=0x14d8a80 <lcore_conf+3551552>, mbufs=0x14d8a90 <lcore_conf+3551568>, cid=4 '\004', count=3, pkts_from_ring=false) at /home/wencyu/github/dpvs/src/netif.c:2325
#6  0x0000000000687cd7 in lcore_job_recv_fwd (arg=0x0) at /home/wencyu/github/dpvs/src/netif.c:2442
#7  0x00000000006961c3 in do_lcore_job (job=0xc61040 <netif_jobs>) at /home/wencyu/github/dpvs/src/scheduler.c:153
#8  0x00000000006962f7 in dpvs_job_loop (arg=0x0) at /home/wencyu/github/dpvs/src/scheduler.c:201
#9  0x0000000000548894 in eal_thread_loop ()
#10 0x00007ffff70d0dc5 in start_thread () from /lib64/libpthread.so.0
#11 0x00007ffff6bf221d in clone () from /lib64/libc.so.6

Which test case are you running? Do you have the backtrace in detail?

ywc689 · 2020-07-28T07:57:03Z

I patched the codes to the lastest devel branch, and tested it using the following the fullnat services. Run the case with 1Gbps/70Wpps HTTP traffic in total, dpvs crashed after serveral hours.

[root@~ dpvs]# ipvsadm -ln
IP Virtual Server version 0.0.0 (size=0)
Prot LocalAddress:Port Scheduler Flags
  -> RemoteAddress:Port           Forward Weight ActiveConn InActConn
TCP  192.168.88.30:80 wrr persistent 15 synproxy conn_timeout 5
  -> 192.168.88.15:80             FullNat 100    0          0         
  -> 192.168.88.115:80            FullNat 100    89         16802     
  -> 192.168.88.215:80            FullNat 100    0          0         
TCP  192.168.88.32:80 rr
  -> 192.168.88.15:80             FullNat 100    29         1688      
  -> 192.168.88.115:80            FullNat 100    32         1684      
  -> 192.168.88.215:80            FullNat 100    29         1690      
TCP  [2001::30]:8080 conhash sip synproxy
  -> 192.168.88.15:80             FullNat 100    89         7500      
  -> 192.168.88.115:80            FullNat 100    0          0         
  -> 192.168.88.215:80            FullNat 100    0          0         
[root@~ dpvs]# dpip link show -s
1: dpdk0: socket 0 mtu 1500 rx-queue 8 tx-queue 8
    UP 10000 Mbps full-duplex auto-nego 
    addr A0:36:9F:74:EC:F0 OF_RX_IP_CSUM OF_TX_IP_CSUM OF_TX_TCP_CSUM OF_TX_UDP_CSUM 
    ipackets            opackets            ibytes              obytes              
    15164676896         15212314092         2989493919579       2994458765519       
    ierrors             oerrors             imissed             rx_nombuf           
    0                   0                   0                   0                   
    mbuf-avail          mbuf-inuse          
    507958              16329

andrewhit · 2020-07-29T05:43:22Z

Okay. We will duplicate this issue and fix it.

andrewhit · 2020-07-29T08:13:51Z

It seems related to the below configuration and it is day 1 issue. Have you tested without the patch of this PR?

# persistence_timeout 15

ywc689 · 2020-07-29T09:06:37Z

I tested the current devel branch codes with this case. It ran about 2 days no crash.

andrewhit · 2020-08-05T14:01:55Z

I have figured out some solution which will be sent out soon.

andrewhit · 2020-08-11T03:48:23Z

With this patch, TCP CPS can be increased by 10% with 14 CPU cores used.

ywc689

It seems that many members of dp_vs_conn are still zerod with memset. Are you sure the latest patch make 10% performance enhancement?

Besides, function pointer members (such as packet_xmit, packet_out_xmit) should always be zerod, because it's a disater if they are not assigned before use.

include/ipvs/conn.h

src/ipvs/ip_vs_conn.c

andrewhit · 2020-08-23T12:22:18Z

It seems that many members of dp_vs_conn are still zerod with memset. Are you sure the latest patch make 10% performance enhancement?

Besides, function pointer members (such as packet_xmit, packet_out_xmit) should always be zerod, because it's a disater if they are not assigned before use.

packet_xmit and packet_out_xmit() is initialized within dp_vs_conn_bind_dest() invoked by dp_vs_conn_new().

andrewhit · 2020-08-23T12:50:58Z

Please have a check on the latest patch.

ywc689 · 2020-08-25T07:34:31Z

@andrewhit The latest patch itself is all OK now. But I'm concerned about its easy use and safety. For example, just as what you say, packet_xmit and packet_out_xmit are initialized in dp_vs_conn_bind_dest, the initialization of the two function pointers are determined by other modules outside of dp_vs_conn_new. This a not a good practice of defensive programming.
We'll evalute the preforamnce, and determine whether adopt this PR or not later.

- TCP CPS can be increased by 10% for the single core with TCP flood traffic sent by hping3.

initialization and it's for the benefit of CPS. a) Remove the operation of bezeroing all the members of dpvs connection within dp_vs_conn_alloc(). b) Each member of dpvs connection is initialized ONCE via dp_vs_conn_pre_init() or dp_vs_conn_new(). - dp_vs_conn_pre_init() initializes all the members of one group as 0, but ack_mbuf. - dp_vs_conn_new() initializes all the members of the other group with the specific values.

ywc689 reviewed Jul 19, 2020

View reviewed changes

ywc689 added the pr/to-confirm-needs consider whether the feature of pr is needed label Jul 19, 2020

ywc689 approved these changes Jul 20, 2020

View reviewed changes

ywc689 added pr/to-test-codes Compile and test the patch of pr to verify if it works. pr/needs-confirmed the feature in the pr is what we need，and list what cases should be checked in later stages and removed pr/to-confirm-needs consider whether the feature of pr is needed labels Jul 20, 2020

ywc689 added pr/codes-test-failed compile problem exists or specified test cases not passed and removed pr/to-test-codes Compile and test the patch of pr to verify if it works. labels Jul 28, 2020

andrewhit closed this Aug 5, 2020

andrewhit deleted the dpvs-bug-21 branch August 5, 2020 14:07

andrewhit restored the dpvs-bug-21 branch August 6, 2020 03:27

andrewhit reopened this Aug 6, 2020

andrewhit changed the title ~~ipvs: no need to zero the bytes of the newly allocated dpvs connection.~~ ipvs: split all the members of dpvs connection into two groups for its initialization for the benefit of CPS. Aug 10, 2020

andrewhit changed the title ~~ipvs: split all the members of dpvs connection into two groups for its initialization for the benefit of CPS.~~ performance optimization: split all the members of dpvs connection into two groups for its initialization. Aug 10, 2020

andrewhit force-pushed the dpvs-bug-21 branch from e4afc47 to 1599667 Compare August 10, 2020 12:15

andrewhit force-pushed the dpvs-bug-21 branch from 1599667 to 6c2671c Compare August 11, 2020 03:55

ywc689 reviewed Aug 21, 2020

View reviewed changes

include/ipvs/conn.h Show resolved Hide resolved

src/ipvs/ip_vs_conn.c Show resolved Hide resolved

andrewhit force-pushed the dpvs-bug-21 branch from 6c2671c to 6d2e80b Compare August 23, 2020 12:35

ywc689 added the pr/to-evaluate-performance the pr is required a performance evaluation label Aug 25, 2020

ywc689 removed the pr/codes-test-failed compile problem exists or specified test cases not passed label Aug 25, 2020

zhuangyan added 2 commits August 3, 2022 10:47

ipvs: no need to zero the bytes of the newly allocated dpvs connection.

c393ca2

- TCP CPS can be increased by 10% for the single core with TCP flood traffic sent by hping3.

andrewhit force-pushed the dpvs-bug-21 branch from 6d2e80b to df1460d Compare August 3, 2022 04:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

performance optimization: split all the members of dpvs connection into two groups for its initialization. #598

performance optimization: split all the members of dpvs connection into two groups for its initialization. #598

andrewhit commented Jun 26, 2020

ywc689 Jul 19, 2020

andrewhit Jul 20, 2020

SimonCZW Jul 29, 2020

andrewhit Aug 10, 2020

ywc689 commented Jul 28, 2020

andrewhit commented Jul 28, 2020 •

edited

Loading

ywc689 commented Jul 28, 2020

andrewhit commented Jul 29, 2020

andrewhit commented Jul 29, 2020 •

edited

Loading

ywc689 commented Jul 29, 2020

andrewhit commented Aug 5, 2020 •

edited

Loading

andrewhit commented Aug 11, 2020

ywc689 left a comment •

edited

Loading

andrewhit commented Aug 23, 2020

andrewhit commented Aug 23, 2020

ywc689 commented Aug 25, 2020

performance optimization: split all the members of dpvs connection into two groups for its initialization. #598

Are you sure you want to change the base?

performance optimization: split all the members of dpvs connection into two groups for its initialization. #598

Conversation

andrewhit commented Jun 26, 2020

ywc689 Jul 19, 2020

Choose a reason for hiding this comment

andrewhit Jul 20, 2020

Choose a reason for hiding this comment

SimonCZW Jul 29, 2020

Choose a reason for hiding this comment

andrewhit Aug 10, 2020

Choose a reason for hiding this comment

ywc689 commented Jul 28, 2020

andrewhit commented Jul 28, 2020 • edited Loading

ywc689 commented Jul 28, 2020

andrewhit commented Jul 29, 2020

andrewhit commented Jul 29, 2020 • edited Loading

ywc689 commented Jul 29, 2020

andrewhit commented Aug 5, 2020 • edited Loading

andrewhit commented Aug 11, 2020

ywc689 left a comment • edited Loading

Choose a reason for hiding this comment

andrewhit commented Aug 23, 2020

andrewhit commented Aug 23, 2020

ywc689 commented Aug 25, 2020

andrewhit commented Jul 28, 2020 •

edited

Loading

andrewhit commented Jul 29, 2020 •

edited

Loading

andrewhit commented Aug 5, 2020 •

edited

Loading

ywc689 left a comment •

edited

Loading