[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