zebra: reduce memory usage by streams when redistributing routes #18030
Conversation
fdumontet6WIND
force-pushed
the
mem_alloc_stream
branch
2 times, most recently
from
7abd16d to
eaf1718
Compare
There was a problem hiding this comment.
I'm going to repeat some of the things I said in the previous version of this.
- encoding zapi is really something that happens... in the zclient library, and it's not really correct to force a bunch of detailed assumptions into zebra (or any other daemon) code.
- consider a single, authoritative "compute_size" function, that accounts for the fixed parts of the zapi_route struct, and the embedded nexthops.
zebra/zapi_msg.c
Outdated
| @@ -1526,6 +1544,7 @@ static void zread_interface_add(ZAPI_HANDLER_ARGS) | |||
| struct interface *ifp; | |||
|
|
|||
| vrf_id_t vrf_id = zvrf_id(zvrf); | |||
|
|
|||
|
please fix the style errors - you can run that tool yourself, in your own sandbox, and save the CI re-runs... |
fdumontet6WIND
force-pushed
the
mem_alloc_stream
branch
from
eaf1718 to
5099bd6
Compare
|
Dear Mark, |
|
Look, opening multiple versions of the same PR is really not encouraged. We would prefer you use the original PR because that one has comments on it that now we cannot reference. Now we are in a state where there are comments on both of the PR's that we need to make sure it is addressed and frankly it's not very cool to do that. |
|
this is due to a bad synchronization with my github repos. commit was erazed, and new pulled request was triggered. sorry, this was not my intention. |
fdumontet6WIND
force-pushed
the
mem_alloc_stream
branch
5 times, most recently
from
cdbf34f to
5f14149
Compare
|
ci:rerun |
|
ci:rerun error not related to that PR |
There was a problem hiding this comment.
this is really very brittle - it's not really an improvement to have a magic number in lib/zclient.c, completely disconnected from the actual struct. I understand that you don't like the current, very conservative buffer size, but the risk in this approach outweighs any benefit, imo.
|
Mark, |
|
itt's not a magic number, elements taken into account are explicit-ed in code comment. |
fdumontet6WIND
force-pushed
the
mem_alloc_stream
branch
from
5f14149 to
75d5fb4
Compare
|
ci:rerun |
required stream size is evaluated as a fix part and variable one. the variable one depend on the number of nexthops. Signed-off-by: Francois Dumontet <[email protected]>
fdumontet6WIND
force-pushed
the
mem_alloc_stream
branch
from
75d5fb4 to
1c37834
Compare
| @@ -619,6 +619,8 @@ int zsend_redistribute_route(int cmd, struct zserv *client, const struct route_n | |||
| return -1; | |||
| } | |||
|
|
|||
| assert(stream_get_endp(s) <= stream_size); | |||
There was a problem hiding this comment.
Do you think this can happen? I don't think the stream 's' will allow itself to overrrun...
| * stream_putw_at(s, 0, stream_get_endp(s)); | ||
| * stream_size += 2; | ||
| */ | ||
| return 74 + (34 * api->nexthop_num); |
There was a problem hiding this comment.
these are the magic numbers I was referring to - if you want to make a change, it cannot be this change.
if you want to compute a size dynamically, try computing using:
a) the size of the entire struct
b) the number of nexthops in -use
c) the number of backup nexthops in-use
d) whether the opaque data is being included
(a) includes the max number of nexthops, and it includes the "opaque" field: try using some arithmetic to subtract from (a) to find a reasonable size. this will allow (a) to change in several ways and allow the computation to remain valid.
required stream size is evaluated as a fix part and variable one. the variable one depend on the number of nexthops.