Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Fri, 28 Sep 2018 11:08:29 +0100
From:   Roman Gushchin <guro@fb.com>
To:     Alexei Starovoitov <alexei.starovoitov@gmail.com>
CC:     <netdev@vger.kernel.org>, Song Liu <songliubraving@fb.com>,
        <linux-kernel@vger.kernel.org>, <kernel-team@fb.com>,
        Daniel Borkmann <daniel@iogearbox.net>,
        Alexei Starovoitov <ast@kernel.org>
Subject: Re: [PATCH v3 bpf-next 10/10] selftests/bpf: cgroup local
 storage-based network counters
Message-ID: <20180928100817.GC9018@castle.DHCP.thefacebook.com>
References: <20180926113326.29069-1-guro@fb.com>
 <20180926113326.29069-11-guro@fb.com>
 <20180928085356.56xe7javtd6cdfz6@ast-mbp.dhcp.thefacebook.com>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20180928085356.56xe7javtd6cdfz6@ast-mbp.dhcp.thefacebook.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
Received-SPF: None (protection.outlook.com: fb.com does not designate
 permitted sender hosts)
SpamDiagnosticOutput: 1:99
SpamDiagnosticMetadata: NSPM
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 28 Sep 2018 10:08:48.5226 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: 8f18869b-9977-4362-2dc1-08d6252a6382
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 8ae927fe-1255-47a7-a2af-5f3a069daaa2
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SN1PR15MB0175
X-OriginatorOrg: fb.com
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:,, definitions=2018-09-28_04:,,
 signatures=0
X-Proofpoint-Spam-Reason: safe
X-FB-Internal: Safe
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Fri, Sep 28, 2018 at 10:53:58AM +0200, Alexei Starovoitov wrote:
> On Wed, Sep 26, 2018 at 12:33:26PM +0100, Roman Gushchin wrote:
> > This commit adds a bpf kselftest, which demonstrates how percpu
> > and shared cgroup local storage can be used for efficient lookup-free
> > network accounting.
> > 
> > Cgroup local storage provides generic memory area with a very efficient
> > lookup free access. To avoid expensive atomic operations for each
> > packet, per-cpu cgroup local storage is used. Each packet is initially
> > charged to a per-cpu counter, and only if the counter reaches certain
> > value (32 in this case), the charge is moved into the global atomic
> > counter. This allows to amortize atomic operations, keeping reasonable
> > accuracy.
> > 
> > The test also implements a naive network traffic throttling, mostly to
> > demonstrate the possibility of bpf cgroup--based network bandwidth
> > control.
> > 
> > Expected output:
> >   ./test_netcnt
> >   test_netcnt:PASS
> > 
> > Signed-off-by: Roman Gushchin <guro@fb.com>
> > Acked-by: Song Liu <songliubraving@fb.com>
> > Cc: Daniel Borkmann <daniel@iogearbox.net>
> > Cc: Alexei Starovoitov <ast@kernel.org>
> > ---
> >  tools/testing/selftests/bpf/Makefile        |   6 +-
> >  tools/testing/selftests/bpf/netcnt_common.h |  23 +++
> >  tools/testing/selftests/bpf/netcnt_prog.c   |  71 +++++++++
> >  tools/testing/selftests/bpf/test_netcnt.c   | 153 ++++++++++++++++++++
> >  4 files changed, 251 insertions(+), 2 deletions(-)
> >  create mode 100644 tools/testing/selftests/bpf/netcnt_common.h
> >  create mode 100644 tools/testing/selftests/bpf/netcnt_prog.c
> >  create mode 100644 tools/testing/selftests/bpf/test_netcnt.c
> > 
> > diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> > index fd3851d5c079..5443399dd3a1 100644
> > --- a/tools/testing/selftests/bpf/Makefile
> > +++ b/tools/testing/selftests/bpf/Makefile
> > @@ -23,7 +23,8 @@ $(TEST_CUSTOM_PROGS): $(OUTPUT)/%: %.c
> >  TEST_GEN_PROGS = test_verifier test_tag test_maps test_lru_map test_lpm_map test_progs \
> >  	test_align test_verifier_log test_dev_cgroup test_tcpbpf_user \
> >  	test_sock test_btf test_sockmap test_lirc_mode2_user get_cgroup_id_user \
> > -	test_socket_cookie test_cgroup_storage test_select_reuseport
> > +	test_socket_cookie test_cgroup_storage test_select_reuseport \
> > +	test_netcnt
> >  
> >  TEST_GEN_FILES = test_pkt_access.o test_xdp.o test_l4lb.o test_tcp_estats.o test_obj_id.o \
> >  	test_pkt_md_access.o test_xdp_redirect.o test_xdp_meta.o sockmap_parse_prog.o     \
> > @@ -35,7 +36,7 @@ TEST_GEN_FILES = test_pkt_access.o test_xdp.o test_l4lb.o test_tcp_estats.o test
> >  	test_get_stack_rawtp.o test_sockmap_kern.o test_sockhash_kern.o \
> >  	test_lwt_seg6local.o sendmsg4_prog.o sendmsg6_prog.o test_lirc_mode2_kern.o \
> >  	get_cgroup_id_kern.o socket_cookie_prog.o test_select_reuseport_kern.o \
> > -	test_skb_cgroup_id_kern.o bpf_flow.o
> > +	test_skb_cgroup_id_kern.o bpf_flow.o netcnt_prog.o
> >  
> >  # Order correspond to 'make run_tests' order
> >  TEST_PROGS := test_kmod.sh \
> > @@ -72,6 +73,7 @@ $(OUTPUT)/test_tcpbpf_user: cgroup_helpers.c
> >  $(OUTPUT)/test_progs: trace_helpers.c
> >  $(OUTPUT)/get_cgroup_id_user: cgroup_helpers.c
> >  $(OUTPUT)/test_cgroup_storage: cgroup_helpers.c
> > +$(OUTPUT)/test_netcnt: cgroup_helpers.c
> >  
> >  .PHONY: force
> >  
> > diff --git a/tools/testing/selftests/bpf/netcnt_common.h b/tools/testing/selftests/bpf/netcnt_common.h
> > new file mode 100644
> > index 000000000000..0e10fc276c2a
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/netcnt_common.h
> > @@ -0,0 +1,23 @@
> > +#ifndef __NETCNT_COMMON_H
> > +#define __NETCNT_COMMON_H
> > +
> > +#include <linux/types.h>
> > +
> > +#define MAX_PERCPU_PACKETS 32
> > +
> > +struct percpu_net_cnt {
> > +	__u64 packets;
> > +	__u64 bytes;
> > +
> > +	__u64 prev_ts;
> > +
> > +	__u64 prev_packets;
> > +	__u64 prev_bytes;
> > +};
> > +
> > +struct net_cnt {
> > +	__u64 packets;
> > +	__u64 bytes;
> > +};
> > +
> > +#endif
> > diff --git a/tools/testing/selftests/bpf/netcnt_prog.c b/tools/testing/selftests/bpf/netcnt_prog.c
> > new file mode 100644
> > index 000000000000..1198abca1360
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/netcnt_prog.c
> > @@ -0,0 +1,71 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +#include <linux/bpf.h>
> > +#include <linux/version.h>
> > +
> > +#include "bpf_helpers.h"
> > +#include "netcnt_common.h"
> > +
> > +#define MAX_BPS	(3 * 1024 * 1024)
> > +
> > +#define REFRESH_TIME_NS	100000000
> > +#define NS_PER_SEC	1000000000
> > +
> > +struct bpf_map_def SEC("maps") percpu_netcnt = {
> > +	.type = BPF_MAP_TYPE_PERCPU_CGROUP_STORAGE,
> > +	.key_size = sizeof(struct bpf_cgroup_storage_key),
> > +	.value_size = sizeof(struct percpu_net_cnt),
> > +};
> > +
> > +struct bpf_map_def SEC("maps") netcnt = {
> > +	.type = BPF_MAP_TYPE_CGROUP_STORAGE,
> > +	.key_size = sizeof(struct bpf_cgroup_storage_key),
> > +	.value_size = sizeof(struct net_cnt),
> > +};
> > +
> > +SEC("cgroup/skb")
> > +int bpf_nextcnt(struct __sk_buff *skb)
> > +{
> > +	struct percpu_net_cnt *percpu_cnt;
> > +	char fmt[] = "%d %llu %llu\n";
> > +	struct net_cnt *cnt;
> > +	__u64 ts, dt;
> > +	int ret;
> > +
> > +	cnt = bpf_get_local_storage(&netcnt, 0);
> > +	percpu_cnt = bpf_get_local_storage(&percpu_netcnt, 0);
> > +
> > +	percpu_cnt->packets++;
> > +	percpu_cnt->bytes += skb->len;
> > +
> > +	if (percpu_cnt->packets > MAX_PERCPU_PACKETS) {
> > +		__sync_fetch_and_add(&cnt->packets,
> > +				     percpu_cnt->packets);
> > +		percpu_cnt->packets = 0;
> > +
> > +		__sync_fetch_and_add(&cnt->bytes,
> > +				     percpu_cnt->bytes);
> > +		percpu_cnt->bytes = 0;
> > +	}
> > +
> > +	ts = bpf_ktime_get_ns();
> > +	dt = ts - percpu_cnt->prev_ts;
> > +
> > +	dt *= MAX_BPS;
> > +	dt /= NS_PER_SEC;
> > +
> > +	if (cnt->bytes + percpu_cnt->bytes - percpu_cnt->prev_bytes < dt)
> > +		ret = 1;
> > +	else
> > +		ret = 0;
> > +
> > +	if (dt > REFRESH_TIME_NS) {
> > +		percpu_cnt->prev_ts = ts;
> > +		percpu_cnt->prev_packets = cnt->packets;
> > +		percpu_cnt->prev_bytes = cnt->bytes;
> > +	}
> > +
> > +	return !!ret;
> > +}
> > +
> > +char _license[] SEC("license") = "GPL";
> > +__u32 _version SEC("version") = LINUX_VERSION_CODE;
> > diff --git a/tools/testing/selftests/bpf/test_netcnt.c b/tools/testing/selftests/bpf/test_netcnt.c
> > new file mode 100644
> > index 000000000000..aa424f8db466
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/test_netcnt.c
> > @@ -0,0 +1,153 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +#include <stdio.h>
> > +#include <stdlib.h>
> > +#include <string.h>
> > +#include <errno.h>
> > +#include <assert.h>
> > +#include <sys/sysinfo.h>
> > +#include <sys/time.h>
> > +
> > +#include <linux/bpf.h>
> > +#include <bpf/bpf.h>
> > +#include <bpf/libbpf.h>
> > +
> > +#include "cgroup_helpers.h"
> > +#include "bpf_rlimit.h"
> > +#include "netcnt_common.h"
> > +
> > +#define BPF_PROG "./netcnt_prog.o"
> > +#define TEST_CGROUP "/test-network-counters/"
> > +
> > +static int bpf_find_map(const char *test, struct bpf_object *obj,
> > +			const char *name)
> > +{
> > +	struct bpf_map *map;
> > +
> > +	map = bpf_object__find_map_by_name(obj, name);
> > +	if (!map) {
> > +		printf("%s:FAIL:map '%s' not found\n", test, name);
> > +		return -1;
> > +	}
> > +	return bpf_map__fd(map);
> > +}
> > +
> > +int main(int argc, char **argv)
> > +{
> > +	struct percpu_net_cnt *percpu_netcnt;
> > +	struct bpf_cgroup_storage_key key;
> > +	int map_fd, percpu_map_fd;
> > +	int error = EXIT_FAILURE;
> > +	struct net_cnt netcnt;
> > +	struct bpf_object *obj;
> > +	int prog_fd, cgroup_fd;
> > +	unsigned long packets;
> > +	int cpu, nproc;
> > +	__u32 prog_cnt;
> > +
> > +	nproc = get_nprocs_conf();
> > +	percpu_netcnt = malloc(sizeof(*percpu_netcnt) * nproc);
> > +	if (!percpu_netcnt) {
> > +		printf("Not enough memory for per-cpu area (%d cpus)\n", nproc);
> > +		goto err;
> > +	}
> > +
> > +	if (bpf_prog_load(BPF_PROG, BPF_PROG_TYPE_CGROUP_SKB,
> > +			  &obj, &prog_fd)) {
> > +		printf("Failed to load bpf program\n");
> > +		goto out;
> > +	}
> > +
> > +	if (setup_cgroup_environment()) {
> > +		printf("Failed to load bpf program\n");
> > +		goto err;
> > +	}
> > +
> > +	/* Create a cgroup, get fd, and join it */
> > +	cgroup_fd = create_and_get_cgroup(TEST_CGROUP);
> > +	if (!cgroup_fd) {
> > +		printf("Failed to create test cgroup\n");
> > +		goto err;
> > +	}
> > +
> > +	if (join_cgroup(TEST_CGROUP)) {
> > +		printf("Failed to join cgroup\n");
> > +		goto err;
> > +	}
> > +
> > +	/* Attach bpf program */
> > +	if (bpf_prog_attach(prog_fd, cgroup_fd, BPF_CGROUP_INET_EGRESS, 0)) {
> > +		printf("Failed to attach bpf program");
> > +		goto err;
> > +	}
> > +
> > +	assert(system("ping localhost -s 500 -c 10000 -f -q > /dev/null") == 0);
> > +
> > +	if (bpf_prog_query(cgroup_fd, BPF_CGROUP_INET_EGRESS, 0, NULL, NULL,
> > +			   &prog_cnt)) {
> > +		printf("Failed to query attached programs");
> > +		goto err;
> > +	}
> > +
> > +	map_fd = bpf_find_map(__func__, obj, "netcnt");
> > +	if (map_fd < 0) {
> > +		printf("Failed to find bpf map with net counters");
> > +		goto err;
> > +	}
> > +
> > +	percpu_map_fd = bpf_find_map(__func__, obj, "percpu_netcnt");
> > +	if (percpu_map_fd < 0) {
> > +		printf("Failed to find bpf map with percpu net counters");
> > +		goto err;
> > +	}
> > +
> > +	if (bpf_map_get_next_key(map_fd, NULL, &key)) {
> > +		printf("Failed to get key in cgroup storage\n");
> > +		goto err;
> > +	}
> > +
> > +	if (bpf_map_lookup_elem(map_fd, &key, &netcnt)) {
> > +		printf("Failed to lookup cgroup storage\n");
> > +		goto err;
> > +	}
> > +
> > +	if (bpf_map_lookup_elem(percpu_map_fd, &key, &percpu_netcnt[0])) {
> > +		printf("Failed to lookup percpu cgroup storage\n");
> > +		goto err;
> > +	}
> > +
> > +	/* Some packets can be still in per-cpu cache, but not more than
> > +	 * MAX_PERCPU_PACKETS.
> > +	 */
> > +	packets = netcnt.packets;
> > +	for (cpu = 0; cpu < nproc; cpu++) {
> > +		if (percpu_netcnt[cpu].packets > 32) {
> 
> pls use MAX_PERCPU_PACKETS in the above check.
> could you also double check that if that #define is changed to 1k or so
> the exact "!= 10000" check below still works as expected?

Do you mean adding a new test with a different MAX_PERCPU_PACKETS?

> 
> > +			printf("Unexpected percpu value: %llu\n",
> > +			       percpu_netcnt[cpu].packets);
> > +			goto err;
> 
> > +		}
> > +
> > +		packets += percpu_netcnt[cpu].packets;
> > +	}
> > +
> > +	/* No packets should be lost */
> > +	if (packets != 10000) {
> > +		printf("Unexpected packet count: %lu\n", packets);
> > +		goto err;
> > +	}
> > +
> > +	/* Let's check that bytes counter value is reasonable */
> > +	if (netcnt.bytes < packets * 500 || netcnt.bytes > packets * 1500) {
> 
> since packet count is accurate why byte count would vary ?

Tbh I'm not sure if the size of the packet here can vary depending
on the environment. Is there a nice way to get the expected size?