On Wed, Nov 25, 2015 at 12:14 PM, Paul Jakma <[email protected]> wrote:
> * bgp_route.c: (bgp_info_cmp) This function is supposed to return a
> preference between the given paths, and does so as binary either or.
> When
> mpath was added, the binary return value was left as is and instead an
> out
> parameter 'paths_eq' was added to indicate the mpath-equality case. It's
> a bit odd, as is the resulting logic in the caller.
>
> Regularise things again by making the function return a strcmp like
> trinary return value of -1,0,1. Get rid of the mpath specific arguments,
> but pass in afi/safi as part of the general context - that plus the
> (struct bgp *) is enough to access configuration.
>
> Update the return values.
>
> The mpath check was testing the IGP metric for equality, even though
> previous to the mpath changes (and consistent with the behaviour of all
> the other tests bar the end), equality results in continuing through to
> the next comparison. Just go back to the previous way - each test finds a
> preference to return, or continues on to let further tests have a go.
>
> (bgp_best_selection) Get rid of the (struct bgp_maxpaths_cfg *) arg, we
> can't add state for every optional feature to the argument list - they
> have to look it up as needed. Do pass through the very general afi/safi
> context though (saves several lookups through the route_node).
>
> Adjust for the new trinary bgp_info_cmp return value and updated args.
> Do the mpath clearing/accumulation in one place, in each loop.
>
> Call to bgp_info_mpath_update similarly gets updated, as there's no cfg
> to
> pass.
>
> (bgp_process_{rsclient,main}) match bgp_best_selection changes.
> * bgp_mpath.c: (bgp_mpath_is_configured_sort) Helper for whether mpath is
> enabled by peer sort.
> (bgp_mpath_is_configured) ditto, generally.
> (bgp_info_mpath_update) caller no longer has the cfg to pass in, look it
> up.
> * bgp_mpath.h: Export the above and Match .c changes.
> ---
> bgpd/bgp_mpath.c | 34 ++++++++++-
> bgpd/bgp_mpath.h | 4 +-
> bgpd/bgp_route.c | 173
> ++++++++++++++++++++++++++-----------------------------
> 3 files changed, 118 insertions(+), 93 deletions(-)
>
> diff --git a/bgpd/bgp_mpath.c b/bgpd/bgp_mpath.c
> index 7999d16..db5996e 100644
> --- a/bgpd/bgp_mpath.c
> +++ b/bgpd/bgp_mpath.c
> @@ -39,6 +39,33 @@
> #include "bgpd/bgp_ecommunity.h"
> #include "bgpd/bgp_mpath.h"
>
> +bool
> +bgp_mpath_is_configured_sort (struct bgp *bgp, bgp_peer_sort_t sort,
> + afi_t afi, safi_t safi)
> +{
> + struct bgp_maxpaths_cfg *cfg = &bgp->maxpaths[afi][safi];
> +
> + /* XXX: BGP_DEFAULT_MAXPATHS is 1, and this test only seems to make
> sense
> + * if if it stays 1, so not sure the DEFAULT define is that useful.
> + */
> + switch (sort)
> + {
> + case BGP_PEER_IBGP:
> + return cfg->maxpaths_ibgp != BGP_DEFAULT_MAXPATHS;
>
It would be worth future proofing this (we have a patch to increase
BGP_DEFAULT_MAXPATHS) and return true if cfg->maxpaths_ibgp > 1
> + case BGP_PEER_EBGP:
> + return cfg->maxpaths_ebgp != BGP_DEFAULT_MAXPATHS;
> + default:
> + return false;
> + }
> +}
> +
> +bool
> +bgp_mpath_is_configured (struct bgp *bgp, afi_t afi, safi_t safi)
> +{
> + return bgp_mpath_is_configured_sort (bgp, BGP_PEER_IBGP, afi, safi)
> + || bgp_mpath_is_configured_sort (bgp, BGP_PEER_EBGP, afi, safi);
> +}
> +
> /*
> * bgp_maximum_paths_set
> *
> @@ -395,7 +422,7 @@ bgp_info_mpath_attr_set (struct bgp_info *binfo,
> struct attr *attr)
> void
> bgp_info_mpath_update (struct bgp_node *rn, struct bgp_info *new_best,
> struct bgp_info *old_best, struct list *mp_list,
> - struct bgp_maxpaths_cfg *mpath_cfg)
> + afi_t afi, safi_t safi)
> {
> u_int16_t maxpaths, mpath_count, old_mpath_count;
> struct listnode *mp_node, *mp_next_node;
> @@ -410,8 +437,11 @@ bgp_info_mpath_update (struct bgp_node *rn, struct
> bgp_info *new_best,
> old_mpath_count = 0;
> prev_mpath = new_best;
> mp_node = listhead (mp_list);
> + struct bgp_maxpaths_cfg *mpath_cfg;
> debug = BGP_DEBUG (events, EVENTS);
> -
> +
> + mpath_cfg = &new_best->peer->bgp->maxpaths[afi][safi];
> +
> if (debug)
> prefix2str (&rn->p, pfx_buf, sizeof (pfx_buf));
>
> diff --git a/bgpd/bgp_mpath.h b/bgpd/bgp_mpath.h
> index 37b9ac8..2a84d5e 100644
> --- a/bgpd/bgp_mpath.h
> +++ b/bgpd/bgp_mpath.h
> @@ -51,6 +51,8 @@ struct bgp_info_mpath
> /* Functions to support maximum-paths configuration */
> extern int bgp_maximum_paths_set (struct bgp *, afi_t, safi_t, int,
> u_int16_t);
> extern int bgp_maximum_paths_unset (struct bgp *, afi_t, safi_t, int);
> +bool bgp_mpath_is_configured_sort (struct bgp *, bgp_peer_sort_t, afi_t,
> safi_t);
> +bool bgp_mpath_is_configured (struct bgp *, afi_t, safi_t);
>
> /* Functions used by bgp_best_selection to record current
> * multipath selections
> @@ -61,7 +63,7 @@ extern void bgp_mp_list_add (struct list *, struct
> bgp_info *);
> extern void bgp_mp_dmed_deselect (struct bgp_info *);
> extern void bgp_info_mpath_update (struct bgp_node *, struct bgp_info *,
> struct bgp_info *, struct list *,
> - struct bgp_maxpaths_cfg *);
> + afi_t, safi_t);
> extern void bgp_info_mpath_aggregate_update (struct bgp_info *,
> struct bgp_info *);
>
> diff --git a/bgpd/bgp_route.c b/bgpd/bgp_route.c
> index 316fa5a..1a49cb3 100644
> --- a/bgpd/bgp_route.c
> +++ b/bgpd/bgp_route.c
> @@ -321,10 +321,12 @@ bgp_med_value (struct attr *attr, struct bgp *bgp)
> }
> }
>
> -/* Compare two bgp route entity. br is preferable then return 1. */
> +/* Compare two bgp route entity. Return -1 if new is preferred, 1 if
> exist
> + * is preferred, or 0 if they are the same (usually will only occur if
> + * multipath is enabled */
> static int
> bgp_info_cmp (struct bgp *bgp, struct bgp_info *new, struct bgp_info
> *exist,
> - int *paths_eq)
> + afi_t afi, safi_t safi)
> {
> struct attr *newattr, *existattr;
> struct attr_extra *newattre, *existattre;
> @@ -345,13 +347,11 @@ bgp_info_cmp (struct bgp *bgp, struct bgp_info *new,
> struct bgp_info *exist,
> int confed_as_route;
> int ret;
>
> - *paths_eq = 0;
> -
> /* 0. Null check. */
> if (new == NULL)
> - return 0;
> - if (exist == NULL)
> return 1;
> + if (exist == NULL)
> + return -1;
>
> newattr = new->attr;
> existattr = exist->attr;
> @@ -367,9 +367,9 @@ bgp_info_cmp (struct bgp *bgp, struct bgp_info *new,
> struct bgp_info *exist,
> exist_weight = existattre->weight;
>
> if (new_weight > exist_weight)
> - return 1;
> + return -1;
> if (new_weight < exist_weight)
> - return 0;
> + return 1;
>
> /* 2. Local preference check. */
> new_pref = exist_pref = bgp->default_local_pref;
> @@ -380,9 +380,9 @@ bgp_info_cmp (struct bgp *bgp, struct bgp_info *new,
> struct bgp_info *exist,
> exist_pref = existattr->local_pref;
>
> if (new_pref > exist_pref)
> - return 1;
> + return -1;
> if (new_pref < exist_pref)
> - return 0;
> + return 1;
>
> /* 3. Local route check. We prefer:
> * - BGP_ROUTE_STATIC
> @@ -390,9 +390,9 @@ bgp_info_cmp (struct bgp *bgp, struct bgp_info *new,
> struct bgp_info *exist,
> * - BGP_ROUTE_REDISTRIBUTE
> */
> if (! (new->sub_type == BGP_ROUTE_NORMAL))
> - return 1;
> + return -1;
> if (! (exist->sub_type == BGP_ROUTE_NORMAL))
> - return 0;
> + return 1;
>
> /* 4. AS path length check. */
> if (! bgp_flag_check (bgp, BGP_FLAG_ASPATH_IGNORE))
> @@ -408,26 +408,26 @@ bgp_info_cmp (struct bgp *bgp, struct bgp_info *new,
> struct bgp_info *exist,
> aspath_hops += aspath_count_confeds (newattr->aspath);
>
> if ( aspath_hops < (exist_hops + exist_confeds))
> - return 1;
> + return -1;
> if ( aspath_hops > (exist_hops + exist_confeds))
> - return 0;
> + return 1;
> }
> else
> {
> int newhops = aspath_count_hops (newattr->aspath);
>
> if (newhops < exist_hops)
> - return 1;
> + return -1;
> if (newhops > exist_hops)
> - return 0;
> + return 1;
> }
> }
>
> /* 5. Origin check. */
> if (newattr->origin < existattr->origin)
> - return 1;
> + return -1;
> if (newattr->origin > existattr->origin)
> - return 0;
> + return 1;
>
> /* 6. MED check. */
> internal_as_route = (aspath_count_hops (newattr->aspath) == 0
> @@ -448,9 +448,9 @@ bgp_info_cmp (struct bgp *bgp, struct bgp_info *new,
> struct bgp_info *exist,
> exist_med = bgp_med_value (exist->attr, bgp);
>
> if (new_med < exist_med)
> - return 1;
> + return -1;
> if (new_med > exist_med)
> - return 0;
> + return 1;
> }
>
> /* 7. Peer type check. */
> @@ -459,10 +459,10 @@ bgp_info_cmp (struct bgp *bgp, struct bgp_info *new,
> struct bgp_info *exist,
>
> if (new_sort == BGP_PEER_EBGP
> && (exist_sort == BGP_PEER_IBGP || exist_sort == BGP_PEER_CONFED))
> - return 1;
> + return -1;
> if (exist_sort == BGP_PEER_EBGP
> && (new_sort == BGP_PEER_IBGP || new_sort == BGP_PEER_CONFED))
> - return 0;
> + return 1;
>
> /* 8. IGP metric check. */
> newm = existm = 0;
> @@ -473,41 +473,32 @@ bgp_info_cmp (struct bgp *bgp, struct bgp_info *new,
> struct bgp_info *exist,
> existm = exist->extra->igpmetric;
>
> if (newm < existm)
> - ret = 1;
> + return -1;
> if (newm > existm)
> - ret = 0;
> + return 1;
>
> /* 9. Maximum path check. */
> - if (newm == existm)
> + if (bgp_mpath_is_configured (bgp, afi, safi))
> {
> if (bgp_flag_check(bgp, BGP_FLAG_ASPATH_MULTIPATH_RELAX))
> {
> -
> - /*
> - * For the two paths, all comparison steps till IGP metric
> - * have succeeded - including AS_PATH hop count. Since 'bgp
> - * bestpath as-path multipath-relax' knob is on, we don't need
> - * an exact match of AS_PATH. Thus, mark the paths are equal.
> - * That will trigger both these paths to get into the multipath
> - * array.
> - */
> - *paths_eq = 1;
> + /*
> + * For the two paths, all comparison steps till IGP metric
> + * have succeeded - including AS_PATH hop count. Since 'bgp
> + * bestpath as-path multipath-relax' knob is on, we don't need
> + * an exact match of AS_PATH. Thus, mark the paths are equal.
> + * That will trigger both these paths to get into the multipath
> + * array.
> + */
> + return 0;
>
I don't follow how bestpath selection is working when max-paths is
enabled. Say we have two paths who only differ by router-id...we still
need to get to step #11 to determine which is the winner.
Daniel
> }
> else if (new->peer->sort == BGP_PEER_IBGP)
> - {
> - if (aspath_cmp (new->attr->aspath, exist->attr->aspath))
> - *paths_eq = 1;
> - }
> + {
> + if (aspath_cmp (new->attr->aspath, exist->attr->aspath))
> + return 0;
> + }
> else if (new->peer->as == exist->peer->as)
> - *paths_eq = 1;
> - }
> - else
> - {
> - /*
> - * TODO: If unequal cost ibgp multipath is enabled we can
> - * mark the paths as equal here instead of returning
> - */
> - return ret;
> + return 0;
> }
>
> /* 10. If both paths are external, prefer the path that was received
> @@ -519,9 +510,9 @@ bgp_info_cmp (struct bgp *bgp, struct bgp_info *new,
> struct bgp_info *exist,
> && exist_sort == BGP_PEER_EBGP)
> {
> if (CHECK_FLAG (new->flags, BGP_INFO_SELECTED))
> - return 1;
> + return -1;
> if (CHECK_FLAG (exist->flags, BGP_INFO_SELECTED))
> - return 0;
> + return 1;
> }
>
> /* 11. Router-ID comparision. */
> @@ -539,9 +530,9 @@ bgp_info_cmp (struct bgp *bgp, struct bgp_info *new,
> struct bgp_info *exist,
> exist_id.s_addr = exist->peer->remote_id.s_addr;
>
> if (ntohl (new_id.s_addr) < ntohl (exist_id.s_addr))
> - return 1;
> + return -1;
> if (ntohl (new_id.s_addr) > ntohl (exist_id.s_addr))
> - return 0;
> + return 1;
>
> /* 12. Cluster length comparision. */
> new_cluster = exist_cluster = 0;
> @@ -552,32 +543,32 @@ bgp_info_cmp (struct bgp *bgp, struct bgp_info *new,
> struct bgp_info *exist,
> exist_cluster = existattre->cluster->length;
>
> if (new_cluster < exist_cluster)
> - return 1;
> + return -1;
> if (new_cluster > exist_cluster)
> - return 0;
> + return 1;
>
> /* 13. Neighbor address comparision. */
> /* Do this only if neither path is "stale" as stale paths do not have
> * valid peer information (as the connection may or may not be up).
> */
> if (CHECK_FLAG (exist->flags, BGP_INFO_STALE))
> - return 1;
> + return -1;
> if (CHECK_FLAG (new->flags, BGP_INFO_STALE))
> - return 0;
> + return 1;
> /* locally configured routes to advertise do not have su_remote */
> if (new->peer->su_remote == NULL)
> - return 0;
> - if (exist->peer->su_remote == NULL)
> return 1;
> + if (exist->peer->su_remote == NULL)
> + return -1;
>
> ret = sockunion_cmp (new->peer->su_remote, exist->peer->su_remote);
> -
> +
> if (ret == 1)
> - return 0;
> - if (ret == -1)
> return 1;
> + if (ret == -1)
> + return -1;
>
> - return 1;
> + return -1;
> }
>
> static enum filter_type
> @@ -1325,8 +1316,8 @@ struct bgp_info_pair
>
> static void
> bgp_best_selection (struct bgp *bgp, struct bgp_node *rn,
> - struct bgp_maxpaths_cfg *mpath_cfg,
> - struct bgp_info_pair *result)
> + struct bgp_info_pair *result,
> + afi_t afi, safi_t safi)
> {
> struct bgp_info *new_select;
> struct bgp_info *old_select;
> @@ -1334,12 +1325,11 @@ bgp_best_selection (struct bgp *bgp, struct
> bgp_node *rn,
> struct bgp_info *ri1;
> struct bgp_info *ri2;
> struct bgp_info *nextri = NULL;
> - int paths_eq, do_mpath;
> + int cmpret, do_mpath;
> struct list mp_list;
> -
> +
> bgp_mp_list_init (&mp_list);
> - do_mpath = (mpath_cfg->maxpaths_ebgp != BGP_DEFAULT_MAXPATHS ||
> - mpath_cfg->maxpaths_ibgp != BGP_DEFAULT_MAXPATHS);
> + do_mpath = bgp_mpath_is_configured (bgp, afi, safi);
>
> /* bgp deterministic-med */
> new_select = NULL;
> @@ -1377,19 +1367,21 @@ bgp_best_selection (struct bgp *bgp, struct
> bgp_node *rn,
> {
> if (CHECK_FLAG (ri2->flags, BGP_INFO_SELECTED))
> old_select = ri2;
> - if (bgp_info_cmp (bgp, ri2, new_select, &paths_eq))
> + if ((cmpret = bgp_info_cmp (bgp, ri2, new_select, afi,
> safi))
> + == -1)
> {
> bgp_info_unset_flag (rn, new_select,
> BGP_INFO_DMED_SELECTED);
> new_select = ri2;
> - if (do_mpath && !paths_eq)
> - {
> - bgp_mp_list_clear (&mp_list);
> - bgp_mp_list_add (&mp_list, ri2);
> - }
> }
>
> - if (do_mpath && paths_eq)
> - bgp_mp_list_add (&mp_list, ri2);
> + if (do_mpath)
> + {
> + if (cmpret != 0)
> + bgp_mp_list_clear (&mp_list);
> +
> + if (cmpret == 0 || cmpret == -1)
> + bgp_mp_list_add (&mp_list, ri);
> + }
>
> bgp_info_set_flag (rn, ri2, BGP_INFO_DMED_CHECK);
> }
> @@ -1397,7 +1389,7 @@ bgp_best_selection (struct bgp *bgp, struct bgp_node
> *rn,
> bgp_info_set_flag (rn, new_select, BGP_INFO_DMED_CHECK);
> bgp_info_set_flag (rn, new_select, BGP_INFO_DMED_SELECTED);
>
> - bgp_info_mpath_update (rn, new_select, old_select, &mp_list,
> mpath_cfg);
> + bgp_info_mpath_update (rn, new_select, old_select, &mp_list, afi,
> safi);
> bgp_mp_list_clear (&mp_list);
> }
>
> @@ -1436,29 +1428,30 @@ bgp_best_selection (struct bgp *bgp, struct
> bgp_node *rn,
> bgp_info_unset_flag (rn, ri, BGP_INFO_DMED_CHECK);
> bgp_info_unset_flag (rn, ri, BGP_INFO_DMED_SELECTED);
>
> - if (bgp_info_cmp (bgp, ri, new_select, &paths_eq))
> + if ((cmpret = bgp_info_cmp (bgp, ri, new_select, afi, safi)) == -1)
> {
> if (do_mpath && bgp_flag_check (bgp, BGP_FLAG_DETERMINISTIC_MED))
> bgp_mp_dmed_deselect (new_select);
>
> new_select = ri;
> -
> - if (do_mpath && !paths_eq)
> - {
> - bgp_mp_list_clear (&mp_list);
> - bgp_mp_list_add (&mp_list, ri);
> - }
> }
> - else if (do_mpath && bgp_flag_check (bgp,
> BGP_FLAG_DETERMINISTIC_MED))
> + else if (cmpret == 1 && do_mpath
> + && bgp_flag_check (bgp, BGP_FLAG_DETERMINISTIC_MED))
> bgp_mp_dmed_deselect (ri);
>
> - if (do_mpath && paths_eq)
> - bgp_mp_list_add (&mp_list, ri);
> + if (do_mpath)
> + {
> + if (cmpret != 0)
> + bgp_mp_list_clear (&mp_list);
> +
> + if (cmpret == 0 || cmpret == -1)
> + bgp_mp_list_add (&mp_list, ri);
> + }
> }
>
>
> if (!bgp_flag_check (bgp, BGP_FLAG_DETERMINISTIC_MED))
> - bgp_info_mpath_update (rn, new_select, old_select, &mp_list,
> mpath_cfg);
> + bgp_info_mpath_update (rn, new_select, old_select, &mp_list, afi,
> safi);
>
> bgp_info_mpath_aggregate_update (new_select, old_select);
> bgp_mp_list_clear (&mp_list);
> @@ -1542,7 +1535,7 @@ bgp_process_rsclient (struct work_queue *wq, void
> *data)
> struct peer *rsclient = bgp_node_table (rn)->owner;
>
> /* Best path selection. */
> - bgp_best_selection (bgp, rn, &bgp->maxpaths[afi][safi], &old_and_new);
> + bgp_best_selection (bgp, rn, &old_and_new, afi, safi);
> new_select = old_and_new.new;
> old_select = old_and_new.old;
>
> @@ -1605,7 +1598,7 @@ bgp_process_main (struct work_queue *wq, void *data)
> struct peer *peer;
>
> /* Best path selection. */
> - bgp_best_selection (bgp, rn, &bgp->maxpaths[afi][safi], &old_and_new);
> + bgp_best_selection (bgp, rn, &old_and_new, afi, safi);
> old_select = old_and_new.old;
> new_select = old_and_new.new;
>
> --
> 2.5.0
>
>
> _______________________________________________
> Quagga-dev mailing list
> [email protected]
> https://lists.quagga.net/mailman/listinfo/quagga-dev
>
_______________________________________________
Quagga-dev mailing list
[email protected]
https://lists.quagga.net/mailman/listinfo/quagga-dev
