From b1ce4ee69beffa9489b47355cfa4ab2d9a42e2b6 Mon Sep 17 00:00:00 2001 From: Simon Sundberg Date: Mon, 18 Jan 2021 18:08:35 +0100 Subject: [PATCH] pping: Format headers and fix TODO.md Format the header files in the Linux kernel style (missed in previous commit). Also fix a formating error in TODO.md that cause empty checkboxes to not display correctly. Signed-off-by: Simon Sundberg --- pping/TODO.md | 16 ++++---- pping/pping.h | 37 ++++++++--------- pping/pping_helpers.h | 95 +++++++++++++++++++++++-------------------- 3 files changed, 75 insertions(+), 73 deletions(-) diff --git a/pping/TODO.md b/pping/TODO.md index e75a233..b52543f 100644 --- a/pping/TODO.md +++ b/pping/TODO.md @@ -4,14 +4,14 @@ - [x] Clean up commits and add signed-off-by tags - [x] Add SPDX-license-identifier tags - [x] Format C-code in kernel style -- [] Use existing funcionality to reuse maps by using BTF-defined maps +- [ ] Use existing funcionality to reuse maps by using BTF-defined maps ## Future -- [] Use libxdp to load XDP program -- [] Cleanup: Unload TC-BPF at program shutdown, and unpin and delete map - In userspace part -- [] Add IPv6 support - In TC-BPF, XDP and userspace part -- [] Check for existance of reverse flow before adding to hash-map (to avoid adding timestamps for flows that we can't see the reverse traffic for) - In TC-BPF part +- [ ] Use libxdp to load XDP program +- [ ] Cleanup: Unload TC-BPF at program shutdown, and unpin and delete map - In userspace part +- [ ] Add IPv6 support - In TC-BPF, XDP and userspace part +- [ ] Check for existance of reverse flow before adding to hash-map (to avoid adding timestamps for flows that we can't see the reverse traffic for) - In TC-BPF part - This could miss the first few packets, would not be ideal for short flows -- [] Keep track of minimum RTT for each flow (done by Pollere's pping, and helps identify buffer bloat) - In XDP part -- [] Add configurable rate-limit for how often each flow can add entries to the map (prevent high-rate flows from quickly filling up the map) - In TCP-BPF part -- [] Improve map cleaning: Use a dynamic time to live for hash map entries based on flow's RTT, instead of static 10s limit - In TC-BPF, XDP and userspace +- [ ] Keep track of minimum RTT for each flow (done by Pollere's pping, and helps identify buffer bloat) - In XDP part +- [ ] Add configurable rate-limit for how often each flow can add entries to the map (prevent high-rate flows from quickly filling up the map) - In TCP-BPF part +- [ ] Improve map cleaning: Use a dynamic time to live for hash map entries based on flow's RTT, instead of static 10s limit - In TC-BPF, XDP and userspace diff --git a/pping/pping.h b/pping/pping.h index 31e0f29..c3feb47 100644 --- a/pping/pping.h +++ b/pping/pping.h @@ -3,33 +3,28 @@ #define TIMESTAMP_MAP_H #include -struct ipv4_flow -{ - __u32 saddr; - __u32 daddr; - __u16 sport; - __u16 dport; +struct ipv4_flow { + __u32 saddr; + __u32 daddr; + __u16 sport; + __u16 dport; }; -struct ts_key -{ - struct ipv4_flow flow; - __u32 tsval; +struct ts_key { + struct ipv4_flow flow; + __u32 tsval; }; -struct ts_timestamp -{ - __u64 timestamp; - //__u64 ttl; // Delete entry after ttl, allows more dynamic map cleaning where entries for flows with short RTTs can be removed earlier - __u8 used; - // __u8 pad[7]; // Need to pad it due to compiler optimization, see "Remove struct padding with aligning members by using #pragma pack." at https://docs.cilium.io/en/v1.9/bpf/ - +struct ts_timestamp { + __u64 timestamp; + //__u64 ttl; // Delete entry after ttl, allows more dynamic map cleaning where entries for flows with short RTTs can be removed earlier + __u8 used; + // __u8 pad[7]; // Need to pad it due to compiler optimization, see "Remove struct padding with aligning members by using #pragma pack." at https://docs.cilium.io/en/v1.9/bpf/ }; -struct rtt_event -{ - struct ipv4_flow flow; - __u64 rtt; +struct rtt_event { + struct ipv4_flow flow; + __u64 rtt; }; #endif diff --git a/pping/pping_helpers.h b/pping/pping_helpers.h index 1b6c8ab..528e8d8 100644 --- a/pping/pping_helpers.h +++ b/pping/pping_helpers.h @@ -5,55 +5,62 @@ #include "pping.h" #define MAX_TCP_OPTIONS 10 -static __always_inline int fill_ipv4_flow(struct ipv4_flow *flow, __u32 saddr, __u32 daddr, __u16 sport, __u16 dport) +static __always_inline int fill_ipv4_flow(struct ipv4_flow *flow, __u32 saddr, + __u32 daddr, __u16 sport, __u16 dport) { - flow->saddr = saddr; - flow->daddr = daddr; - flow->sport = sport; - flow->dport = dport; - return 0; + flow->saddr = saddr; + flow->daddr = daddr; + flow->sport = sport; + flow->dport = dport; + return 0; } - -// Parses the TSval and TSecr values from the TCP options field - returns 0 if sucessful and -1 on failure -// If sucessful the TSval and TSecr values will be stored at tsval and tsecr (in network byte order!) -static __always_inline int parse_tcp_ts(struct tcphdr *tcph, void *data_end, __u32 *tsval, __u32 *tsecr) +/* + * Parses the TSval and TSecr values from the TCP options field. If sucessful + * the TSval and TSecr values will be stored at tsval and tsecr (in network + * byte order). + * Returns 0 if sucessful and -1 on failure + */ +static __always_inline int parse_tcp_ts(struct tcphdr *tcph, void *data_end, + __u32 *tsval, __u32 *tsecr) { - if (tcph + 1 > data_end) // To hopefully please verifier - return -1; - int len = tcph->doff << 2; - if (len <= sizeof(struct tcphdr)) // No TCP options - return -1; - void *pos = (void *)(tcph + 1); - void *opt_end = ((void *)tcph + len); - __u8 i, opt, opt_size; - #pragma unroll - for (i = 0; i < MAX_TCP_OPTIONS; i++) { - if (pos+1 > opt_end || pos+1 > data_end) - return -1; - opt = *(__u8 *)pos; // Save value to variable so I don't have to perform any more data_end checks on the option kind - if (opt == 0) // Reached end of TCP options - return -1; - if (opt == 1) {// TCP NOP option - advance one byte - pos++; - continue; - } - // Option > 1, should have option size - if (pos+2 > opt_end || pos+2 > data_end) - return -1; - opt_size = *(__u8 *)(pos+1); // Save value to variable so I don't have to perform any more data_end checks on option size + if (tcph + 1 > data_end) + return -1; + int len = tcph->doff << 2; + if (len <= sizeof(struct tcphdr)) // No TCP options + return -1; + void *pos = (void *)(tcph + 1); + void *opt_end = ((void *)tcph + len); + __u8 i, opt, opt_size; +#pragma unroll + for (i = 0; i < MAX_TCP_OPTIONS; i++) { + if (pos + 1 > opt_end || pos + 1 > data_end) + return -1; + opt = *(__u8 *)pos; // Save value to avoid future data_end comparisons + if (opt == 0) // Reached end of TCP options + return -1; + if (opt == 1) { // TCP NOP option - advance one byte + pos++; + continue; + } + // Option > 1, should have option size + if (pos + 2 > opt_end || pos + 2 > data_end) + return -1; + opt_size = *(__u8 *)(pos + 1); // Save value to avoid future data_end comparisons - if (opt == 8 && opt_size == 10) { // Option-kind is TCP timestap (yey!) - if (pos + opt_size > opt_end || pos + opt_size > data_end) + // Option-kind is TCP timestap (yey!) + if (opt == 8 && opt_size == 10) { + if (pos + opt_size > opt_end || + pos + opt_size > data_end) + return -1; + *tsval = *(__u32 *)(pos + 2); + *tsecr = *(__u32 *)(pos + 6); + return 0; + } + + // Some other TCP option - advance option-length bytes + pos += opt_size; + } return -1; - *tsval = *(__u32 *)(pos + 2); - *tsecr = *(__u32 *)(pos + 6); - return 0; - } - - // Some other TCP option - advance option-length bytes - pos += opt_size; - } - return -1; } #endif