2015-06-25 9:15 GMT-07:00 Slava Shwartsman <[email protected]>: > Hello Pascal, > > Thank you for the quick response. > > > > The main reason I wanted to send this patch via mail thread is because I > wanted to developers to take a look at this patch and maybe see things > differently and get comments on the way I implemented my solution for the > problem – this is why I used the RFC flag in the header. > > > > This patch affects the way InfiniBand packets are being dissected. >
This is also applicable for RFC patches. Gerrit has a lot more convenient framework for reviewing / commenting / amending patches. > > Thank you for reminding me about the Gerrit server – I ‘have used it > before. > > > > *From:* [email protected] [mailto: > [email protected]] *On Behalf Of *Pascal Quantin > *Sent:* Thursday, June 25, 2015 16:41 > *To:* Developer support list for Wireshark > *Subject:* Re: [Wireshark-dev] [RFC-PATCH] IB: Create single conversation > for Infiniband connections whenever possible > > > > 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 <[email protected]> > --- > 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 <[email protected]> > Archives: https://www.wireshark.org/lists/wireshark-dev > Unsubscribe: https://wireshark.org/mailman/options/wireshark-dev > mailto:[email protected] > ?subject=unsubscribe > > ___________________________________________________________________________ > Sent via: Wireshark-dev mailing list <[email protected]> > Archives: https://www.wireshark.org/lists/wireshark-dev > Unsubscribe: https://wireshark.org/mailman/options/wireshark-dev > mailto:[email protected] > ?subject=unsubscribe >
___________________________________________________________________________ Sent via: Wireshark-dev mailing list <[email protected]> Archives: https://www.wireshark.org/lists/wireshark-dev Unsubscribe: https://wireshark.org/mailman/options/wireshark-dev mailto:[email protected]?subject=unsubscribe
