Thanks for the review. Pushed to master and branch-2.3
On Tue, Jun 3, 2014 at 11:30 PM, YAMAMOTO Takashi
<yamam...@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 <yamam...@valinux.co.jp>
>> Signed-off-by: Andy Zhou <az...@nicira.com>
>
> Acked-by: YAMAMOTO Takashi <yamam...@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@openvswitch.org
>> http://openvswitch.org/mailman/listinfo/dev
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
