On Tue, Mar 25, 2014 at 9:04 AM, Ben Pfaff <b...@nicira.com> wrote:
> On Mon, Mar 24, 2014 at 06:58:43PM -0700, Andy Zhou wrote:
>> Add basic recirculation infrastructure and user space
>> data path support for it. The following bond mega flow patch will
>> make use of this infrastructure.
>>
>> Signed-off-by: Andy Zhou <az...@nicira.com>
>
> I'm not sure the placement of the new enumeration values in enum
> ovs_key_attr is correct. In particular, I don't entirely understand why
> OVS_KEY_ATTR_IPV4_TUNNEL is in #ifdef __KERNEL__, but if that value is
> part of the ABI then the new values should go after it.
>
> I am not sure that putting pad bytes in the middle of ovs_action_recirc
> is a great idea. I might just make hash_alg a uint32.
>
The following incremental change should fix the issues mentioned above.
diff --git a/include/linux/openvswitch.h b/include/linux/openvswitch.h
index acc2352..d9282d6 100644
--- a/include/linux/openvswitch.h
+++ b/include/linux/openvswitch.h
@@ -307,12 +307,13 @@ enum ovs_key_attr {
OVS_KEY_ATTR_TUNNEL, /* Nested set of ovs_tunnel attributes */
OVS_KEY_ATTR_SCTP, /* struct ovs_key_sctp */
OVS_KEY_ATTR_TCP_FLAGS, /* be16 TCP flags. */
- OVS_KEY_ATTR_DP_HASH, /* u32 hash value */
- OVS_KEY_ATTR_RECIRC_ID, /* u32 recirc id */
#ifdef __KERNEL__
OVS_KEY_ATTR_IPV4_TUNNEL, /* struct ovs_key_ipv4_tunnel */
#endif
+ OVS_KEY_ATTR_DP_HASH = 20, /* u32 hash value */
+ OVS_KEY_ATTR_RECIRC_ID, /* u32 recirc id */
+
OVS_KEY_ATTR_MPLS = 62, /* array of struct ovs_key_mpls.
* The implementation may restrict
* the accepted length of the array. */
@@ -551,8 +552,7 @@ enum ovs_recirc_hash_alg {
* @hash_bias: bias used for computing hash. used to compute hash prior to rec
*/
struct ovs_action_recirc {
- uint8_t hash_alg; /* One of ovs_dp_hash_alg. */
- uint8_t pad[3]; /* Always zero. */
+ uint32_t hash_alg; /* One of ovs_dp_hash_alg. */
uint32_t hash_bias;
uint32_t recirc_id; /* Recirculation label. */
};
> In dpif-netdev.c, does anything protect against infinite recursion?
>
This is addressed in the next patch.
> The following change appears to be a mistake:
>> diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
>> index 7b81516..277c1d7 100644
>> --- a/ofproto/ofproto.c
>> +++ b/ofproto/ofproto.c
>> @@ -4482,21 +4482,6 @@ handle_flow_mod__(struct ofproto *ofproto, struct
>> ofconn *ofconn,
>> {
>> enum ofperr error;
>>
>> - /* Only internal flow mods can set recircualtion fields. */
>> - if (!(fm->flags & OFPUTIL_FF_INTERNAL)) {
>> - char *err_field = NULL;
>> -
>> - err_field = fm->match.flow.recirc_id ? "recirc_id" : err_field;
>> - err_field = fm->match.flow.dp_hash ? "dp_hash" : err_field;
>> -
>> - if (err_field) {
>> - VLOG_WARN_RL(&rl, "%s: (flow_mod) only internal flows can set
>> %s",
>> - ofproto->name, err_field);
>> - error = OFPERR_OFPFMFC_EPERM;
>> - return error;
>> - }
>> - }
>> -
>> ovs_mutex_lock(&ofproto_mutex);
>> if (ofproto->n_pending < 50) {
>> switch (fm->command) {
Fixed.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
