'show bgp detail' memory improvement

[pguibert6WIND]

After having loaded a full route, the memory consumption still increases after executing the 'show bgp ipv4 json detail' command. The memory consumed is related to json objects stored for BGP as paths, standard and large communities.

The below pull request proposes to incrementally display each path detailed information, without relying on the json storage.

Memory consumed after BGP routes have been loaded:

dut-orwell-nianticvf# show ip route summary
Route Source         Routes               FIB  (vrf default)
kernel               1                    1
connected            2                    2
ebgp                 993276               992665
ibgp                 0                    0
------
Totals               993279               992668

root@dut-orwell-nianticvf:~# ps -aux | grep bgpd
root      106598 84.0 10.2 1224244 1044028 ?     Ssl  15:54   1:46 /usr/bin/bgpd -A 127.0.0.1 -M snmp -M rpki -M bmp

Memory consumed after the 'show bgp ipv4 json detail' command is executed, without fix:

root@dut-orwell-nianticvf:~# time vtysh -c "show bgp ipv4 detail json" > /tmp/aa.txt

real    0m25.336s
user    0m2.438s
sys     0m4.745s
root@dut-orwell-nianticvf:~# ps -aux | grep bgpd
root       16770 59.0 18.6 2080984 1900816 ?     Ssl  09:13   2:03 /usr/bin/bgpd -A 127.0.0.1 -M snmp -M rpki -M bmp

after fix:

root@dut-orwell-nianticvf:~# time vtysh -c "show bgp ipv4 detail json" > /tmp/aa.txt

real    0m33.106s
user    0m2.594s
sys     0m4.533s
root@dut-sureau-nianticvf:~# ps -aux | grep bgpd
root       45983 45.4 13.6 1567712 1387988 ?     Ssl  11:24   2:10 /usr/bin/bgpd -A 127.0.0.1 -M snmp -M rpki -M bmp

[ton31337]

I'm a bit confused, is this an issue of the JSON library or our implementation?

[pguibert6WIND]

I'm a bit confused, is this an issue of the JSON library or our implementation?

Json structs are locally allocated at show time in BGP as paths. This makes increase of memory used, whereas we already struggle with memory with BGP handling million of routes.

I had the possibility to allocate json at startup. In this way, even before show time, we would have reached a memory size that would not move, event after.
But I did not want that, because it takes a lot of memory ( 113K json AS paths structs if I remember well).

So I decided to not store json.
Then I had two options:
a- either allocate and free json objects after the show command.
b- or use vty, and avoid minimize the usage of json objects

I took the choice of using b).
allocating and freeing variable size objects is always a stress for Linux processed, and I did not want to increase the VMSIZE, only because of json.

[mjstapp]

the "before" and "after" examples in the description: where on master were those taken? did both include the "incremental vtysh" change that we merged recently? I'm wondering whether there's something we might do to extend that concept for large json output, if necessary.

[mjstapp]

Just at the top level, I'm not keen to try to manually do json encoding inside bgpd. I'd prefer to see something incremental, for example, that still used the common library in some more efficient way.

[pguibert6WIND]

the "before" and "after" examples in the description: where on master were those taken? did both include the "incremental vtysh" change that we merged recently? I'm wondering whether there's something we might do to extend that concept for large json output, if necessary.

Measures have been done on the top of master (with spike commit + json incremental fixes partial for BGP).

[pguibert6WIND]

Just at the top level, I'm not keen to try to manually do json encoding inside bgpd. I'd prefer to see something incremental, for example, that still used the common library in some more efficient way.

json_object_set_serializer() was an API that could help to implement our output for each kind of json object. For instance, this could help by allocating enough memory and storing the output in the buffer. But the result was the same, I was allocating a lot of memory. Alternatively, I did not find how to pass vty pointer in those functions.
I tried a second method by parsing the json objects by myself. But this kind of work needs to be done in the json-c library , not outside, because I access private structures.

I think that on a long term basis, all this code will be replaced by a real state model defined in YANG. But today, we do not have this in BGP.

[github-actions]

This pull request has conflicts, please resolve those before we can evaluate the pull request.

[mjstapp]

Have you looked at the json-c library? it looks like it would be possible to do our own string output walk through a tree of json objects, keep track of any container objects that have been partially printed, and free inner objects once they've been printed. I'll bet that would a) take fewer lines of change, and b) be reusable generally.

[pguibert6WIND]

Have you looked at the json-c library? it looks like it would be possible to do our own string output walk through a tree of json objects, keep track of any container objects that have been partially printed, and free inner objects once they've been printed. I'll bet that would a) take fewer lines of change, and b) be reusable generally.

The main problem is we store json objects in BGP aspath/communities/large communities structure, and it is too expensive.
At least, I think we should get rid of this pre-allocation.

[pguibert6WIND]

Have you looked at the json-c library? it looks like it would be possible to do our own string output walk through a tree of json objects, keep track of any container objects that have been partially printed, and free inner objects once they've been printed. I'll bet that would a) take fewer lines of change, and b) be reusable generally.

The main problem is we store json objects in BGP aspath/communities/large communities structure, and it is too expensive. At least, I think we should get rid of this pre-allocation.

I made a branch with using extended API.
https://github.com/pguibert6WIND/frr/tree/bgp_detail_no_json_alloc_draft

With a custom release using serialized code for aspath only (community and large community code has been removed) we demonstrate that the memory taken is similar to before the 'show bgp ipv4 json detail' command.

before the show

root@dut-sureau-nianticvf:~# ps -aux | grep bgpd
root       11213 60.0 10.2 1224168 1044272 ?     Ssl  09:52   1:42 /usr/bin/bgpd -A 127.0.0.1 -M snmp -M rpki -M bmp

during the show

root@dut-sureau-nianticvf:~# time vtysh -c "show bgp ipv4 json detail" > /tmp/aa.txt  
real    0m26.592s
user    0m2.384s
sys     0m5.100s

after the show

root@dut-sureau-nianticvf:~# ps -aux | grep bgpd
root       11213 39.7 10.3 1234176 1054420 ?     Ssl  09:52   2:05 /usr/bin/bgpd -A 127.0.0.1 -M snmp -M rpki -M bmp

[pguibert6WIND]

The global memory used before the show

System allocator statistics:
  Total heap allocated:  980 MiB
  Holding block headers: 24 MiB
  Used small blocks:     0 bytes
  Used ordinary blocks:  979 MiB
  Free small blocks:     336 KiB
  Free ordinary blocks:  1877 KiB
  Ordinary blocks:       6483
  Small blocks:          2794
  Holding blocks:        22

after the show

System allocator statistics:
  Total heap allocated:  1316 MiB
  Holding block headers: 24 MiB
  Used small blocks:     0 bytes
  Used ordinary blocks:  979 MiB
  Free small blocks:     336 KiB
  Free ordinary blocks:  337 MiB
  Ordinary blocks:       6522
  Small blocks:          2798
  Holding blocks:        22

[pguibert6WIND]

Have you looked at the json-c library? it looks like it would be possible to do our own string output walk through a tree of json objects, keep track of any container objects that have been partially printed, and free inner objects once they've been printed. I'll bet that would a) take fewer lines of change, and b) be reusable generally.

@mjstapp , please review
The implementation uses json-c API to create our own string.
Solution is not perfect.

• VMsize went from 1224244 to 1567712 instead of 2080984.
• and the time to produce json is increased of a few seconds. (from 25 seconds to 33 seconds).

[pguibert6WIND]

closing this pull request for clarity. please go to:
#16880