Looks good to me. Only a few minor comments, and questions. This
will really make it much easier to work on both the bridge and bonding
so I think it's a huge win.
Ethan
> + /* Tag set saved for next bond_run(). */
> + struct tag_set tags;
> +};
It wasn't entirely clear to me exactly what this tag set is for. I
assume its a tag_set of slaves which need to be revalidated? I think
it could benefit from a more descriptive name then tags if you have
one handy.
> +/* Frees 'bond'.@ */
> +void
> +bond_destroy(struct bond *bond)
> +{
Unnecessary @ in the comment.
> + HMAP_FOR_EACH_SAFE (slave, next_slave, hmap_node, &bond->slaves) {
> + hmap_remove(&bond->slaves, &slave->hmap_node);
> + /* Client owns 'slave->netdev'. */
> + free(slave->name);
> + free(slave);
> + }
I wonder if it would make sense for bond_destroy to return a tag union
of all the slaves which were removed. This would allow the caller to
revalidate the relevant flows.
> +/* Callback for lacp_run(). */
> +static void
> +bond_send_pdu_cb(void *slave_, const struct lacp_pdu *pdu)
> +{
> + struct bond_slave *slave = slave_;
> + uint8_t ea[ETH_ADDR_LEN];
> + int error;
> +
> + error = netdev_get_etheraddr(slave->netdev, ea);
> + if (!error) {
> + struct lacp_pdu *packet_pdu;
> + struct ofpbuf packet;
> +
> + ofpbuf_init(&packet, 0);
> + packet_pdu = eth_compose(&packet, eth_addr_lacp, ea, ETH_TYPE_LACP,
> + sizeof *packet_pdu);
Incorrect indentation here.
> + /* Enable slaves based on link status and LACP feedbkac. */
> + HMAP_FOR_EACH (slave, hmap_node, &bond->slaves) {
> + bond_link_status_update(slave, tags);
> + }
Typo in the comment.
> + if (!tag_set_is_empty(&bond->tags)) {
> + poll_immediate_wake();
> + }
After some thinking I realized that this is so bond_run can cause its
tags to be revalidated immediately. It looks slightly strange, so I
think it could benefit from a comment.
> +/* Returns true if 'bond' needs the client to send out packets to send out
> + * packets to assist with MAC learning on 'bond'. If this function returns
> + * true, then the client should iterate through its MAC learning table for
> the
> + * bridge on which 'bond' is located. For each MAC that has been learned on
> a
> + * port other than 'bond', it should call bond_send_learning_packet().
> + *
> + * This function will only return true if 'bond' is in SLB mode and LACP is
> not
> + * negotiated. Otherwise sending learning packets isn't necessary.
> + *
> + * Calling this function resets the state that it checks. */
Typo in the first sentence of this comment.
> +static struct bond_entry *
> +choose_entry_to_migrate(const struct bond_slave *from, uint64_t to_tx_bytes)
> +{
> + struct bond_entry *e;
> +
> + LIST_FOR_EACH (e, list_node, &from->entries) {
> + double old_ratio, new_ratio;
> + uint64_t delta = e->tx_bytes;
> +
> + if (delta == 0 || from->tx_bytes - delta == 0) {
> + /* Pointless move. */
> + continue;
> + }
I may have misinterpreted the code, but it seems to me that
from->tx_bytes should always be >= delta. Is this correct? It may be
worth asserting to avoid strange rebalancing decisions which are hard
to track down.
> +
> + if (to_tx_bytes == 0) {
> + /* Nothing on the new slave, move it. */
> + return e;
> + }
> +
> + old_ratio = (double)from->tx_bytes / to_tx_bytes;
> + new_ratio = (double)(from->tx_bytes - delta) /
> + (to_tx_bytes + delta);
> +
> + if (new_ratio == 0) {
> + /* Should already be covered but check to prevent division
> + * by zero. */
> + continue;
> + }
> +
> + if (new_ratio < 1) {
> + new_ratio = 1 / new_ratio;
> + }
Assuming my previous assumption is correct, that from->tx_bytes is
always greater or equal to delta. It's not clear to me how new_ration
could ever be > 1. Do we need the check? Perhaps an assertion would
be better?
> +static void
> +bond_unixctl_show(struct unixctl_conn *conn,
> + const char *args, void *aux OVS_UNUSED)
> +{
> + struct ds ds = DS_EMPTY_INITIALIZER;
> + const struct bond_slave *slave;
> + const struct bond *bond;
> +
> + bond = bond_find(args);
> + if (!bond) {
> + unixctl_command_reply(conn, 501, "no such bond");
> + return;
> + }
> +
> + ds_put_format(&ds, "bond_mode: %s\n",
> + bond_mode_to_string(bond->balance));
> +
> + if (bond->lacp) {
> + ds_put_format(&ds, "lacp: %s\n",
> + lacp_is_active(bond->lacp) ? "active" : "passive");
> + } else {
> + ds_put_cstr(&ds, "lacp: off\n");
> + }
> +
> + if (bond->balance != BM_AB) {
> + ds_put_format(&ds, "bond-hash-algorithm: %s\n",
> + bond_is_tcp_hash(bond) ? "balance-tcp" :
> "balance-slb");
> + }
> +
> +
Redundant newline.
> +
> +static void
> +enable_slave(struct unixctl_conn *conn, const char *args_, bool enable)
> +{
> + char *args = (char *) args_;
> + char *save_ptr = NULL;
> + char *bond_s, *slave_s;
> + struct bond *bond;
> + struct bond_slave *slave;
> +
> + bond_s = strtok_r(args, " ", &save_ptr);
> + slave_s = strtok_r(NULL, " ", &save_ptr);
> + if (!slave_s) {
> + unixctl_command_reply(conn, 501,
> + "usage: bond/enable/disable-slave BOND SLAVE");
I think this would be clearer if we simply used a ternary operator to
print the command the user actually typed. "usage: bond/%s-slave BOND
SLAVE", enable ? "enable" : "disable"
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
