Wireshark · Wireshark-dev: Re: [Wireshark-dev] [RFC-PATCH] IB: Create single conversation for Infiniband connections whenever p

[Pascal Quantin <pascal.quantin@xxxxxxxxx>]

Le 6 juil. 2015 8:01 AM, "Slava Shwartsman" <slavash@xxxxxxxxxxxx> a écrit :
>
> Hi Pascal,
>
> Seems like my concerns become true.
>
> I uploaded the patch more than a week  ago, and no reviews were made.
>
> https://code.wireshark.org/review/#/c/9210/
>
>  
>
> Would you like to assist please?
>
>  

Hi Slava,

This is exactly why I recommended you to upload the patch on Gerrit.
Developers are volunteers doing the review on their own limited spare time with a huge backlog. And not of us know anything about Infiniband (I do not for sure).
Sending an email is the risk of not getting any feedback (as you saw).
On the opposite uploading a patch gives a chance to somebody, when he has time, to look at your work. Creating a bug on https://bugs.wireshark.org with a pcap file demonstrating the issue is even better.

Best regards,
Pascal.

>
>  
>
>  
>
>  
>
> Thanks in advance,
>
> Slava
>
>  
>
> From: wireshark-dev-bounces@xxxxxxxxxxxxx [mailto:wireshark-dev-bounces@xxxxxxxxxxxxx] On Behalf Of Pascal Quantin
> Sent: Thursday, June 25, 2015 19:17
>
> To: Developer support list for Wireshark
> Subject: Re: [Wireshark-dev] [RFC-PATCH] IB: Create single conversation for Infiniband connections whenever possible
>
>  
>
>  
>
>  
>
> 2015-06-25 9:15 GMT-07:00 Slava Shwartsman <slavash@xxxxxxxxxxxx>:
>>
>> 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@xxxxxxxxxxxxx [mailto:wireshark-dev-bounces@xxxxxxxxxxxxx] 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@xxxxxxxxxxxx>
>> ---
>>  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@xxxxxxxxxxxxx>
>> Archives:    https://www.wireshark.org/lists/wireshark-dev
>> Unsubscribe: https://wireshark.org/mailman/options/wireshark-dev
>>              mailto:wireshark-dev-request@xxxxxxxxxxxxx?subject=unsubscribe
>>
>>
>> ___________________________________________________________________________
>> Sent via:    Wireshark-dev mailing list <wireshark-dev@xxxxxxxxxxxxx>
>> Archives:    https://www.wireshark.org/lists/wireshark-dev
>> Unsubscribe: https://wireshark.org/mailman/options/wireshark-dev
>>              mailto:wireshark-dev-request@xxxxxxxxxxxxx?subject=unsubscribe
>
>  
>
>
> ___________________________________________________________________________
> Sent via:    Wireshark-dev mailing list <wireshark-dev@xxxxxxxxxxxxx>
> Archives:    https://www.wireshark.org/lists/wireshark-dev
> Unsubscribe: https://wireshark.org/mailman/options/wireshark-dev
>              mailto:wireshark-dev-request@xxxxxxxxxxxxx?subject=unsubscribe