Re: [RFC-PATCH] IB: Create single conversation for Infiniband connections whenever possible

[Pascal Quantin <pascal.quantin () gmail com>]

Hi Slava,

Wireshark is not a project using an email based work flow for patch review
and merge (contrary to some other projects). Instead should register to our
Gerrit server https://code.wireshark.org/review and upload your patch as
explained here:
https://www.wireshark.org/docs/wsdg_html_chunked/ChSrcContribute.html
This way it will not be lost and will get reviewed by a core developer.

Best regards,
Pascal.
IB packets do not have source QP in each packet. For that reason
conversations created by infiniband dissector create one conversation
for each direction of the connection. This does not allow dissectors
above IB to be able to fully analyse the protocol such as matching
requests to responses. This behavior can be improved if our capture
includes CM establishment packets that contain both source and
destination QP numbers. The solution is to create a single conversation
and insert it into hash table with the keys that will be looked up by
dissectors. These keys woud be:
1) Source and destination QP and LID/GID
2) Source QP and LID/GID with -1 as destination QP
3) Destination QP and LID/GID with -1 as source QP

All above keys point to the same conversation.

Note that since there is no source, the actual QP number is passed as
destination port along with LID/GID as destination address.

Change-Id: I070e6d246b2c8818b0967b19f2e8548fe2eeeeb0
Signed-off-by: Slava Shwartsman <slavash () mellanox com>
---
 epan/conversation.c                 | 56
++++++++++++++++++++++++++++++-------
 epan/dissectors/packet-infiniband.c | 23 +++------------
 2 files changed, 50 insertions(+), 29 deletions(-)

diff --git a/epan/conversation.c b/epan/conversation.c
index 8595551..cc83359 100644
--- a/epan/conversation.c
+++ b/epan/conversation.c
@@ -548,16 +548,52 @@ conversation_init(void)
 static void
 conversation_insert_into_hashtable(GHashTable *hashtable, conversation_t
*conv)
 {
-       conversation_t *chain_head, *chain_tail, *cur, *prev;
-
-       chain_head = (conversation_t *)g_hash_table_lookup(hashtable,
conv->key_ptr);
-
-       if (NULL==chain_head) {
-               /* New entry */
-               conv->next = NULL;
-               conv->last = conv;
-               g_hash_table_insert(hashtable, conv->key_ptr, conv);
-               DPRINT(("created a new conversation chain"));
+    conversation_t *chain_head, *chain_tail, *cur, *prev;
+    conversation_key *src_qp_key;
+    conversation_key *dst_qp_key;
+
+    chain_head = (conversation_t *)g_hash_table_lookup(hashtable,
conv->key_ptr);
+
+    if (NULL==chain_head) {
+        /* New entry */
+        conv->next = NULL;
+        conv->last = conv;
+        DPRINT(("created a new conversation chain"));
+        if (!((hashtable == conversation_hashtable_exact)
+                && (conv->key_ptr->ptype == PT_IBQP)))
+               g_hash_table_insert(hashtable, conv->key_ptr, conv);
+        /*
+         * IB packets do not have source QP in each packet. For that reason
+         * conversations created by Infiniband dissector create one
conversation
+         * for each direction of the connection. This does not allow
dissectors
+         * above IB to be able to fully analyze the protocol such as
matching
+         * requests to responses. This behavior can be improved if our
capture
+         * includes CM establishment packets that contain both source and
+         * destination QP numbers. The solution is to create a single
conversation
+         * and insert it into hash table with the keys that will be looked
up by
+         * dissectors.
+         */
+        else {
+               src_qp_key = wmem_new(wmem_file_scope(), struct
conversation_key);
+               src_qp_key->next = conversation_keys;
+            conversation_keys = src_qp_key;
+            src_qp_key->addr1 = conv->key_ptr->addr1;
+            src_qp_key->addr2 = conv->key_ptr->addr2;
+            src_qp_key->ptype = conv->key_ptr->ptype;
+            src_qp_key->port1 = 0xffffffff;
+            src_qp_key->port2 = conv->key_ptr->port2;
+            g_hash_table_insert(hashtable, src_qp_key, conv);
+
+            dst_qp_key = wmem_new(wmem_file_scope(), struct
conversation_key);
+            dst_qp_key->next = conversation_keys;
+            conversation_keys = dst_qp_key;
+            dst_qp_key->addr1 = conv->key_ptr->addr2;
+            dst_qp_key->addr2 = conv->key_ptr->addr1;
+            dst_qp_key->ptype = conv->key_ptr->ptype;
+            dst_qp_key->port1 = 0xffffffff;
+            dst_qp_key->port2 = conv->key_ptr->port1;
+            g_hash_table_insert(hashtable, dst_qp_key, conv);
+               }
        }
        else {
                /* There's an existing chain for this key */
diff --git a/epan/dissectors/packet-infiniband.c
b/epan/dissectors/packet-infiniband.c
index 33fe8ea..9b0b5dd 100644
--- a/epan/dissectors/packet-infiniband.c
+++ b/epan/dissectors/packet-infiniband.c
@@ -1703,7 +1703,6 @@ skip_lrh:
                 dctBthHeader = TRUE;
                 bthSize += 8;
             }
-
             base_transport_header_item =
proto_tree_add_item(all_headers_tree, hf_infiniband_BTH, tvb, offset,
bthSize, ENC_NA);
             proto_item_set_text(base_transport_header_item, "%s", "Base
Transport Header");
             base_transport_header_tree =
proto_item_add_subtree(base_transport_header_item, ett_bth);
@@ -3098,33 +3097,20 @@ static void parse_COM_MGT(proto_tree *parentTree,
packet_info *pinfo, tvbuff_t *
                     proto_data = wmem_new(wmem_file_scope(),
conversation_infiniband_data);
                     proto_data->service_id = connection->service_id;

-                    /* RC traffic never(?) includes a field indicating the
source QPN, so
-                       the destination host knows it only from previous
context (a single
-                       QPN on the host that is part of an RC can only
receive traffic from
-                       that RC). For this reason we do not register
conversations with both
-                       sides, but rather we register the same conversation
twice - once for
-                       each side of the Reliable Connection. */
-
                     /* first register the conversation using the GIDs */
                     SET_ADDRESS(&req_addr, AT_IB, GID_SIZE,
connection->req_gid);
                     SET_ADDRESS(&resp_addr, AT_IB, GID_SIZE,
connection->resp_gid);

-                    conv = conversation_new(pinfo->fd->num, &req_addr,
&req_addr,
-                                            PT_IBQP, connection->req_qp,
connection->req_qp, NO_ADDR2|NO_PORT2);
-                    conversation_add_proto_data(conv, proto_infiniband,
proto_data);
-                    conv = conversation_new(pinfo->fd->num, &resp_addr,
&resp_addr,
-                                            PT_IBQP, connection->resp_qp,
connection->resp_qp, NO_ADDR2|NO_PORT2);
+                    conv = conversation_new(pinfo->fd->num,
&resp_addr,&req_addr,
+                        PT_IBQP, connection->resp_qp, connection->req_qp,
0);
                     conversation_add_proto_data(conv, proto_infiniband,
proto_data);

                     /* next, register the conversation using the LIDs */
                     SET_ADDRESS(&req_addr, AT_IB, sizeof(guint16),
&(connection->req_lid));
                     SET_ADDRESS(&resp_addr, AT_IB, sizeof(guint16),
&(connection->resp_lid));

-                    conv = conversation_new(pinfo->fd->num, &req_addr,
&req_addr,
-                                            PT_IBQP, connection->req_qp,
connection->req_qp, NO_ADDR2|NO_PORT2);
-                    conversation_add_proto_data(conv, proto_infiniband,
proto_data);
-                    conv = conversation_new(pinfo->fd->num, &resp_addr,
&resp_addr,
-                                            PT_IBQP, connection->resp_qp,
connection->resp_qp, NO_ADDR2|NO_PORT2);
+                    conv = conversation_new(pinfo->fd->num, &resp_addr,
&req_addr,
+                        PT_IBQP, connection->resp_qp, connection->req_qp,
0);
                     conversation_add_proto_data(conv, proto_infiniband,
proto_data);

                     g_hash_table_remove(CM_context_table, &hash_key);
@@ -3162,7 +3148,6 @@ static void parse_COM_MGT(proto_tree *parentTree,
packet_info *pinfo, tvbuff_t *
             proto_item_append_text(CM_header_item, " (Dissector Not
Implemented)"); local_offset += MAD_DATA_SIZE;
             break;
     }
-
     *offset = local_offset;
 }

--
1.8.3.1

___________________________________________________________________________
Sent via:    Wireshark-dev mailing list <wireshark-dev () wireshark org>
Archives:    https://www.wireshark.org/lists/wireshark-dev
Unsubscribe: https://wireshark.org/mailman/options/wireshark-dev
             mailto:wireshark-dev-request () wireshark org?subject=unsubscribe

___________________________________________________________________________
Sent via:    Wireshark-dev mailing list <wireshark-dev () wireshark org>
Archives:    https://www.wireshark.org/lists/wireshark-dev
Unsubscribe: https://wireshark.org/mailman/options/wireshark-dev
             mailto:wireshark-dev-request () wireshark org?subject=unsubscribe