[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