[ovs-dev] [PATCH] odp-execute: Reuse rss hash in OVS_ACTION_ATTR_HASH.

[Ilya Maximets]

On 12.07.2017 10:15, Andy Zhou wrote:
> On Tue, Jul 11, 2017 at 7:30 AM, Ilya Maximets <i.maximets at samsung.com> wrote:
>> If RSS hash exists in a packet it can be reused instead of
>> 5 tuple hash re-calculation in OVS_ACTION_ATTR_HASH. This
>> leads to increasing the performance of sending packets to
>> the OVS bonding in userspace datapath up to 10-15%.
>>
>> Signed-off-by: Ilya Maximets <i.maximets at samsung.com>
>> ---
>>  lib/odp-execute.c | 13 +++++++++++--
>>  1 file changed, 11 insertions(+), 2 deletions(-)
>>
>> diff --git a/lib/odp-execute.c b/lib/odp-execute.c
>> index d656334..471a364 100644
>> --- a/lib/odp-execute.c
>> +++ b/lib/odp-execute.c
>> @@ -646,8 +646,17 @@ odp_execute_actions(void *dp, struct dp_packet_batch *batch, bool steal,
>>                  uint32_t hash;
>>
>>                  DP_PACKET_BATCH_FOR_EACH (packet, batch) {
>> -                    flow_extract(packet, &flow);
>> -                    hash = flow_hash_5tuple(&flow, hash_act->hash_basis);
>> +                    /* RSS hash can be used here instead of 5tuple for
>> +                     * performance reasons. */
>> +                    if (dp_packet_rss_valid(packet)) {
>> +                        hash = dp_packet_get_rss_hash(packet);
> 
>> +                        if (hash_act->hash_basis) {
>> +                            hash = hash_finish(hash, hash_act->hash_basis);
>> +                        }
> 
> This is not a full review. I have some comments on the 3 lines of code above.
> 
> Would it make more sense to always include 'hash_basis'?  Also it
> seems hash_int() would be more appropriate than hash_finish().
> 
> I suppose the performance gain may not be as significant. On the other
> hand, I am not sure if we should count on the performance
> gain by assuming hash_basis is always zero.

I performed few tests with set and not set hash basis and found
no significant performance difference. Also I checked your
suggestion with the following incremental applied:
-----------------------------------------------------------------------------
-                        if (hash_act->hash_basis) {
-                            hash = hash_finish(hash, hash_act->hash_basis);
-                        }
+                        hash = hash_int(hash, hash_act->hash_basis);
-----------------------------------------------------------------------------

This also doesn't affect performance significantly.

I agree that code without assumptions on hash_basis value looks more correct.
So, I will send v2 with that change. Also, I fixed one unit test that depends
on dp_hash value. I'll include this fix in v2 too.

> 
>> +                    } else {
>> +                        flow_extract(packet, &flow);
>> +                        hash = flow_hash_5tuple(&flow, hash_act->hash_basis);
>> +                    }
>>                      packet->md.dp_hash = hash;
>>                  }
>>              } else {
>> --
>> 2.7.4
>>
> 
> 
>