[ovs-dev] [slow path HASH and RECIRC action v2] dpif: Fix slow action handling for DP_HASH and RECIRC

Andy Zhou azhou at nicira.com
Wed Jun 4 23:57:04 UTC 2014


Thanks for the review. Pushed to master and branch-2.3

On Tue, Jun 3, 2014 at 11:30 PM, YAMAMOTO Takashi
<yamamoto at valinux.co.jp> wrote:
>> In case DP_HASH and RECIRC actions need to be executed in slow path,
>> current implementation simply don't handle them -- vswitchd simply
>> crashes. This patch fixes them by supply an implementation for them.
>>
>> RECIRC will be handled by the datapath, same as the output action.
>>
>> DP_HASH, on the other hand, is handled in the user space. Although the
>> resulting hash values may not match those computed by the datapath, it
>> is less expensive; current use case (bonding) does not require a strict
>> match to work properly.
>>
>> Reported-by: YAMAMOTO Takashi <yamamoto at valinux.co.jp>
>> Signed-off-by: Andy Zhou <azhou at nicira.com>
>
> Acked-by: YAMAMOTO Takashi <yamamoto at valinux.co.jp>
>
>>
>> ---
>> v1->v2:   Addresses Ben's review feedback
>>           fix the comment on why execute dp_hash in user space.
>>         Add assert for unknown hash algorithm specification.
>> ---
>>  lib/dpif.c        |  4 ++--
>>  lib/odp-execute.c | 23 ++++++++++++++++++++++-
>>  2 files changed, 24 insertions(+), 3 deletions(-)
>>
>> diff --git a/lib/dpif.c b/lib/dpif.c
>> index 84dba28..ac73be1 100644
>> --- a/lib/dpif.c
>> +++ b/lib/dpif.c
>> @@ -1069,6 +1069,7 @@ dpif_execute_helper_cb(void *aux_, struct ofpbuf *packet,
>>      switch ((enum ovs_action_attr)type) {
>>      case OVS_ACTION_ATTR_OUTPUT:
>>      case OVS_ACTION_ATTR_USERSPACE:
>> +    case OVS_ACTION_ATTR_RECIRC:
>>          execute.actions = action;
>>          execute.actions_len = NLA_ALIGN(action->nla_len);
>>          execute.packet = packet;
>> @@ -1077,6 +1078,7 @@ dpif_execute_helper_cb(void *aux_, struct ofpbuf *packet,
>>          aux->error = aux->dpif->dpif_class->execute(aux->dpif, &execute);
>>          break;
>>
>> +    case OVS_ACTION_ATTR_HASH:
>>      case OVS_ACTION_ATTR_PUSH_VLAN:
>>      case OVS_ACTION_ATTR_POP_VLAN:
>>      case OVS_ACTION_ATTR_PUSH_MPLS:
>> @@ -1084,8 +1086,6 @@ dpif_execute_helper_cb(void *aux_, struct ofpbuf *packet,
>>      case OVS_ACTION_ATTR_SET:
>>      case OVS_ACTION_ATTR_SAMPLE:
>>      case OVS_ACTION_ATTR_UNSPEC:
>> -    case OVS_ACTION_ATTR_RECIRC:
>> -    case OVS_ACTION_ATTR_HASH:
>>      case __OVS_ACTION_ATTR_MAX:
>>          OVS_NOT_REACHED();
>>      }
>> diff --git a/lib/odp-execute.c b/lib/odp-execute.c
>> index 12ad679..cc18536 100644
>> --- a/lib/odp-execute.c
>> +++ b/lib/odp-execute.c
>> @@ -26,6 +26,7 @@
>>  #include "ofpbuf.h"
>>  #include "odp-util.h"
>>  #include "packets.h"
>> +#include "flow.h"
>>  #include "unaligned.h"
>>  #include "util.h"
>>
>> @@ -208,7 +209,6 @@ odp_execute_actions__(void *dp, struct ofpbuf *packet, bool steal,
>>          case OVS_ACTION_ATTR_OUTPUT:
>>          case OVS_ACTION_ATTR_USERSPACE:
>>          case OVS_ACTION_ATTR_RECIRC:
>> -        case OVS_ACTION_ATTR_HASH:
>>              if (dp_execute_action) {
>>                  /* Allow 'dp_execute_action' to steal the packet data if we do
>>                   * not need it any more. */
>> @@ -219,6 +219,27 @@ odp_execute_actions__(void *dp, struct ofpbuf *packet, bool steal,
>>              }
>>              break;
>>
>> +        case OVS_ACTION_ATTR_HASH: {
>> +            const struct ovs_action_hash *hash_act = nl_attr_get(a);
>> +
>> +            /* Calculate a hash value directly.  This might not match the
>> +             * value computed by the datapath, but it is much less expensive,
>> +             * and the current use case (bonding) does not require a strict
>> +             * match to work properly. */
>> +            if (hash_act->hash_alg == OVS_HASH_ALG_L4) {
>> +                struct flow flow;
>> +                uint32_t hash;
>> +
>> +                flow_extract(packet, md, &flow);
>> +                hash = flow_hash_5tuple(&flow, hash_act->hash_basis);
>> +                md->dp_hash = hash ? hash : 1;
>> +            } else {
>> +                /* Assert on unknown hash algorithm.  */
>> +                OVS_NOT_REACHED();
>> +            }
>> +            break;
>> +        }
>> +
>>          case OVS_ACTION_ATTR_PUSH_VLAN: {
>>              const struct ovs_action_push_vlan *vlan = nl_attr_get(a);
>>              eth_push_vlan(packet, htons(ETH_TYPE_VLAN), vlan->vlan_tci);
>> --
>> 1.9.1
>>
>> _______________________________________________
>> dev mailing list
>> dev at openvswitch.org
>> http://openvswitch.org/mailman/listinfo/dev



More information about the dev mailing list