On 17.12.2019 20:23, Ben Pfaff wrote:
> Thanks for the patch!
>
> "sparse" reports some type errors:
>
> ../ofproto/bond.c:357:30: error: incorrect type in assignment (different
> base types)
> ../ofproto/bond.c:357:30: expected unsigned int
> ../ofproto/bond.c:357:30: got restricted ofp_port_t [usertype] ofp_port
> ../ofproto/ofproto-dpif.c:3454:56: error: incorrect type in argument 2
> (different base types)
> ../ofproto/ofproto-dpif.c:3454:56: expected restricted ofp_port_t
> [usertype]
> ../ofproto/ofproto-dpif.c:3454:56: got unsigned int [usertype]
> ../ofproto/ofproto-dpif.c:3453:31: error: incorrect type in assignment
> (different base types)
> ../ofproto/ofproto-dpif.c:3453:31: expected unsigned int [usertype]
> ../ofproto/ofproto-dpif.c:3453:31: got restricted odp_port_t
> ../ofproto/ofproto-dpif-xlate.c:4208:39: error: incorrect type in
> argument 3 (different base types)
> ../ofproto/ofproto-dpif-xlate.c:4208:39: expected restricted
> odp_port_t [usertype] value
> ../ofproto/ofproto-dpif-xlate.c:4208:39: got unsigned int const
> [usertype] recirc_id
>
> I took a look. The underlying issue is that code here mixes integers,
> ofp_port_t, and odp_port_t. OVS uses "sparse" annotations to keep these
> from being confused, since they are different in important ways. I
> spent some time working through the types here and appended a patch that
> fixes them up. I also made a bunch of style updates throughout the
> code, which might obscure the relevant changes a bit; my apologies.
>
> It seems odd that dpif_bond_*() succeed if the dpif doesn't support
> them. Shouldn't they return an error by default, instead of 0?
>
> Is there a reason to allow users to turn this off? What is the downside
> of enabling it? Why is it disabled by default? In general, OVS should
> optimize itself to the extent it can rather than relying on a
> knowledgeable user to tweak it. If it's necessary to make it
> configurable, then the documentation (in vswitch.xml) should explain why
> one would want to turn it on or off and what the default is.
Hi Ben,
I just wanted to add a few comments here. There are few possibly unwanted
things that appears while enabling the feature:
1. Bonding rebalancing triggers reload of PMD threads, that might affect
performance of highly loaded threads.
2. While datapath supports lb_output action, netdev-offload provider might
not support it. This could be an issue.
So, I think it's sane to have a configuration knob. Not sure about disabling
by default but it might make a bit of sense keeping this feature as experimental
for some time. And yes, I agree that documentation should be updated.
>
> This introduces a new capabilities flag, which should be documented with
> the other capabilities in vswitch.xml.
>
> I'm appending a suggested diff to fold into your patch.
>
> I did not do a full technical review for correctness. I'll do that on
> the next revision.
>
> Thanks,
>
> Ben.
>
> -8<--------------------------cut here-------------------------->8--
>
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index 9a2befdcbc02..fa9ff73a4101 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -604,7 +604,7 @@ struct tx_port {
>
> /* Contained by struct tx_bond 'slave_buckets' */
> struct slave_entry {
> - uint32_t slave_id;
> + odp_port_t slave_id;
> atomic_ullong n_packets;
> atomic_ullong n_bytes;
> };
> @@ -1427,12 +1427,10 @@ dpif_netdev_dp_bond_show(struct unixctl_conn *conn,
> int argc,
> const char *argv[], void *aux OVS_UNUSED)
> {
> struct ds reply = DS_EMPTY_INITIALIZER;
> - struct dp_netdev *dp = NULL;
> - uint32_t bucket;
> - struct tx_bond *dp_bond_entry = NULL;
>
> ovs_mutex_lock(&dp_netdev_mutex);
>
> + struct dp_netdev *dp = NULL;
> if (argc == 2) {
> dp = shash_find_data(&dp_netdevs, argv[1]);
> } else if (shash_count(&dp_netdevs) == 1) {
> @@ -1446,10 +1444,12 @@ dpif_netdev_dp_bond_show(struct unixctl_conn *conn,
> int argc,
> return;
> }
> ds_put_cstr(&reply, "\nBonds:\n");
> +
> + struct tx_bond *dp_bond_entry;
> HMAP_FOR_EACH (dp_bond_entry, node, &dp->bonds) {
> ds_put_format(&reply, "\tbond-id %u :\n",
> dp_bond_entry->bond_id);
> - for (bucket = 0; bucket < BOND_BUCKETS; bucket++) {
> + for (int bucket = 0; bucket < BOND_BUCKETS; bucket++) {
> ds_put_format(&reply, "\t\tbucket %u - slave %u \n",
> bucket,
> dp_bond_entry->slave_buckets[bucket].slave_id);
> @@ -1735,7 +1735,6 @@ dp_netdev_free(struct dp_netdev *dp)
> OVS_REQUIRES(dp_netdev_mutex)
> {
> struct dp_netdev_port *port, *next;
> - struct tx_bond *bond, *next_bond;
>
> shash_find_and_delete(&dp_netdevs, dp->name);
>
> @@ -1746,6 +1745,7 @@ dp_netdev_free(struct dp_netdev *dp)
> ovs_mutex_unlock(&dp->port_mutex);
>
> ovs_mutex_lock(&dp->bond_mutex);
> + struct tx_bond *bond, *next_bond;
> HMAP_FOR_EACH_SAFE (bond, next_bond, node, &dp->bonds) {
> hmap_remove(&dp->bonds, &bond->node);
> free(bond);
> @@ -4933,11 +4933,10 @@ pmd_remove_stale_bonds(struct dp_netdev *dp,
> OVS_EXCLUDED(pmd->bond_mutex)
> OVS_EXCLUDED(dp->bond_mutex)
> {
> - struct tx_bond *tx, *tx_next;
> -
> ovs_mutex_lock(&dp->bond_mutex);
> ovs_mutex_lock(&pmd->bond_mutex);
>
> + struct tx_bond *tx, *tx_next;
> HMAP_FOR_EACH_SAFE (tx, tx_next, node, &pmd->tx_bonds) {
> if (!tx_bond_lookup(&dp->bonds, tx->bond_id)) {
> dp_netdev_del_bond_tx_from_pmd(pmd, tx);
> @@ -4958,7 +4957,6 @@ reconfigure_datapath(struct dp_netdev *dp)
> struct hmapx busy_threads = HMAPX_INITIALIZER(&busy_threads);
> struct dp_netdev_pmd_thread *pmd;
> struct dp_netdev_port *port;
> - struct tx_bond *bond;
> int wanted_txqs;
>
> dp->last_reconfigure_seq = seq_read(dp->reconfigure_seq);
> @@ -5119,6 +5117,7 @@ reconfigure_datapath(struct dp_netdev *dp)
> CMAP_FOR_EACH (pmd, node, &dp->poll_threads) {
> ovs_mutex_lock(&pmd->bond_mutex);
> if (hmap_count(&pmd->poll_list) || pmd->core_id == NON_PMD_CORE_ID) {
> + struct tx_bond *bond;
> HMAP_FOR_EACH (bond, node, &dp->bonds) {
> dp_netdev_add_bond_tx_to_pmd(pmd, bond);
> }
> @@ -5577,13 +5576,11 @@ static void
> pmd_load_cached_bonds(struct dp_netdev_pmd_thread *pmd)
> OVS_REQUIRES(pmd->bond_mutex)
> {
> - struct tx_bond *tx_bond, *tx_bond_cached;
> -
> pmd_free_cached_bonds(pmd);
> hmap_shrink(&pmd->bond_cache);
>
> + struct tx_bond *tx_bond, *tx_bond_cached;
> HMAP_FOR_EACH (tx_bond, node, &pmd->tx_bonds) {
> - uint32_t bucket = 0;
> /* Check if bond already exist on pmd. */
> tx_bond_cached = tx_bond_lookup(&pmd->bond_cache, tx_bond->bond_id);
>
> @@ -5594,12 +5591,12 @@ pmd_load_cached_bonds(struct dp_netdev_pmd_thread
> *pmd)
> hash_bond_id(tx_bond_cached->bond_id));
> } else {
> /* Update the slave-map. */
> - for (bucket = 0; bucket <= BOND_MASK; bucket++) {
> + for (int bucket = 0; bucket <= BOND_MASK; bucket++) {
> tx_bond_cached->slave_buckets[bucket].slave_id =
> tx_bond->slave_buckets[bucket].slave_id;
> }
> }
> - VLOG_DBG("Caching bond-id %d pmd %d\n",
> + VLOG_DBG("Caching bond-id %d pmd %d",
> tx_bond_cached->bond_id, pmd->core_id);
> }
> }
> @@ -6432,14 +6429,11 @@ dp_netdev_add_bond_tx_to_pmd(struct
> dp_netdev_pmd_thread *pmd,
> struct tx_bond *bond)
> OVS_REQUIRES(pmd->bond_mutex)
> {
> - struct tx_bond *tx;
> - uint32_t i;
> bool reload = false;
> -
> - tx = tx_bond_lookup(&pmd->tx_bonds, bond->bond_id);
> + struct tx_bond *tx = tx_bond_lookup(&pmd->tx_bonds, bond->bond_id);
> if (tx) {
> /* Check if mapping is changed. */
> - for (i = 0; i <= BOND_MASK; i++) {
> + for (int i = 0; i <= BOND_MASK; i++) {
> if (bond->slave_buckets[i].slave_id !=
> tx->slave_buckets[i].slave_id) {
> /* Mapping is modified. Reload pmd bond cache again. */
> @@ -6453,7 +6447,7 @@ dp_netdev_add_bond_tx_to_pmd(struct
> dp_netdev_pmd_thread *pmd,
> hmap_insert(&pmd->tx_bonds, &tx->node, hash_bond_id(bond->bond_id));
> reload = true;
> }
> - if (reload == true) {
> + if (reload) {
> pmd->need_reload = true;
> }
> }
> @@ -7296,49 +7290,89 @@ dp_execute_userspace_action(struct
> dp_netdev_pmd_thread *pmd,
> }
> }
>
> -static int
> +static bool
> dp_execute_output_action(struct dp_netdev_pmd_thread *pmd,
> struct dp_packet_batch *packets_,
> - bool should_steal,
> - odp_port_t port_no)
> + bool should_steal, odp_port_t port_no)
> {
> - struct tx_port *p;
> - p = pmd_send_port_cache_lookup(pmd, port_no);
> - if (OVS_LIKELY(p)) {
> - struct dp_packet *packet;
> - struct dp_packet_batch out;
> - if (!should_steal) {
> - dp_packet_batch_clone(&out, packets_);
> - dp_packet_batch_reset_cutlen(packets_);
> - packets_ = &out;
> - }
> - dp_packet_batch_apply_cutlen(packets_);
> + struct tx_port *p = pmd_send_port_cache_lookup(pmd, port_no);
> + if (!OVS_LIKELY(p)) {
> + return false;
> + }
> +
> + struct dp_packet_batch out;
> + if (!should_steal) {
> + dp_packet_batch_clone(&out, packets_);
> + dp_packet_batch_reset_cutlen(packets_);
> + packets_ = &out;
> + }
> + dp_packet_batch_apply_cutlen(packets_);
> #ifdef DPDK_NETDEV
> - if (OVS_UNLIKELY(!dp_packet_batch_is_empty(&p->output_pkts)
> - && packets_->packets[0]->source
> - != p->output_pkts.packets[0]->source)) {
> - /* netdev-dpdk assumes that all packets in a single
> - * output batch has the same source. Flush here to
> - * avoid memory access issues. */
> - dp_netdev_pmd_flush_output_on_port(pmd, p);
> - }
> + if (OVS_UNLIKELY(!dp_packet_batch_is_empty(&p->output_pkts)
> + && packets_->packets[0]->source
> + != p->output_pkts.packets[0]->source)) {
> + /* netdev-dpdk assumes that all packets in a single
> + * output batch has the same source. Flush here to
> + * avoid memory access issues. */
> + dp_netdev_pmd_flush_output_on_port(pmd, p);
> + }
> #endif
> - if (dp_packet_batch_size(&p->output_pkts)
> - + dp_packet_batch_size(packets_) > NETDEV_MAX_BURST) {
> - /* Flush here to avoid overflow. */
> - dp_netdev_pmd_flush_output_on_port(pmd, p);
> - }
> - if (dp_packet_batch_is_empty(&p->output_pkts)) {
> - pmd->n_output_batches++;
> - }
> - DP_PACKET_BATCH_FOR_EACH (i, packet, packets_) {
> - p->output_pkts_rxqs[dp_packet_batch_size(&p->output_pkts)] =
> - pmd->ctx.last_rxq;
> - dp_packet_batch_add(&p->output_pkts, packet);
> + if (dp_packet_batch_size(&p->output_pkts)
> + + dp_packet_batch_size(packets_) > NETDEV_MAX_BURST) {
> + /* Flush here to avoid overflow. */
> + dp_netdev_pmd_flush_output_on_port(pmd, p);
> + }
> + if (dp_packet_batch_is_empty(&p->output_pkts)) {
> + pmd->n_output_batches++;
> + }
> +
> + struct dp_packet *packet;
> + DP_PACKET_BATCH_FOR_EACH (i, packet, packets_) {
> + p->output_pkts_rxqs[dp_packet_batch_size(&p->output_pkts)] =
> + pmd->ctx.last_rxq;
> + dp_packet_batch_add(&p->output_pkts, packet);
> + }
> + return true;
> +}
> +
> +static bool
> +dp_execute_lb_output_action(struct dp_netdev_pmd_thread *pmd,
> + struct dp_packet_batch *packets_,
> + bool should_steal, uint32_t bond)
> +{
> + struct tx_bond *p_bond = pmd_tx_bond_cache_lookup(pmd, bond);
> + if (!p_bond) {
> + return false;
> + }
> +
> + struct dp_packet_batch del_pkts;
> + dp_packet_batch_init(&del_pkts);
> +
> + struct dp_packet *packet;
> + DP_PACKET_BATCH_FOR_EACH (i, packet, packets_) {
> + /*
> + * Lookup the bond-hash table using hash to get the slave.
> + */
> + uint32_t hash = dp_packet_get_rss_hash(packet);
> + struct slave_entry *s_entry = &p_bond->slave_buckets[hash &
> BOND_MASK];
> + odp_port_t bond_member = s_entry->slave_id;
> + uint32_t size = dp_packet_size(packet);
> +
> + struct dp_packet_batch output_pkt;
> + dp_packet_batch_init_packet(&output_pkt, packet);
> + if (OVS_LIKELY(dp_execute_output_action(pmd, &output_pkt,
> should_steal,
> + bond_member))) {
> + /* Update slave stats. */
> + non_atomic_ullong_add(&s_entry->n_packets, 1);
> + non_atomic_ullong_add(&s_entry->n_bytes, size);
> + } else {
> + dp_packet_batch_add(&del_pkts, packet);
> }
> - return 0;
> }
> - return -1;
> +
> + /* Delete packets that failed OUTPUT action */
> + dp_packet_delete_batch(&del_pkts, should_steal);
> + return true;
> }
>
> static void
> @@ -7352,59 +7386,21 @@ dp_execute_cb(void *aux_, struct dp_packet_batch
> *packets_,
> struct dp_netdev *dp = pmd->dp;
> int type = nl_attr_type(a);
> struct tx_port *p;
> - int ret;
>
> switch ((enum ovs_action_attr)type) {
> case OVS_ACTION_ATTR_OUTPUT:
> - ret = dp_execute_output_action(pmd, packets_, should_steal,
> - nl_attr_get_odp_port(a));
> - if (ret == 0) {
> - /* Output action executed successfully. */
> + if (dp_execute_output_action(pmd, packets_, should_steal,
> + nl_attr_get_odp_port(a))) {
> return;
> }
> break;
>
> - case OVS_ACTION_ATTR_LB_OUTPUT: {
> - uint32_t bond = nl_attr_get_u32(a);
> - uint32_t bond_member;
> - uint32_t bucket, hash;
> - struct dp_packet_batch del_pkts;
> - struct dp_packet_batch output_pkt;
> - struct dp_packet *packet;
> - struct tx_bond *p_bond;
> - struct slave_entry *s_entry;
> - uint32_t size;
> -
> - p_bond = pmd_tx_bond_cache_lookup(pmd, bond);
> - dp_packet_batch_init(&del_pkts);
> - if (p_bond) {
> - DP_PACKET_BATCH_FOR_EACH (i, packet, packets_) {
> - /*
> - * Lookup the bond-hash table using hash to get the slave.
> - */
> - hash = dp_packet_get_rss_hash(packet);
> - bucket = hash & BOND_MASK;
> - s_entry = &p_bond->slave_buckets[bucket];
> - bond_member = s_entry->slave_id;
> - size = dp_packet_size(packet);
> -
> - dp_packet_batch_init_packet(&output_pkt, packet);
> - ret = dp_execute_output_action(pmd, &output_pkt,
> should_steal,
> - u32_to_odp(bond_member));
> - if (OVS_UNLIKELY(ret != 0)) {
> - dp_packet_batch_add(&del_pkts, packet);
> - } else {
> - /* Update slave stats. */
> - non_atomic_ullong_add(&s_entry->n_packets, 1);
> - non_atomic_ullong_add(&s_entry->n_bytes, size);
> - }
> - }
> - /* Delete packets that failed OUTPUT action */
> - dp_packet_delete_batch(&del_pkts, should_steal);
> + case OVS_ACTION_ATTR_LB_OUTPUT:
> + if (dp_execute_lb_output_action(pmd, packets_, should_steal,
> + nl_attr_get_u32(a))) {
> return;
> }
> break;
> - }
>
> case OVS_ACTION_ATTR_TUNNEL_PUSH:
> if (should_steal) {
> @@ -7936,23 +7932,23 @@ dpif_netdev_ipf_dump_done(struct dpif *dpif
> OVS_UNUSED, void *ipf_dump_ctx)
> }
>
> static int
> -dpif_netdev_bond_add(struct dpif *dpif, uint32_t bond_id, uint32_t
> slave_map[])
> +dpif_netdev_bond_add(struct dpif *dpif, uint32_t bond_id,
> + odp_port_t slave_map[])
> {
> struct dp_netdev *dp = get_dp_netdev(dpif);
> - struct dp_netdev_pmd_thread *pmd;
> - uint32_t bucket;
> - struct tx_bond *dp_bond_entry = NULL;
>
> ovs_mutex_lock(&dp->bond_mutex);
> /*
> * Lookup for the bond. If already exists, just update the slave-map.
> * Else create new.
> */
> - dp_bond_entry = tx_bond_lookup(&dp->bonds, bond_id);
> + struct tx_bond *dp_bond_entry = tx_bond_lookup(&dp->bonds, bond_id);
> if (dp_bond_entry) {
> - for (bucket = 0; bucket <= BOND_MASK; bucket++) {
> + for (int bucket = 0; bucket <= BOND_MASK; bucket++) {
> dp_bond_entry->slave_buckets[bucket].slave_id =
> slave_map[bucket];
> }
> +
> + struct dp_netdev_pmd_thread *pmd;
> CMAP_FOR_EACH (pmd, node, &dp->poll_threads) {
> ovs_mutex_lock(&pmd->bond_mutex);
> dp_netdev_add_bond_tx_to_pmd(pmd, dp_bond_entry);
> @@ -7961,12 +7957,14 @@ dpif_netdev_bond_add(struct dpif *dpif, uint32_t
> bond_id, uint32_t slave_map[])
> } else {
> struct tx_bond *dp_bond = xzalloc(sizeof *dp_bond);
> dp_bond->bond_id = bond_id;
> - for (bucket = 0; bucket < BOND_BUCKETS; bucket++) {
> + for (int bucket = 0; bucket < BOND_BUCKETS; bucket++) {
> dp_bond->slave_buckets[bucket].slave_id = slave_map[bucket];
> }
> hmap_insert(&dp->bonds, &dp_bond->node,
> hash_bond_id(dp_bond->bond_id));
> +
> /* Insert the bond map in all pmds. */
> + struct dp_netdev_pmd_thread *pmd;
> CMAP_FOR_EACH (pmd, node, &dp->poll_threads) {
> ovs_mutex_lock(&pmd->bond_mutex);
> dp_netdev_add_bond_tx_to_pmd(pmd, dp_bond);
> @@ -7981,15 +7979,14 @@ static int
> dpif_netdev_bond_del(struct dpif *dpif, uint32_t bond_id)
> {
> struct dp_netdev *dp = get_dp_netdev(dpif);
> - struct dp_netdev_pmd_thread *pmd;
> - struct tx_bond *dp_bond_entry = NULL;
>
> ovs_mutex_lock(&dp->bond_mutex);
>
> /* Find the bond and delete it if present */
> - dp_bond_entry = tx_bond_lookup(&dp->bonds, bond_id);
> + struct tx_bond *dp_bond_entry = tx_bond_lookup(&dp->bonds, bond_id);
> if (dp_bond_entry) {
> /* Remove the bond map in all pmds. */
> + struct dp_netdev_pmd_thread *pmd;
> CMAP_FOR_EACH (pmd, node, &dp->poll_threads) {
> ovs_mutex_lock(&pmd->bond_mutex);
> dp_netdev_del_bond_tx_from_pmd(pmd, dp_bond_entry);
> @@ -8008,24 +8005,22 @@ dpif_netdev_bond_stats_get(struct dpif *dpif,
> uint32_t bond_id,
> uint64_t *n_bytes)
> {
> struct dp_netdev *dp = get_dp_netdev(dpif);
> - struct dp_netdev_pmd_thread *pmd;
> - struct tx_bond *dp_bond_entry = NULL;
> - struct tx_bond *pmd_bond_entry = NULL;
> - uint32_t i;
>
> ovs_mutex_lock(&dp->bond_mutex);
>
> /* Find the bond and retrieve stats if present */
> - dp_bond_entry = tx_bond_lookup(&dp->bonds, bond_id);
> + struct tx_bond *dp_bond_entry = tx_bond_lookup(&dp->bonds, bond_id);
> if (dp_bond_entry) {
> /* Search the bond in all PMDs */
> + struct dp_netdev_pmd_thread *pmd;
> CMAP_FOR_EACH (pmd, node, &dp->poll_threads) {
> - uint64_t pmd_n_bytes;
> ovs_mutex_lock(&pmd->bond_mutex);
> - pmd_bond_entry = tx_bond_lookup(&pmd->bond_cache, bond_id);
> + struct tx_bond *pmd_bond_entry
> + = tx_bond_lookup(&pmd->bond_cache, bond_id);
> if (pmd_bond_entry) {
> /* Read bond stats. */
> - for (i = 0;i <= BOND_MASK; i++) {
> + for (int i = 0; i <= BOND_MASK; i++) {
> + uint64_t pmd_n_bytes;
> atomic_read_relaxed(
> &pmd_bond_entry->slave_buckets[i].n_bytes,
> &pmd_n_bytes);
> diff --git a/lib/dpif-provider.h b/lib/dpif-provider.h
> index c544684ec0c0..82e2149d2a79 100644
> --- a/lib/dpif-provider.h
> +++ b/lib/dpif-provider.h
> @@ -616,7 +616,8 @@ struct dpif_class {
> struct ofputil_meter_stats *, uint16_t n_bands);
>
> /* Adds a bond with 'bond_id' and the slave-map to 'dpif'. */
> - int (*bond_add)(struct dpif *dpif, uint32_t bond_id, uint32_t
> slave_map[]);
> + int (*bond_add)(struct dpif *dpif, uint32_t bond_id,
> + odp_port_t slave_map[]);
> /* Removes bond identified by 'bond_id' from 'dpif'. */
> int (*bond_del)(struct dpif *dpif, uint32_t bond_id);
> /* Reads bond stats from 'dpif'. */
> diff --git a/lib/dpif.c b/lib/dpif.c
> index 2411c2cbeb41..71a752dba7dd 100644
> --- a/lib/dpif.c
> +++ b/lib/dpif.c
> @@ -1990,7 +1990,7 @@ dpif_meter_del(struct dpif *dpif, ofproto_meter_id
> meter_id,
> }
>
> int
> -dpif_bond_add(struct dpif *dpif, uint32_t bond_id, uint32_t slave_map[])
> +dpif_bond_add(struct dpif *dpif, uint32_t bond_id, odp_port_t slave_map[])
> {
> int error = 0;
>
> @@ -2013,8 +2013,9 @@ dpif_bond_del(struct dpif *dpif, uint32_t bond_id)
> return error;
> }
>
> -int dpif_bond_stats_get(struct dpif *dpif, uint32_t bond_id,
> - uint64_t *n_bytes)
> +int
> +dpif_bond_stats_get(struct dpif *dpif, uint32_t bond_id,
> + uint64_t *n_bytes)
> {
> int error = 0;
>
> diff --git a/lib/dpif.h b/lib/dpif.h
> index 82b1901697a3..6907bf19925d 100644
> --- a/lib/dpif.h
> +++ b/lib/dpif.h
> @@ -907,7 +907,7 @@ bool dpif_supports_tnl_push_pop(const struct dpif *);
>
> bool dpif_supports_balance_tcp_opt(const struct dpif *);
>
> -int dpif_bond_add(struct dpif *dpif, uint32_t bond_id, uint32_t slave_map[]);
> +int dpif_bond_add(struct dpif *dpif, uint32_t bond_id, odp_port_t
> slave_map[]);
> int dpif_bond_del(struct dpif *dpif, uint32_t bond_id);
> int dpif_bond_stats_get(struct dpif *dpif, uint32_t bond_id,
> uint64_t *n_bytes);
> diff --git a/ofproto/bond.c b/ofproto/bond.c
> index df9110e92d18..59fc5c9c43b3 100644
> --- a/ofproto/bond.c
> +++ b/ofproto/bond.c
> @@ -335,7 +335,7 @@ update_recirc_rules__(struct bond *bond, uint32_t
> bond_recirc_id)
> uint64_t ofpacts_stub[128 / 8];
> struct ofpbuf ofpacts;
> int i;
> - uint32_t slave_map[BOND_MASK];
> + ofp_port_t slave_map[BOND_MASK];
>
> ofpbuf_use_stub(&ofpacts, ofpacts_stub, sizeof ofpacts_stub);
>
> @@ -356,7 +356,7 @@ update_recirc_rules__(struct bond *bond, uint32_t
> bond_recirc_id)
> &bond->hash[i].pr_rule);
> slave_map[i] = slave->ofp_port;
> } else {
> - slave_map[i] = -1;
> + slave_map[i] = OFP_PORT_C(-1);
> }
> }
> ofproto_dpif_bundle_add(bond->ofproto, bond->recirc_id, slave_map);
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index 13789ac35225..5c1d0b981e06 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -4203,9 +4203,8 @@ compose_output_action__(struct xlate_ctx *ctx,
> ofp_port_t ofp_port,
> *
> * Currently support for netdev datapath only.
> */
> - nl_msg_put_odp_port(ctx->odp_actions,
> - OVS_ACTION_ATTR_LB_OUTPUT,
> - xr->recirc_id);
> + nl_msg_put_u32(ctx->odp_actions, OVS_ACTION_ATTR_LB_OUTPUT,
> + xr->recirc_id);
> } else {
> act_hash = nl_msg_put_unspec_uninit(ctx->odp_actions,
> OVS_ACTION_ATTR_HASH,
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index 5f9f1889bc32..1ccebcab4438 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -3441,32 +3441,23 @@ bundle_remove(struct ofport *port_)
>
> int
> ofproto_dpif_bundle_add(struct ofproto_dpif *ofproto,
> - uint32_t bond_id,
> - uint32_t slave_map[])
> + uint32_t bond_id, const ofp_port_t slave_map[])
> {
> - int error;
> - uint32_t bucket;
> -
> /* Convert ofp_port to odp_port */
> - for (bucket = 0; bucket < BOND_BUCKETS; bucket++) {
> - if (slave_map[bucket] != -1) {
> - slave_map[bucket] =
> - ofp_port_to_odp_port(ofproto, slave_map[bucket]);
> - }
> + odp_port_t odp_map[BOND_BUCKETS];
> + for (int bucket = 0; bucket < BOND_BUCKETS; bucket++) {
> + odp_map[bucket] = (slave_map[bucket] == OFP_PORT_C(-1)
> + ? ODP_PORT_C(-1)
> + : ofp_port_to_odp_port(ofproto,
> slave_map[bucket]));
> }
>
> - error = dpif_bond_add(ofproto->backer->dpif, bond_id, slave_map);
> - return error;
> + return dpif_bond_add(ofproto->backer->dpif, bond_id, odp_map);
> }
>
> int
> -ofproto_dpif_bundle_del(struct ofproto_dpif *ofproto,
> - uint32_t bond_id)
> +ofproto_dpif_bundle_del(struct ofproto_dpif *ofproto, uint32_t bond_id)
> {
> - int error;
> -
> - error = dpif_bond_del(ofproto->backer->dpif, bond_id);
> - return error;
> + return dpif_bond_del(ofproto->backer->dpif, bond_id);
> }
>
> static void
> diff --git a/ofproto/ofproto-dpif.h b/ofproto/ofproto-dpif.h
> index 484a6d4b5272..aa46d9d67fe2 100644
> --- a/ofproto/ofproto-dpif.h
> +++ b/ofproto/ofproto-dpif.h
> @@ -378,11 +378,9 @@ int ofproto_dpif_add_internal_flow(struct ofproto_dpif *,
> struct rule **rulep);
> int ofproto_dpif_delete_internal_flow(struct ofproto_dpif *, struct match *,
> int priority);
> -int ofproto_dpif_bundle_add(struct ofproto_dpif *,
> - uint32_t bond_id,
> - uint32_t slave_map[]);
> -int ofproto_dpif_bundle_del(struct ofproto_dpif *,
> - uint32_t bond_id);
> +int ofproto_dpif_bundle_add(struct ofproto_dpif *, uint32_t bond_id,
> + const ofp_port_t slave_map[]);
> +int ofproto_dpif_bundle_del(struct ofproto_dpif *, uint32_t bond_id);
>
> bool ovs_native_tunneling_is_on(struct ofproto_dpif *);
>
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
