Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bgp give some ordering increases #16774

Merged
merged 2 commits into from
Sep 17, 2024

Conversation

donaldsharp
Copy link
Member

a) Allow bgp_process to push a item to the front of the list
b) Remove a redundant check

@@ -4091,7 +4091,10 @@ void bgp_process(struct bgp *bgp, struct bgp_dest *dest,

/* can't be enqueued twice */
assert(STAILQ_NEXT(dest, pq) == NULL);
STAILQ_INSERT_TAIL(&pqnode->pqueue, dest, pq);
if (early_process)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In what case is this true?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's never true at the moment. I'm just adding the ability to use this. It will be useful down the line, I am trying to coordinate with some other people that is all

Copy link
Member

@riw777 riw777 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good

@@ -806,7 +806,8 @@ extern void bgp_withdraw(struct peer *peer, const struct prefix *p,

/* for bgp_nexthop and bgp_damp */
extern void bgp_process(struct bgp *bgp, struct bgp_dest *dest,
struct bgp_path_info *pi, afi_t afi, safi_t safi);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

instead of changing this function, maybe add a "process_early" version, that you can use in the specific places where you want the "early" behavior?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

adding "early," though, allows you to control the behavior on a route-by-route basis, perhaps even (eventually) using a route-map or some policy to let some routes be processed before others (?) ...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that there needs to be an API, and that it needs to be called - I'm just thinking it'd be better to have a dedicated api, that would only involve changing the places where it's needed. This change requires many callers to be changed, so that they can say "false"...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a bgp_process_early I believe it solves Marks issues

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's great, thanks.
so ... could we have a comment, even a one-liner, to say that the "early" version puts the dest at the front of the list?

@donaldsharp donaldsharp force-pushed the bgp_give_some_ordering_increases branch from ed70531 to 304ebd8 Compare September 11, 2024 19:47
@github-actions github-actions bot added size/M and removed size/L labels Sep 11, 2024
Copy link
Contributor

@mjstapp mjstapp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No code is using the new api yet - is that right?

@@ -806,7 +806,8 @@ extern void bgp_withdraw(struct peer *peer, const struct prefix *p,

/* for bgp_nexthop and bgp_damp */
extern void bgp_process(struct bgp *bgp, struct bgp_dest *dest,
struct bgp_path_info *pi, afi_t afi, safi_t safi);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's great, thanks.
so ... could we have a comment, even a one-liner, to say that the "early" version puts the dest at the front of the list?

There is a need to be able to process certain bgp
routes earlier than others.  Especially when there
is major trauma going on in the network.  Start
the ability for this to happen.

Signed-off-by: Donald Sharp <[email protected]>
The check for an equivalent bgp pointer makes no sense
in the context of the workqueue as that we have a
work queue per bgp process, as such the bgp pointer
will always be the same as the pqnode.

Signed-off-by: Donald Sharp <[email protected]>
@donaldsharp donaldsharp force-pushed the bgp_give_some_ordering_increases branch from 304ebd8 to 220b4ef Compare September 11, 2024 21:46
@donaldsharp
Copy link
Member Author

yes no-one is using it yet.

@riw777 riw777 merged commit da96ad0 into FRRouting:master Sep 17, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants