On 09/18/14 at 10:55am, Simon Horman wrote: > +const struct nlattr *bucket_actions(const struct nlattr *attr) > +{ > + const struct nlattr *a; > + int rem; > + > + for (a = nla_data(attr), rem = nla_len(attr); rem > 0; > + a = nla_next(a, &rem)) { > + if (nla_type(a) == OVS_BUCKET_ATTR_ACTIONS) { > + return a; > + } > + } > + > + return NULL; > +}
This is identical to nla_find(). I realize this is not the only example but I think we should stop replicating existing Netlink functionality and add missing pieces to lib/nlattr.c. > +static u16 bucket_weight(const struct nlattr *attr) > +{ > + const struct nlattr *weight; > + > + /* validate_and_copy_bucket() ensures that the first > + * attribute is OVS_BUCKET_ATTR_WEIGHT */ > + weight = nla_data(attr); > + BUG_ON(nla_type(weight) != OVS_BUCKET_ATTR_WEIGHT); > + return nla_get_u16(weight); > +} > + > +static int select_group(struct datapath *dp, struct sk_buff *skb, > + const struct nlattr *attr) > +{ > + const struct nlattr *best_bucket = NULL; > + const struct nlattr *acts_list; > + const struct nlattr *bucket; > + struct sk_buff *sample_skb; > + u32 best_score = 0; > + u32 basis; > + u32 i = 0; > + int rem; > + > + basis = skb_get_hash(skb); > + > + /* Only possible type of attributes is OVS_SELECT_GROUP_ATTR_BUCKET */ > + for (bucket = nla_data(attr), rem = nla_len(attr); rem > 0; > + bucket = nla_next(bucket, &rem)) { > + uint16_t weight = bucket_weight(bucket); I think we should validate only once when we copy then assume it is correct. > + // XXX: This hashing seems expensive > + u32 score = (jhash_1word(i, basis) & 0xffff) * weight; Maybe just calculate a weighted distribution table pointing to the buckets which you index with 8 bits of the hash. > + if (score >= best_score) { > + best_bucket = bucket; > + best_score = score; > + } > + i++; > + } > + > + acts_list = bucket_actions(best_bucket); > + > + /* A select group action is always the final action so > + * there is no need to clone the skb in case of side effects. > + * Instead just take a reference to it which will be released > + * by do_execute_actions(). */ > + skb_get(skb); > + > + return do_execute_actions(dp, skb, nla_data(acts_list), > + nla_len(acts_list)); Do we need a recursion limit here? > +} _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev