bgpd: cleanup for path_info extra labels #15307
Conversation
louis-6wind
force-pushed
the
cleanup-extra
branch
2 times, most recently
from
07a3201 to
90d0085
Compare
|
ci:rerun |
louis-6wind
force-pushed
the
cleanup-extra
branch
from
90d0085 to
2b2a40d
Compare
|
ci:rerun |
louis-6wind
force-pushed
the
cleanup-extra
branch
2 times, most recently
from
f00b48f to
542206d
Compare
|
I've just added the reason for the rework in the first commit log as requested in the last meeting |
louis-6wind
force-pushed
the
cleanup-extra
branch
from
542206d to
8e8ee97
Compare
bgpd/bgp_label.h
Outdated
|
|
||
| static inline bool bgp_path_has_valid_label(struct bgp_path_info *path) | ||
| { | ||
| return bgp_path_has_valid_label_const( |
There was a problem hiding this comment.
Why do we need two functions const vs. non-const?
There was a problem hiding this comment.
Only one const function now: bgp_path_has_valid_label()
louis-6wind
force-pushed
the
cleanup-extra
branch
from
8e8ee97 to
ad9fe87
Compare
|
@donaldsharp is concerned about the information passed to route map processing; need to think about this more before merging |
I think we can do the cleanup rework even we don't move labels from extra to attr. There is definitively something wrong in the way label[] is accessed. |
|
To write down my problem: Currently FRR bgp sends updates to peers where we track the attribute + the list of prefixes that use that attribute. This allows FRR to pack data via nlri very efficiently to peers. You can get 100's-1000's of prefixes per bgp update packet because of this. With this change it's possible to have a label per prefix, thus generating a attribute per prefix. BGP memory allocation would take a huge hit and in addition packing data to the peers would take a huge hit as well. I am not comfortable with this approach of moving the label information into the attribute. |
I agree with you @donaldsharp. The alternative approach to solve the following issues would be the solution from #15205
#15205 duplicates the labels to bgp_adj_in and bgp_adj_out structures. A better approach would be to reference the extra pointer in bgp_adj_in and bgp_adj_out structures. What do you think of that ? If labels are kept in extra, it seems the cleanup of this PR is still needed. |
louis-6wind
force-pushed
the
cleanup-extra
branch
from
ad9fe87 to
b5d0ae6
Compare
louis-6wind
changed the title
The handling of MPLS labels in BGP faces an issue due to the way labels
are stored in memory. They are stored in bgp_path_info but not in
bgp_adj_in and bgp_adj_out structures. As a consequence, some
configuration changes result in losing labels or even a bgpd crash. For
example, when retrieving routes from the Adj-RIB-in table
("soft-reconfiguration inbound" enabled), labels are missing.
bgp_path_info stores the MPLS labels, as shown below:
> struct bgp_path_info {
> struct bgp_path_info_extra *extra;
> [...]
> struct bgp_path_info_extra {
> mpls_label_t label[BGP_MAX_LABELS];
> uint32_t num_labels;
> [...]
To solve those issues, a solution would be to set label data to the
bgp_adj_in and bgp_adj_out structures in addition to the
bgp_path_info_extra structure. The idea is to reference a common label
pointer in all these three structures. And to store the data in a hash
list in order to save memory.
However, an issue in the code prevents us from setting clean data
without a rework. The extra->num_labels field, which is intended to
indicate the number of labels in extra->label[], is not reliably checked
or set. The code often incorrectly assumes that if the extra pointer is
present, then a label must also be present, leading to direct access to
extra->label[] without verifying extra->num_labels. This assumption
usually works because extra->label[0] is set to MPLS_INVALID_LABEL when
a new bgp_path_info_extra is created, but it is technically incorrect.
Cleanup the label code by setting num_labels each time values are set in
extra->label[] and checking extra->num_labels before accessing the
labels.
Signed-off-by: Louis Scalbert <[email protected]>
Add bgp_path_has_valid_label to check that a path_info has a valid label. Signed-off-by: Louis Scalbert <[email protected]>
No need to init labels at extra allocation. num_labels is the number of set labels in label[] and is initialized to 0 by default. Signed-off-by: Louis Scalbert <[email protected]>
In bgp_update(), path_info *new has just been created and has void labels. bgp_labels_same() is always false. Do not compare previous labels before setting them. Signed-off-by: Louis Scalbert <[email protected]>
Add bgp_path_info_labels_same() to compare labels with labels from path_info. Remove labels_same() that was used for mplsvpn only. Signed-off-by: Louis Scalbert <[email protected]>
num_labels cannot be greater than BGP_MAX_LABELS by design. Remove the check and the override. Signed-off-by: Louis Scalbert <[email protected]>
In route_vty_out_detail(), tag_buf stores a string representation of the VNI label. Rename tag_buf to vni_buf for clarity and rework the code a little bit to prepare the following commits. Signed-off-by: Louis Scalbert <[email protected]>
Add bgp_path_info_num_labels() to get the number of labels stored in a path_info structure. Signed-off-by: Louis Scalbert <[email protected]>
8 bits are sufficient to store the number of labels because the current maximum is 2. Signed-off-by: Louis Scalbert <[email protected]>
louis-6wind
force-pushed
the
cleanup-extra
branch
from
b5d0ae6 to
863bd28
Compare
|
@donaldsharp In the new #15434 pull-request, the approach is now to store labels in a new bgp_labels structure. Reference bgp_labels pointer in |
Should we close this one? |
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
|
replaced by #15434 |
The handling of MPLS labels in BGP faces an issue due to the way labels
are stored in memory. They are stored in bgp_path_info but not in
bgp_adj_in and bgp_adj_out structures. As a consequence, some
configuration changes result in losing labels or even a bgpd crash. For
example, when retrieving routes from the Adj-RIB-in table
("soft-reconfiguration inbound" enabled), labels are missing.
bgp_path_info stores the MPLS labels, as shown below:
To solve those issues, a solution would be to set label data to the
bgp_adj_in and bgp_adj_out structures in addition to the
bgp_path_info_extra structure. The idea is to reference a common label
pointer in all these three structures. And to store the data in a hash
list in order to save memory.
However, an issue in the code prevents us from setting clean data
without a rework. The extra->num_labels field, which is intended to
indicate the number of labels in extra->label[], is not reliably checked
or set. The code often incorrectly assumes that if the extra pointer is
present, then a label must also be present, leading to direct access to
extra->label[] without verifying extra->num_labels. This assumption
usually works because extra->label[0] is set to MPLS_INVALID_LABEL when
a new bgp_path_info_extra is created, but it is technically incorrect.
Cleanup the label code by setting num_labels each time values are set in
extra->label[] and checking extra->num_labels before accessing the
labels.
Add some functions to factorize frequent patterns in the code and do some
small rework.