| Message ID | 20221202173147.3032702-7-odivlad@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | OVN IC bugfixes & proposals/questions | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_ovn-kubernetes | success | github build: passed |
| ovsrobot/github-robot-_Build_and_Test | fail | github build: failed |
On 12/2/22 18:31, Vladislav Odintsov wrote: > This patch is intended to show that currently it's possible to build > ECMP group of 65k buckets. > > Signed-off-by: Vladislav Odintsov <odivlad@gmail.com> > --- > northd/northd.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/northd/northd.c b/northd/northd.c > index e1f3bace8..f8f7977ae 100644 > --- a/northd/northd.c > +++ b/northd/northd.c > @@ -9271,7 +9271,7 @@ static void > ecmp_groups_add_route(struct ecmp_groups_node *group, > const struct parsed_route *route) > { > - if (group->route_count == UINT16_MAX) { > + if (group->route_count == 1024) { > static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1); > VLOG_WARN_RL(&rl, "too many routes in a single ecmp group."); > return; Should we make the limit configurable? What if the CMS wants to install a route with more than 1K paths? Not sure if that's realistic but I would avoid the hardcoded 1K. Thanks, Dumitru
It’s a good idea. But one thing is that this is not the only one place where the buckets are created. Also they’re created in LBs. Should we just put some common function, which returns the current configured (or default MAX) and use it in every place? Regards, Vladislav Odintsov > On 5 Dec 2022, at 19:37, Dumitru Ceara <dceara@redhat.com> wrote: > > On 12/2/22 18:31, Vladislav Odintsov wrote: >> This patch is intended to show that currently it's possible to build >> ECMP group of 65k buckets. >> >> Signed-off-by: Vladislav Odintsov <odivlad@gmail.com> >> --- >> northd/northd.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/northd/northd.c b/northd/northd.c >> index e1f3bace8..f8f7977ae 100644 >> --- a/northd/northd.c >> +++ b/northd/northd.c >> @@ -9271,7 +9271,7 @@ static void >> ecmp_groups_add_route(struct ecmp_groups_node *group, >> const struct parsed_route *route) >> { >> - if (group->route_count == UINT16_MAX) { >> + if (group->route_count == 1024) { >> static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1); >> VLOG_WARN_RL(&rl, "too many routes in a single ecmp group."); >> return; > > Should we make the limit configurable? What if the CMS wants to install > a route with more than 1K paths? Not sure if that's realistic but I > would avoid the hardcoded 1K. > > Thanks, > Dumitru
Saying more, the OVS register, which is used to store the bucket ID is a 16-bit reg8[16..31], which was introduced by Han. @Han, can you clarify if it was planned to have 2^16 ECMP paths within one group (route)? Or maybe this is not needed to have such a bit IDs space and we can give more space for the routes instead of paths? Regards, Vladislav Odintsov > On 5 Dec 2022, at 21:14, Vladislav Odintsov <odivlad@gmail.com> wrote: > > It’s a good idea. > But one thing is that this is not the only one place where the buckets are created. > Also they’re created in LBs. Should we just put some common function, which returns the current configured (or default MAX) and use it in every place? > > Regards, > Vladislav Odintsov > >> On 5 Dec 2022, at 19:37, Dumitru Ceara <dceara@redhat.com <mailto:dceara@redhat.com>> wrote: >> >> On 12/2/22 18:31, Vladislav Odintsov wrote: >>> This patch is intended to show that currently it's possible to build >>> ECMP group of 65k buckets. >>> >>> Signed-off-by: Vladislav Odintsov <odivlad@gmail.com <mailto:odivlad@gmail.com>> >>> --- >>> northd/northd.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/northd/northd.c b/northd/northd.c >>> index e1f3bace8..f8f7977ae 100644 >>> --- a/northd/northd.c >>> +++ b/northd/northd.c >>> @@ -9271,7 +9271,7 @@ static void >>> ecmp_groups_add_route(struct ecmp_groups_node *group, >>> const struct parsed_route *route) >>> { >>> - if (group->route_count == UINT16_MAX) { >>> + if (group->route_count == 1024) { >>> static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1); >>> VLOG_WARN_RL(&rl, "too many routes in a single ecmp group."); >>> return; >> >> Should we make the limit configurable? What if the CMS wants to install >> a route with more than 1K paths? Not sure if that's realistic but I >> would avoid the hardcoded 1K. >> >> Thanks, >> Dumitru >
diff --git a/northd/northd.c b/northd/northd.c index e1f3bace8..f8f7977ae 100644 --- a/northd/northd.c +++ b/northd/northd.c @@ -9271,7 +9271,7 @@ static void ecmp_groups_add_route(struct ecmp_groups_node *group, const struct parsed_route *route) { - if (group->route_count == UINT16_MAX) { + if (group->route_count == 1024) { static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1); VLOG_WARN_RL(&rl, "too many routes in a single ecmp group."); return;
This patch is intended to show that currently it's possible to build ECMP group of 65k buckets. Signed-off-by: Vladislav Odintsov <odivlad@gmail.com> --- northd/northd.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)