[ovs-dev] [multipath 7/7] Implement a new Nicira extension action for multipath link selection.
Justin Pettit
jpettit at nicira.com
Fri Dec 17 22:28:17 UTC 2010
On Dec 16, 2010, at 2:42 PM, Ben Pfaff wrote:
> + /* L2 through L4, symmetric across src/dst. Specifically, each of the
> + * following fields, if present, is hashed (slashes separate symmetric
> + * pairs):
> + *
> + * - NXM_OF_ETH_DST / NXM_OF_ETH_SRC
> + * - NXM_OF_ETH_TYPE
> + * - NXM_OF_VLAN_TCI (ignoring VLAN_CFI)
We should probably not include the PCP bits, since they may not be symmetric.
> +/* multipath_check(). */
> +int
> +multipath_check(const struct nx_action_multipath *mp)
> +{
> ...
> + } else if (mp->algorithm != htons(NX_MP_ALG_MODULO_N)) {
> + VLOG_WARN_RL(&rl, "unsupported algorithm %"PRIu16,
> + ntohs(mp->algorithm));
What about the other algorithms that are supported?
> +void
> +multipath_execute(const struct nx_action_multipath *mp, struct flow *flow)
> +{
> + /* Calculate value to store. */
> + uint32_t hash = multipath_hash(flow, ntohs(mp->fields), ntohs(mp->basis));
> + uint16_t link = multipath_algorithm(hash, ntohs(mp->algorithm),
> + ntohs(mp->max_link) + 1,
> + ntohs(mp->arg));
I believe "arg" is 32 bits, so that should be ntohl().
> +static uint32_t
> +hash_symmetric_l4(const struct flow *flow, uint16_t basis)
> +{
> ...
> + for (i = 0; i < 6; i++) {
> + fields.eth_addr[i] = flow->dl_src[i] ^ flow->dl_dst[i];
> + }
Rather than "6", it seems like it would be more consistent to use ETH_ADDR_LEN.
> + fields.vlan_tci = flow->vlan_tci & ~htons(VLAN_CFI);
As previously mentioned, I'd exclude the PCP bits, too.
> +void
> +multipath_parse(struct nx_action_multipath *mp, const char *s_)
> +{
> ...
> + mp->max_link = htons(atoi(n_links) - 1);
> + mp->arg = htons(atoi(arg));
Once again, I believe "arg" is 32 bits, so that should be htonl().
> +void
> +multipath_format(const struct nx_action_multipath *mp, struct ds *s)
> +{
> ...
> + ds_put_format(s, "multipath(%s,%"PRIu16",%s,%d,%"PRIu16",",
> + fields, ntohs(mp->basis), algorithm, ntohs(mp->max_link) + 1,
> + ntohs(mp->arg));
Same here, but with ntohl().
> static void
> str_to_action(char *str, struct ofpbuf *b)
> {
> ...
> + } else if (act[actlen] == '(') {
> + /* The argument can be surrounded by balanced parentheses. The
> + * outemost set of parentheses is removed. */
"outemost" -> "outermost"
> +\fBhash_threshold\fR, \fBhrw\fR, and \fBiter_hash\fR. Only
> +the \BIiter_hash\fR algorithm uses \fIarg\fR.
I believe you mean "\fB" instead of "\BI".
--Justin
More information about the dev
mailing list