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

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

2015-06-25 9:15 GMT-07:00 Slava Shwartsman <slavash () mellanox com>:

>  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:* wireshark-dev-bounces () wireshark org [mailto:
> wireshark-dev-bounces () wireshark org] *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 <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
___________________________________________________________________________
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