[ovs-dev] [multipath 7/7] Implement a new Nicira extension action for multipath link selection.

Ben Pfaff blp at nicira.com
Fri Dec 17 22:38:57 UTC 2010


On Fri, Dec 17, 2010 at 02:28:17PM -0800, Justin Pettit wrote:
> 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.

Thanks for pointing this out.  I made that change.

> > +/* 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?

Oops, initially I didn't plan to implement those in the first cut, but
they managed to squeak through.  Thanks, fixed.

> > +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().

Oops, thanks.

> > +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.

Changed, thanks.

> > +    fields.vlan_tci = flow->vlan_tci & ~htons(VLAN_CFI);
> 
> As previously mentioned, I'd exclude the PCP bits, too.

Fixed, thanks.

> > +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().

Fixed, thanks.

> > +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().

Fixed, thanks.

> > 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"

Fixed, thanks.

> > +\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".

Oops, thank you.




More information about the dev mailing list