On Fri, Jul 15, 2011 at 01:50:04PM -0700, Ethan Jackson wrote:
> This patch creates a new action called "bundle". Bundles are a way
> to implement a simple form of multipath in OpenFlow by grouping
> several ports in a single output-like action.
The "bundle" term is already used, with a different meaning, in the
ofproto-private.h interface. We should rename one of them or the
other; I'm open to either. (I'd be inclined toward renaming the
action, since "bundle" isn't really a verb, and we should at least try
to give our actions names that indicate what they do, not that we've
been consistent about that in the past.)
> +/* Action structure for NXAST_BUNDLE.
> + *
> + * NXAST_BUNDLE chooses a slave from a supplied list of options, and outputs
> to
> + * its selection.
> + *
> + * The list of possible slaves is appended to the end of the nx_action_bundle
> + * structure.
"is appended to the end" makes it sound to me like the switch modifies
the actions. Maybe just "follows"?
> + * Switches infer from the 'slave_type' parameter the size of each slave.
> All
> + * implementations must support the NXM_OF_IN_PORT 'slave_type' which
> indicates
> + * that the slaves are OpenFlow port numbers with NXM_LENGTH(NXM_OF_IN_PORT)
> ==
> + * 16 bit width. Switches should reject actions which indicate unknown or
> + * unsupported slave types.
The NXM_* constants are 32 bits wide, but 'slave_type' is an
ovs_be16. Doesn't it need to be an ovs_be32?
The comment implies NXM_LENGTH(NXM_OF_IN_PORT) that has value 16
(bits) but in fact it equals 2 (bytes). Probably best not to confuse
readers that way.
> + * The 'zero' parameter at the end of the action structure is reserved for
> + * future use. Switches are required to reject actions which have nonzero
> + * bytes in the 'zero' field. */
> +
> +struct nx_action_bundle {
Would you mind dropping the blank line between the comment and the
"struct nx_action_bundle" line.
> +enum nx_bd_algorithm {
> + /* Chooses the first live slave listed in the bundle.
> + *
> + * O(n_slaves) performance. */
> + NX_BD_ALG_ACTIVE_BACKUP,
> +
> + /* for i in [0,n_slaves):
> + * weights[i] = hash(flow, i)
> + * slave = { slaves[i] such that weights[i] >= weights[j] for all j != i
> }
> + *
> + * Redistributes 1/n_slaves of traffic when a slave's liveness changes.
> + * O(n_slaves) performance.
> + *
> + * Uses the 'fields' and 'basis' parameters. */
> + NX_BD_ALG_HRW = NX_MP_ALG_HRW /* Highest Random Weight. */
I know that I suggested giving NX_BD_ALG_HRW and NX_MP_ALG_HRW the
same values, but actually looking at it, I don't think it's a good
idea anymore. It's going to be hard to maintain as we add more
algorithms. Let's drop that part.
> +/* Executes 'nab' on 'flow'. Uses 'slave_enabled' to determine if the slave
> + * designated by 'ofp_port' is up. Returns the choosen slave, or OFPP_NONE
> if
> + * none of the slaves are acceptable. */
s/choosen/chosen/
> +uint16_t
> +bundle_execute(const struct nx_action_bundle *nab, const struct flow *flow,
> + bool slave_enabled(uint16_t ofp_port, void *aux), void *aux)
Automatically adding a * in function prototypes is a weird and not
widely understood corner case in the C standard, so would you mind
writing (*) around slave_enabled to make it obvious to the reader,
e.g.:
bool (*slave_enabled)(uint16_t ofp_port, void *aux)
> + if (slaves_size < n_slaves * sizeof(ovs_be16)) {
> + VLOG_WARN_RL(&rl, "Nicira action %"PRIu16" only has %zu bytes"
> + " allocated for slaves. %zu bytes are required for"
> + " %"PRIu16" slaves.\n", subtype, slaves_size,
> + n_slaves * sizeof(ovs_be16), n_slaves);
The style I've always used in long strings is to put the spaces at
ends of lines, e.g.:
VLOG_WARN_RL(&rl, "Nicira action %"PRIu16" only has %zu bytes "
"allocated for slaves. %zu bytes are required for "
"%"PRIu16" slaves.\n", subtype, slaves_size,
n_slaves * sizeof(ovs_be16), n_slaves);
No, it doesn't really matter, but consistency is nice when there's
little cost.
Also, please omit the \n at the end of the log message.
> + if (strcasecmp(slave_delim, "slaves")) {
> + ovs_fatal(0, "%s: missing slave delimiter, expected `slaves' got
> `%s'",
> + s, slave_delim);
> + }
> +
> + ofpbuf_clear(b);
> + ofpbuf_prealloc_headroom(b, sizeof *nab);
This strategy looks like a mistake. It will, I believe, cause any
action already added to 'b' to be erased, but I don't know of a reason
that "bundle" couldn't be preceded by some other action. It would be
better to just "put" space for the header before we start adding
slaves (and then keep track of its position as an offset from
b->base).
> + /* Slaves array must be multiple of 8 bytes long. */
> + ofpbuf_put_zeros(b, 8 - b->size % 8);
That will put eight extra zero bytes if b->size is already a multiple
of 8. That will work but it's wasteful. I'd either test for b->size
% 8 == 0 in an 'if' statement or use ROUND_UP(b->size, 8) - b->size.
> +/* Appends a human-readbale representation of 'nab' to 's'. */
s/readbale/readable/
> +/* Returns a pointer to the array of slaves contained in 'nab' */
> +static inline const ovs_be16 *
> +bundle_slaves(const struct nx_action_bundle *nab)
> +{
> + return (ovs_be16 *)((char *)nab + sizeof *nab);
> +}
I'm pretty sure that this will cause a warning on RISC architectures
like SPARC because it converts from a pointer with no alignment
restrictions (char *) to a pointer with 16-bit alignment (ovs_be16
*). To supress it, add yet another cast through a generic pointer:
return (ovs_be16 *)(void *)((char *)nab + sizeof *nab);
or just:
return (ovs_be16 *)(nab + 1);
Every existing use of bundle_slaves() is like this:
ntohs(bundle_slaves(nab)[i])
Maybe a more convenient helper would be:
static inline uint16_t
bundle_get_slave(const struct nx_action_bundle *nab, int index)
{
return ntohs(bundle_slaves(nab)[index]);
}
> +AT_SETUP([bundle action bad slave delimeter])
s/delimeter/delimiter/
> +static struct nx_action_bundle *
> +parse_bundle_actions(char *actions)
> +{
> + struct nx_action_bundle *nab;
> + struct ofpbuf b;
> +
> + ofpbuf_init(&b, 0);
> + bundle_parse(&b, actions);
> + nab = ofpbuf_steal_data(&b);
> + ofpbuf_uninit(&b);
> +
> + if (ntohs(nab->n_slaves) > BD_MAX_SLAVES) {
> + ovs_fatal(0, "At most %u slaves are supported", BD_MAX_SLAVES);
> + }
This implies that bundle_parse() will parse more than BD_MAX_SLAVES
slaves. Shouldn't that be checked in bundle_parse() itself?
> + return nab;
> +}
> +
> + /* Cycles through each possible liveness permutation for the given
> + * n_slaves. The initial state is equivalent to all slaves down, so we
> + * skip it by starting at i = 1. We do one extra iteration to cover
> + * transitioning from the final state back to the initial state. */
> + old_n_enabled = 0;
> + n_permute = 1 << sg.n_slaves;
> + for (i = 1; i <= n_permute + 1; i++) {
> + struct slave *slave;
> + size_t j, n_enabled, changed;
> + double disruption, perfect;
> + uint8_t mask;
> +
> + mask = i % n_permute;
> +
> + /* Gray coding ensures that in each iteration exactly one slave
> + * changes its liveness. This makes the expected disruption a bit
> + * easier to calculate, and is likely similar to how failures will be
> + * experienced in the wild. */
> + mask = mask ^ (mask >> 1);
This is a clever idea. Great, thank you.
> + /* Initialize slaves. */
> + n_enabled = 0;
> + for (j = 0; j < sg.n_slaves; j++) {
> + slave = &sg.slaves[j];
> + slave->flow_count = 0;
> + slave->enabled = (1 << j) & mask;
Would you mind making this:
slave->enabled = ((1 << j) & mask) != 0;
Some compilers still don't have a native 'bool' type, so on those
platforms it's a typedef for 'char'. Without != 0, slaves above 7
would never be enabled on those platforms.
> --- a/utilities/ovs-ofctl.8.in
> +++ b/utilities/ovs-ofctl.8.in
> @@ -729,6 +729,17 @@ Otherwise, the register will be populated with \fIid\fR
> itself.
> Refer to \fBnicira\-ext.h\fR for more details.
> .RE
> .
> +.IP "\fBbundle(\fIfields\fB, \fIbasis\fB, \fIalgorithm\fB, \fIslave_type\fB,
> slaves:[\fIs1\fB, \fIs2\fB, ...])\fR"
> +Hashes \fIfields\fR using \fIbasis\fR as a universal hash parameter, then
> +applies the bundle link selection \fIalgorithm\fR to choose one of the listed
> +slaves represented as \fIslave_type\fR. Outputs to the selected slave.
> +.IP
> +Currently, \fIfields\fR must be either \fBeth_src\fR or \fBsymmetric_l4\fR
> and
> +\fIalgorithm\fR must be one of \fBhrw\fR and \fBactive_backup\fR.
> +.IP
> +Refer to \fBnicira\-ext.h\fR for more details.
> +.RE
> +.
Currently slave_type only has one valid value, so we should say what
it is (and what each of the \fIs1\fR through \fIsN\fR needs to be).
It would also be nice to give an example.
This adds a new .RE that wasn't there before. That will probably
screw up the indentation for everything below it.
Thanks,
Ben.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
