Good points, I will address them in V2
On Tue, Apr 15, 2014 at 7:13 AM, Pravin Shelar <pshe...@nicira.com> wrote:
> On Sat, Apr 12, 2014 at 3:30 AM, Andy Zhou <az...@nicira.com> wrote:
>> Currently recirculation action can optionally compute hash. This patch
>> adds a hash action that is independent of the recirc action, which
>> no longer computes hash. For megaflow bond with recirc, the output
>> to a bond port action will look like:
>>
>> hash(hash_l4(0)), recric(<recirc_id>)
>>
>> Obviously, when a recirculation application that does not depend on
>> hash value can just use the recirc action alone.
>>
>> Signed-off-by: Andy Zhou <az...@nicira.com>
>>
>> ---
>> V1->v2: rebased against version ca3d034b3909
>> ---
>> include/linux/openvswitch.h | 21 +++++++++------------
>> lib/dpif-netdev.c | 30 +++++++++++++++++-------------
>> lib/dpif.c | 1 +
>> lib/odp-execute.c | 1 +
>> lib/odp-util.c | 26 +++++++++++++++++---------
>> ofproto/ofproto-dpif-xlate.c | 19 ++++++++++++-------
>> 6 files changed, 57 insertions(+), 41 deletions(-)
>>
>> diff --git a/include/linux/openvswitch.h b/include/linux/openvswitch.h
>> index a88f6f1..11a4969 100644
>> --- a/include/linux/openvswitch.h
>> +++ b/include/linux/openvswitch.h
>> @@ -537,26 +537,22 @@ struct ovs_action_push_vlan {
>>
>> /* Data path hash algorithm for computing Datapath hash.
>> *
>> - * The Algorithm type only specifies the fields in a flow
>> + * The algorithm type only specifies the fields in a flow
>> * will be used as part of the hash. Each datapath is free
>> * to use its own hash algorithm. The hash value will be
>> * opaque to the user space daemon.
>> */
>> -enum ovs_recirc_hash_alg {
>> - OVS_RECIRC_HASH_ALG_NONE,
>> - OVS_RECIRC_HASH_ALG_L4,
>> +enum ovs_hash_alg {
>> + OVS_HASH_ALG_L4,
>> };
>> /*
>> - * struct ovs_action_recirc - %OVS_ACTION_ATTR_RECIRC action argument.
>> - * @recirc_id: The Recirculation label, Zero is invalid.
>> + * struct ovs_action_hash - %OVS_ACTION_ATTR_HASH action argument.
>> * @hash_alg: Algorithm used to compute hash prior to recirculation.
>> - * @hash_bias: bias used for computing hash. used to compute hash prior to
>> - * recirculation.
>> + * @hash_bias: bias used for computing hash.
>> */
>> -struct ovs_action_recirc {
>> - uint32_t hash_alg; /* One of ovs_dp_hash_alg. */
>> +struct ovs_action_hash {
>> + uint32_t hash_alg; /* One of ovs_hash_alg. */
>> uint32_t hash_bias;
>> - uint32_t recirc_id; /* Recirculation label. */
>> };
>>
>> /**
>> @@ -599,7 +595,8 @@ enum ovs_action_attr {
>> OVS_ACTION_ATTR_SAMPLE, /* Nested OVS_SAMPLE_ATTR_*. */
>> OVS_ACTION_ATTR_PUSH_MPLS, /* struct ovs_action_push_mpls. */
>> OVS_ACTION_ATTR_POP_MPLS, /* __be16 ethertype. */
>> - OVS_ACTION_ATTR_RECIRC, /* struct ovs_action_recirc. */
>> + OVS_ACTION_ATTR_RECIRC, /* u32 recirc_id. */
>> + OVS_ACTION_ATTR_HASH, /* struct ovs_action_hash. */
>> __OVS_ACTION_ATTR_MAX
>> };
>>
>> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
>> index 9ad9386..75cfd9e 100644
>> --- a/lib/dpif-netdev.c
>> +++ b/lib/dpif-netdev.c
>> @@ -2138,25 +2138,29 @@ dp_execute_cb(void *aux_, struct ofpbuf *packet,
>> break;
>> }
>>
>> + case OVS_ACTION_ATTR_HASH: {
>> + const struct ovs_action_hash *hash_act;
>> + hash_act = nl_attr_get(a);
>> + if (hash_act->hash_alg == OVS_HASH_ALG_L4) {
>> + uint32_t l4_hash;
>> +
>> + l4_hash = flow_hash_symmetric_l4(aux->key, hash_act->hash_bias);
>> + if (!l4_hash) {
>> + l4_hash = 1; /* 0 is not valid */
>> + }
>> +
>> + md->dp_hash = l4_hash;
>> + }
>> + break;
>> + }
>> +
> We should check hash_alg at flow-install for support hashing
> algorithms, so that we do not silently ignore this action and can skip
> this check on every packet execute.
>
>> case OVS_ACTION_ATTR_RECIRC:
>> if (*depth < MAX_RECIRC_DEPTH) {
>> struct pkt_metadata recirc_md = *md;
>> struct ofpbuf *recirc_packet;
>> - const struct ovs_action_recirc *act;
>>
>> recirc_packet = may_steal ? packet : ofpbuf_clone(packet);
>> -
>> - act = nl_attr_get(a);
>> - recirc_md.recirc_id = act->recirc_id;
>> - recirc_md.dp_hash = 0;
>> -
>> - if (act->hash_alg == OVS_RECIRC_HASH_ALG_L4) {
>> - recirc_md.dp_hash = flow_hash_symmetric_l4(aux->key,
>> - act->hash_bias);
>> - if (!recirc_md.dp_hash) {
>> - recirc_md.dp_hash = 1; /* 0 is not valid */
>> - }
>> - }
>> + recirc_md.recirc_id = nl_attr_get_u32(a);
>>
>> (*depth)++;
>> dp_netdev_input(aux->dp, recirc_packet, &recirc_md);
>> diff --git a/lib/dpif.c b/lib/dpif.c
>> index a8bd674..41b8eb7 100644
>> --- a/lib/dpif.c
>> +++ b/lib/dpif.c
>> @@ -1134,6 +1134,7 @@ dpif_execute_helper_cb(void *aux_, struct ofpbuf
>> *packet,
>> 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 37e44e3..12ad679 100644
>> --- a/lib/odp-execute.c
>> +++ b/lib/odp-execute.c
>> @@ -208,6 +208,7 @@ 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. */
>> diff --git a/lib/odp-util.c b/lib/odp-util.c
>> index b58f1c0..d5e37d8 100644
>> --- a/lib/odp-util.c
>> +++ b/lib/odp-util.c
>> @@ -79,7 +79,8 @@ odp_action_len(uint16_t type)
>> case OVS_ACTION_ATTR_POP_VLAN: return 0;
>> case OVS_ACTION_ATTR_PUSH_MPLS: return sizeof(struct
>> ovs_action_push_mpls);
>> case OVS_ACTION_ATTR_POP_MPLS: return sizeof(ovs_be16);
>> - case OVS_ACTION_ATTR_RECIRC: return sizeof(struct ovs_action_recirc);
>> + case OVS_ACTION_ATTR_RECIRC: return sizeof(uint32_t);
>> + case OVS_ACTION_ATTR_HASH: return sizeof(struct ovs_action_hash);
>> case OVS_ACTION_ATTR_SET: return -2;
>> case OVS_ACTION_ATTR_SAMPLE: return -2;
>>
>> @@ -387,16 +388,20 @@ format_mpls(struct ds *ds, const struct ovs_key_mpls
>> *mpls_key,
>> }
>>
>> static void
>> -format_odp_recirc_action(struct ds *ds,
>> - const struct ovs_action_recirc *act)
>> +format_odp_recirc_action(struct ds *ds, uint32_t recirc_id)
>> {
>> - ds_put_format(ds, "recirc(");
>> + ds_put_format(ds, "recirc(%"PRIu32")", recirc_id);
>> +}
>>
>> - if (act->hash_alg == OVS_RECIRC_HASH_ALG_L4) {
>> - ds_put_format(ds, "hash_l4(%"PRIu32"), ", act->hash_bias);
>> - }
>> +static void
>> +format_odp_hash_action(struct ds *ds, const struct ovs_action_hash
>> *hash_act)
>> +{
>> + ds_put_format(ds, "hash(");
>>
>> - ds_put_format(ds, "%"PRIu32")", act->recirc_id);
>> + if (hash_act->hash_alg == OVS_HASH_ALG_L4) {
>> + ds_put_format(ds, "hash_l4(%"PRIu32")", hash_act->hash_bias);
>> + }
>> + ds_put_format(ds, ")");
>> }
> If hash action has unsupported algorithm then we should print it or
> atleast print error.
>
>>
>> static void
>> @@ -422,7 +427,10 @@ format_odp_action(struct ds *ds, const struct nlattr *a)
>> format_odp_userspace_action(ds, a);
>> break;
>> case OVS_ACTION_ATTR_RECIRC:
>> - format_odp_recirc_action(ds, nl_attr_get(a));
>> + format_odp_recirc_action(ds, nl_attr_get_u32(a));
>> + break;
>> + case OVS_ACTION_ATTR_HASH:
>> + format_odp_hash_action(ds, nl_attr_get(a));
>> break;
>> case OVS_ACTION_ATTR_SET:
>> ds_put_cstr(ds, "set(");
>> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
>> index 91ce7b7..04998b9 100644
>> --- a/ofproto/ofproto-dpif-xlate.c
>> +++ b/ofproto/ofproto-dpif-xlate.c
>> @@ -1152,7 +1152,7 @@ output_normal(struct xlate_ctx *ctx, const struct
>> xbundle *out_xbundle,
>>
>> if (ctx->xout->use_recirc) {
>> /* Only TCP mode uses recirculation. */
>> - xr->hash_alg = OVS_RECIRC_HASH_ALG_L4;
>> + xr->hash_alg = OVS_HASH_ALG_L4;
>> bond_update_post_recirc_rules(out_xbundle->bond, false);
>>
>> /* Recirculation does not require unmasking hash fields. */
>> @@ -1843,14 +1843,19 @@ compose_output_action__(struct xlate_ctx *ctx,
>> ofp_port_t ofp_port,
>> &ctx->xout->wc);
>>
>> if (ctx->xout->use_recirc) {
>> - struct ovs_action_recirc *act_recirc;
>> + struct ovs_action_hash *act_hash;
>> struct xlate_recirc *xr = &ctx->xout->recirc;
>>
>> - act_recirc = nl_msg_put_unspec_uninit(&ctx->xout->odp_actions,
>> - OVS_ACTION_ATTR_RECIRC, sizeof *act_recirc);
>> - act_recirc->recirc_id = xr->recirc_id;
>> - act_recirc->hash_alg = xr->hash_alg;
>> - act_recirc->hash_bias = xr->hash_bias;
>> + /* Hash action. */
>> + act_hash = nl_msg_put_unspec_uninit(&ctx->xout->odp_actions,
>> + OVS_ACTION_ATTR_HASH,
>> + sizeof *act_hash);
>> + act_hash->hash_alg = xr->hash_alg;
>> + act_hash->hash_bias = xr->hash_bias;
>> +
>> + /* Recirc action. */
>> + nl_msg_put_u32(&ctx->xout->odp_actions, OVS_ACTION_ATTR_RECIRC,
>> + xr->recirc_id);
>> } else {
>> nl_msg_put_odp_port(&ctx->xout->odp_actions,
>> OVS_ACTION_ATTR_OUTPUT,
>> out_port);
>> --
>> 1.7.9.5
>>
>> _______________________________________________
>> dev mailing list
>> dev@openvswitch.org
>> http://openvswitch.org/mailman/listinfo/dev
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
