bpf: clean up IPv4 fragments support (and bpf/), add option for map size #10927

qmonnet · 2020-04-10T00:01:58Z

Support for IPv4 was recently introduced, but a few possible improvements were raised after the merge (see #10264). Let's fix them.

One in particular, the removal of the CONDITIONAL_PREALLOC flag for the fragment tracking map, is a minor bug fix (this is a LRU map which will not work without pre-allocating the space). Labelling PR as bug because of it, but other commits are not bugfixes so I'm unsure this is the thing to do.

Other fixes include:

Addition of an option to tune the size of LRU maps for fragments tracking
bpf/-wide cleanup for __attribute__((packed)) -> __packed
bpf/-wide cleanup for #if defined(foo)
Miscellaneous minor cleanup on fragments tracking code

Please refer to individual commit logs for details.

coveralls · 2020-04-10T00:38:24Z

Coverage decreased (-0.02%) to 46.623% when pulling b4f4773 on pr/qmonnet/fragments_cleanup into 5958dd6 on master.

Remove the CONDITIONAL_PREALLOC flag for the fragment tracking map: This is a LRU map which will not work without pre-allocating the space. Fixes: e65adc0 ("bpf: support IPv4 fragments for L4 load balancer key extraction") Signed-off-by: Quentin Monnet <quentin@isovalent.com>

Support for IPv4 was recently introduced, but a few possible improvements were raised after the merge (see #10264). Let's fix them. Signed-off-by: Quentin Monnet <quentin@isovalent.com>

Add an option to pass a size (number of entries) for the LRU map backing fragment tracking. Default to the current value, 8192. Signed-off-by: Quentin Monnet <quentin@isovalent.com>

The use of preprocessor conditions on definitions is not consistent throughout the bpf/ directory. Several forms are used: - #ifdef foo = #if defined foo = #if defined(foo) Harmonise it: - Use #ifdef if possible (only one check) - Use #if defined(foo) everywhere else Signed-off-by: Quentin Monnet <quentin@isovalent.com>

The former is easier on the eyes, let's switch all occurrences under the bpf/ directory (except for bpf/include/linux/) Signed-off-by: Quentin Monnet <quentin@isovalent.com>

joestringer · 2020-04-10T04:08:45Z

test-me-please

pchaigno

Thanks for the BPF-wide cleanups! I have a couple minor comments below.

pkg/maps/fragmap/fragmap.go

pkg/option/config.go

pchaigno · 2020-04-10T07:03:19Z

bpf/lib/ipv4.h

 struct bpf_elf_map __section_maps IPV4_FRAG_DATAGRAMS_MAP = {
 	.type           = BPF_MAP_TYPE_LRU_HASH,
 	.size_key	= sizeof(struct ipv4_frag_id),
 	.size_value	= sizeof(struct ipv4_frag_l4ports),
 	.pinning	= PIN_GLOBAL_NS,
 	.max_elem	= CILIUM_IPV4_FRAG_MAP_MAX_ENTRIES,
-	.flags		= CONDITIONAL_PREALLOC,


Could this also be the fix for #10925?

Seems likely. The -ENOTSUPP returned would be consistent.

qmonnet · 2020-04-10T12:06:11Z

@pchaigno All the nits you pointed out on the tunable option are because I based the changes on the existing option for policy map size. I'm unsure whether to stay close to the code of the existing option or to fix according to your suggestions.

(Thanks a lot for the review!)

borkmann · 2020-04-10T19:46:54Z

@pchaigno All the nits you pointed out on the tunable option are because I based the changes on the existing option for policy map size. I'm unsure whether to stay close to the code of the existing option or to fix according to your suggestions.

(Thanks a lot for the review!)

Thanks for the cleanups, look good. Agree with both of you. Could we get @pchaigno's feedback addressed but then also for the other options to make them consistent with each other? That would be great, thanks!

As a follow-up to IPv4 fragmentation cleanup (#10927), improve the way we handle configuration for the maximum number of entries in policy map and fragment-tracking map. Functional change: When the option passed by the user is outside of the limits used for policy map (upper/lower) or fragmap (upper), fall back to the limit and print a warning instead of stopping with an error. Move such "limits" to package "defaults" (We cannot move them to the respective map packages, as those cannot be imported from "config"). Remove unnecessary InitMapInfo() for fragmap. Keep it for policy map, because passing the size at map creation and removing this function would mean: - Updating the list of arguments passed to Open(), OpenAndCreate(), Create() and pass the size (option.Config.PolicyMapMaxEntries) each time, which seems verbose and not really useful. We cannot import the "option" package in "policy" to use the option just once in newMap(). - Making sure that policymap.MaxEntries is never called from other packages before the map is created. This is probably not the case so we do want to set the size before map creation. Signed-off-by: Quentin Monnet <quentin@isovalent.com>

As a follow-up to IPv4 fragmentation cleanup (#10927), clean-up configuration for the maximum number of entries in policy map and fragment-tracking map. Remove unnecessary InitMapInfo() for fragmap. Keep it for policy map, because passing the size at map creation and removing this function would mean: - Updating the list of arguments passed to Open(), OpenAndCreate(), Create() and pass the size (option.Config.PolicyMapEntries) each time, which seems verbose and not really useful. We cannot import the "option" package in "policy" to use the option just once in newMap(). - Making sure that policymap.MaxEntries is never called from other packages before the map is created. This is probably not the case so we do want to set the size before map creation. Initialise policymap.MaxEntries to defaults.PolicyMapEntries. Theoretically we don't have to initialise it because it is overwritten by InitMapInfo() before use, but the unit tests do not call InitMapInfo() and rely a non-zero value to successfully create a map (eppolicymap unit tests also use the value but does not care about map creation being successful). Signed-off-by: Quentin Monnet <quentin@isovalent.com>

qmonnet added kind/bug This is a bug in the Cilium logic. pending-review sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. release-note/bug This PR fixes an issue in a previous release of Cilium. labels Apr 10, 2020

qmonnet requested review from borkmann and a team April 10, 2020 00:01

qmonnet requested review from a team as code owners April 10, 2020 00:01

maintainer-s-little-helper bot added this to In progress in 1.8.0 Apr 10, 2020

qmonnet mentioned this pull request Apr 10, 2020

Support for IPv4 fragments #10264

Merged

qmonnet added wip and removed pending-review labels Apr 10, 2020

qmonnet added 2 commits April 10, 2020 02:23

bpf: clean up minor items related to IPv4 fragments support

3b5c303

Support for IPv4 was recently introduced, but a few possible improvements were raised after the merge (see #10264). Let's fix them. Signed-off-by: Quentin Monnet <quentin@isovalent.com>

qmonnet force-pushed the pr/qmonnet/fragments_cleanup branch from 72474b5 to 870333c Compare April 10, 2020 01:24

qmonnet requested a review from a team April 10, 2020 01:24

qmonnet changed the title ~~bpf: clean up minor items related to IPv4 fragments support~~ bpf: clean up IPv4 fragments support (and bpf/), add option for map size Apr 10, 2020

qmonnet added 3 commits April 10, 2020 02:29

pkg/maps/fragmap: add an option to tune the size of the BPF map

0772786

Add an option to pass a size (number of entries) for the LRU map backing fragment tracking. Default to the current value, 8192. Signed-off-by: Quentin Monnet <quentin@isovalent.com>

bpf: use __packed instead of __attribute__((packed))

b4f4773

The former is easier on the eyes, let's switch all occurrences under the bpf/ directory (except for bpf/include/linux/) Signed-off-by: Quentin Monnet <quentin@isovalent.com>

qmonnet force-pushed the pr/qmonnet/fragments_cleanup branch from 870333c to b4f4773 Compare April 10, 2020 01:30

qmonnet added pending-review and removed wip labels Apr 10, 2020

pchaigno requested changes Apr 10, 2020

View reviewed changes

borkmann merged commit 4e8be22 into master Apr 10, 2020

1.8.0 automation moved this from In progress to Merged Apr 10, 2020

borkmann deleted the pr/qmonnet/fragments_cleanup branch April 10, 2020 19:47

This was referenced Apr 14, 2020

policymap, fragmap: clean up doc, map size configuration #10964

Merged

"Map object 'cilium_ipv4_frag_datagrams' rejected: Unknown error 524 (524)! #10925

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bpf: clean up IPv4 fragments support (and bpf/), add option for map size #10927

bpf: clean up IPv4 fragments support (and bpf/), add option for map size #10927

qmonnet commented Apr 10, 2020 •

edited

coveralls commented Apr 10, 2020 •

edited

joestringer commented Apr 10, 2020

pchaigno left a comment

pchaigno Apr 10, 2020

qmonnet Apr 10, 2020

qmonnet commented Apr 10, 2020

borkmann commented Apr 10, 2020

bpf: clean up IPv4 fragments support (and bpf/), add option for map size #10927

bpf: clean up IPv4 fragments support (and bpf/), add option for map size #10927

Conversation

qmonnet commented Apr 10, 2020 • edited

coveralls commented Apr 10, 2020 • edited

joestringer commented Apr 10, 2020

pchaigno left a comment

Choose a reason for hiding this comment

pchaigno Apr 10, 2020

Choose a reason for hiding this comment

qmonnet Apr 10, 2020

Choose a reason for hiding this comment

qmonnet commented Apr 10, 2020

borkmann commented Apr 10, 2020

qmonnet commented Apr 10, 2020 •

edited

coveralls commented Apr 10, 2020 •

edited