[PULL,3/8] colo-compare: compare the packet in a specified Connection

Message ID 1510625498-4821-4-git-send-email-jasowang@redhat.com
State New
Headers show
Series
  • [PULL,1/8] net: fix check for number of parameters to -netdev socket
Related show

Commit Message

Jason Wang Nov. 14, 2017, 2:11 a.m.
From: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>

A package from pri_indev or sec_indev only belongs to a particular
Connection, so we only need to compare the package in the specified
Connection's primary_list and secondary_list, rather than for each
the whole Connection list to compare. This is time-consuming and
unnecessary.

Less checkpoint more efficiency.

Cc: Zhang Chen <zhangckid@gmail.com>
Cc: Li Zhijian <lizhijian@cn.fujitsu.com>
Cc: Jason Wang <jasowang@redhat.com>
Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 net/colo-compare.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

Comments

Peter Maydell Nov. 15, 2017, 6:57 p.m. | #1
On 14 November 2017 at 02:11, Jason Wang <jasowang@redhat.com> wrote:
> From: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
>
> A package from pri_indev or sec_indev only belongs to a particular
> Connection, so we only need to compare the package in the specified
> Connection's primary_list and secondary_list, rather than for each
> the whole Connection list to compare. This is time-consuming and
> unnecessary.
>
> Less checkpoint more efficiency.
>
> Cc: Zhang Chen <zhangckid@gmail.com>
> Cc: Li Zhijian <lizhijian@cn.fujitsu.com>
> Cc: Jason Wang <jasowang@redhat.com>
> Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
>  net/colo-compare.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/net/colo-compare.c b/net/colo-compare.c
> index 54b6347..5d2429b 100644
> --- a/net/colo-compare.c
> +++ b/net/colo-compare.c
> @@ -136,7 +136,7 @@ static int colo_insert_packet(GQueue *queue, Packet *pkt)
>   * Return 0 on success, if return -1 means the pkt
>   * is unsupported(arp and ipv6) and will be sent later
>   */
> -static int packet_enqueue(CompareState *s, int mode)
> +static int packet_enqueue(CompareState *s, int mode, Connection **con)
>  {
>      ConnectionKey key;
>      Packet *pkt = NULL;
> @@ -179,6 +179,7 @@ static int packet_enqueue(CompareState *s, int mode)
>                           "drop packet");
>          }
>      }
> +    con = &conn;
>
>      return 0;
>  }

Coverity points out that this looks a bit fishy --
presumably you meant
  *con = conn;

? The statement you have now doesn't do anything, since
'con' is unused after you change it.

(CID 1382804.)

thanks
-- PMM
Mao Zhongyi Nov. 16, 2017, 1:32 a.m. | #2
On 11/16/2017 02:57 AM, Peter Maydell wrote:
> On 14 November 2017 at 02:11, Jason Wang <jasowang@redhat.com> wrote:
>> From: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
>>
>> A package from pri_indev or sec_indev only belongs to a particular
>> Connection, so we only need to compare the package in the specified
>> Connection's primary_list and secondary_list, rather than for each
>> the whole Connection list to compare. This is time-consuming and
>> unnecessary.
>>
>> Less checkpoint more efficiency.
>>
>> Cc: Zhang Chen <zhangckid@gmail.com>
>> Cc: Li Zhijian <lizhijian@cn.fujitsu.com>
>> Cc: Jason Wang <jasowang@redhat.com>
>> Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>> ---
>>  net/colo-compare.c | 13 ++++++++-----
>>  1 file changed, 8 insertions(+), 5 deletions(-)
>>
>> diff --git a/net/colo-compare.c b/net/colo-compare.c
>> index 54b6347..5d2429b 100644
>> --- a/net/colo-compare.c
>> +++ b/net/colo-compare.c
>> @@ -136,7 +136,7 @@ static int colo_insert_packet(GQueue *queue, Packet *pkt)
>>   * Return 0 on success, if return -1 means the pkt
>>   * is unsupported(arp and ipv6) and will be sent later
>>   */
>> -static int packet_enqueue(CompareState *s, int mode)
>> +static int packet_enqueue(CompareState *s, int mode, Connection **con)
>>  {
>>      ConnectionKey key;
>>      Packet *pkt = NULL;
>> @@ -179,6 +179,7 @@ static int packet_enqueue(CompareState *s, int mode)
>>                           "drop packet");
>>          }
>>      }
>> +    con = &conn;
>>
>>      return 0;
>>  }

Hi, Peter

> Coverity points out that this looks a bit fishy --
> presumably you meant
>   *con = conn;
>

Well´╝îI will fix it right away.

> ? The statement you have now doesn't do anything, since
> 'con' is unused after you change it.

It will be used in colo_compare_connection.

Thanks,
Mao

Patch

diff --git a/net/colo-compare.c b/net/colo-compare.c
index 54b6347..5d2429b 100644
--- a/net/colo-compare.c
+++ b/net/colo-compare.c
@@ -136,7 +136,7 @@  static int colo_insert_packet(GQueue *queue, Packet *pkt)
  * Return 0 on success, if return -1 means the pkt
  * is unsupported(arp and ipv6) and will be sent later
  */
-static int packet_enqueue(CompareState *s, int mode)
+static int packet_enqueue(CompareState *s, int mode, Connection **con)
 {
     ConnectionKey key;
     Packet *pkt = NULL;
@@ -179,6 +179,7 @@  static int packet_enqueue(CompareState *s, int mode)
                          "drop packet");
         }
     }
+    con = &conn;
 
     return 0;
 }
@@ -728,8 +729,9 @@  static void compare_set_vnet_hdr(Object *obj,
 static void compare_pri_rs_finalize(SocketReadState *pri_rs)
 {
     CompareState *s = container_of(pri_rs, CompareState, pri_rs);
+    Connection *conn = NULL;
 
-    if (packet_enqueue(s, PRIMARY_IN)) {
+    if (packet_enqueue(s, PRIMARY_IN, &conn)) {
         trace_colo_compare_main("primary: unsupported packet in");
         compare_chr_send(s,
                          pri_rs->buf,
@@ -737,19 +739,20 @@  static void compare_pri_rs_finalize(SocketReadState *pri_rs)
                          pri_rs->vnet_hdr_len);
     } else {
         /* compare connection */
-        g_queue_foreach(&s->conn_list, colo_compare_connection, s);
+        colo_compare_connection(conn, s);
     }
 }
 
 static void compare_sec_rs_finalize(SocketReadState *sec_rs)
 {
     CompareState *s = container_of(sec_rs, CompareState, sec_rs);
+    Connection *conn = NULL;
 
-    if (packet_enqueue(s, SECONDARY_IN)) {
+    if (packet_enqueue(s, SECONDARY_IN, &conn)) {
         trace_colo_compare_main("secondary: unsupported packet in");
     } else {
         /* compare connection */
-        g_queue_foreach(&s->conn_list, colo_compare_connection, s);
+        colo_compare_connection(conn, s);
     }
 }