Re: [PATCH bpf-next v4] bpf: increase supported cgroup storage value size

On Tue, Jul 27, 2021 at 4:25 PM [off-list ref] wrote:

On 07/27, Andrii Nakryiko wrote:

quoted

On Tue, Jul 27, 2021 at 3:23 PM Stanislav Fomichev [off-list ref] wrote:

quoted

Current max cgroup storage value size is 4k (PAGE_SIZE). The other local
storages accept up to 64k (BPF_LOCAL_STORAGE_MAX_VALUE_SIZE). Let's

align

quoted

max cgroup value size with the other storages.

For percpu, the max is 32k (PCPU_MIN_UNIT_SIZE) because percpu
allocator is not happy about larger values.

netcnt test is extended to exercise those maximum values
(non-percpu max size is close to, but not real max).

v4:
* remove inner union (Andrii Nakryiko)
* keep net_cnt on the stack (Andrii Nakryiko)

v3:
* refine SIZEOF_BPF_LOCAL_STORAGE_ELEM comment (Yonghong Song)
* anonymous struct in percpu_net_cnt & net_cnt (Yonghong Song)
* reorder free (Yonghong Song)

v2:
* cap max_value_size instead of BUILD_BUG_ON (Martin KaFai Lau)

Cc: Martin KaFai Lau <redacted>
Cc: Yonghong Song <redacted>
Cc: Andrii Nakryiko <andrii@kernel.org>
Signed-off-by: Stanislav Fomichev <redacted>
---

quoted

Added Martin's ack and applied to bpf-next. Please carry over received
Acks between revisions.

Ah, sorry, forgot about it :-(

quoted

It's also a good practice to separate selftest from the kernel (or
libbpf) changes, unless kernel change doesn't immediately break
selftest. Please consider doing that for the future.

I've actually seen some back and forth on this one. I used to split
them in the past (assuming it makes it easy to do the
backports/cherry-picks), but I remember at some point it was
suggested not to split them for small changes like this.

So we asked to split UAPI header sync for tools/include/ into a
separate patch initially. But then I just improved libbpf's sync
script to handle that regardless and we stopped asking for that. But
the libbpf vs kernel vs selftests split was always (perhaps
implicitly) advised. Personally, I've only had a few cases where
selftest changes had to go in with kernel changes in the same patch to
avoid breaking selftests. In all other cases it's cleaner to have them
split out.

Might be a good idea to document this (when and if to separate
libbpf/selftests)
on bpf_devel_QA.rst

quoted

I also just noticed that test_netcnt isn't part of test_progs. It
would be great to migrate it under the common test_progs
infrastructure. We've been steadily moving towards that, but there are
still a bunch of tests that are not run in CI.

SG, I might do a follow up on this one.

Sounds good, thanks!