-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Reduce the number of Singleton objects when using weight for NHG's #16609
Conversation
donaldsharp
commented
Aug 20, 2024
f176b11
to
c6dc50c
Compare
lib/nexthop.h
Outdated
@@ -206,7 +206,8 @@ uint32_t nexthop_hash_quick(const struct nexthop *nexthop); | |||
extern bool nexthop_same(const struct nexthop *nh1, const struct nexthop *nh2); | |||
extern bool nexthop_same_no_labels(const struct nexthop *nh1, | |||
const struct nexthop *nh2); | |||
extern int nexthop_cmp(const struct nexthop *nh1, const struct nexthop *nh2); | |||
extern int nexthop_cmp(const struct nexthop *nh1, const struct nexthop *nh2, | |||
bool use_weight); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe it'd be better to add a new api to make this new logic visible, instead of modifying this existing api. that would reduce the footprint of the change - and there's only one place where you want to use the new logic?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
at the moment. I am unaware of any other place this needs to be changed. I can add a new api, it just got a lot messier in the nexthop.c file( which is what I did originally )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking something like: the new api gets the logic that you have now, with the added boolean. the old function becomes a facade, a pass-through to the new api, with the new boolean hard-coded to 'true'. I think that'd largely keep changes to nexthop.c and the one zebra function where you want to use the new api logic?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah i'll do it. The fact that I had that brain fart caused me to already rewrite it like 3 times.
Just annoyed with myself
8f21cf5
to
e94b02f
Compare
Currently nexthop weight is a discriminator on whether or not a nexthop matches. There is a need to no use the weight as part of this comparison function so let's add a boolean to allow us to say use this or not. Signed-off-by: Donald Sharp <[email protected]>
Currently FRR when it has two nexthop groups: A nexthop 1 weight 5 nexthop 2 weight 6 nexthop 3 weight 7 B nexthop 1 weight 3 nexthop 2 weight 4 nexthop 3 weight 5 We end up with 5 singleton nexthops and two groups: ID: 181818168 (sharp) RefCnt: 1 Uptime: 00:04:52 VRF: default Valid, Installed Depends: (69) (70) (71) via 192.168.119.1, enp13s0 (vrf default), weight 182 via 192.168.119.2, enp13s0 (vrf default), weight 218 via 192.168.119.3, enp13s0 (vrf default), weight 255 ID: 181818169 (sharp) RefCnt: 1 Uptime: 00:02:08 VRF: default Valid, Installed Depends: (71) (127) (128) via 192.168.119.1, enp13s0 (vrf default), weight 127 via 192.168.119.2, enp13s0 (vrf default), weight 170 via 192.168.119.3, enp13s0 (vrf default), weight 255 id 69 via 192.168.119.1 dev enp13s0 scope link proto 194 id 70 via 192.168.119.2 dev enp13s0 scope link proto 194 id 71 via 192.168.119.3 dev enp13s0 scope link proto 194 id 127 via 192.168.119.1 dev enp13s0 scope link proto 194 id 128 via 192.168.119.2 dev enp13s0 scope link proto 194 id 181818168 group 69,182/70,218/71,255 proto 194 id 181818169 group 71,255/127,127/128,170 proto 194 This is not a desirable state to be in. If you have a link flapping in the network and weights are changing rapidly you end up with a large number of singleton nexthops that are being used by the nexthop groups. This fills up asic space and clutters the table. Additionally singleton nexthops cannot have any weight and the fact that you attempt to create a singleton nexthop with different weights means nothing to the linux kernel( or any asic dplane ). Let's modify the code to always create the singleton nexthops without a weight and then just creating the NHG's that use the singletons with the appropriate weight. ID: 181818168 (sharp) RefCnt: 1 Uptime: 00:00:32 VRF: default Valid, Installed Depends: (22) (24) (28) via 192.168.119.1, enp13s0 (vrf default), weight 182 via 192.168.119.2, enp13s0 (vrf default), weight 218 via 192.168.119.3, enp13s0 (vrf default), weight 255 ID: 181818169 (sharp) RefCnt: 1 Uptime: 00:00:14 VRF: default Valid, Installed Depends: (22) (24) (28) via 192.168.119.1, enp13s0 (vrf default), weight 153 via 192.168.119.2, enp13s0 (vrf default), weight 204 via 192.168.119.3, enp13s0 (vrf default), weight 255 id 22 via 192.168.119.1 dev enp13s0 scope link proto 194 id 24 via 192.168.119.2 dev enp13s0 scope link proto 194 id 28 via 192.168.119.3 dev enp13s0 scope link proto 194 id 181818168 group 22,182/24,218/28,255 proto 194 id 181818169 group 22,153/24,204/28,255 proto 194 Signed-off-by: Donald Sharp <[email protected]>
e94b02f
to
c20fa97
Compare