* [PATCH net 2/2] ibmvnic: Update driver queues after change in ring size support
From: Thomas Falcon @ 2018-11-21 17:17 UTC (permalink / raw)
To: netdev, linuxppc-dev; +Cc: mwb, julietk, tyreld, Thomas Falcon
In-Reply-To: <1542820679-30184-1-git-send-email-tlfalcon@linux.ibm.com>
During device reset, queue memory is not being updated to accommodate
changes in ring buffer sizes supported by backing hardware. Track
any differences in ring buffer sizes following the reset and update
queue memory when possible.
Signed-off-by: Thomas Falcon <tlfalcon@linux.ibm.com>
---
drivers/net/ethernet/ibm/ibmvnic.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/ibm/ibmvnic.c b/drivers/net/ethernet/ibm/ibmvnic.c
index 066897a..c0203a0 100644
--- a/drivers/net/ethernet/ibm/ibmvnic.c
+++ b/drivers/net/ethernet/ibm/ibmvnic.c
@@ -1737,6 +1737,7 @@ static int do_reset(struct ibmvnic_adapter *adapter,
struct ibmvnic_rwi *rwi, u32 reset_state)
{
u64 old_num_rx_queues, old_num_tx_queues;
+ u64 old_num_rx_slots, old_num_tx_slots;
struct net_device *netdev = adapter->netdev;
int i, rc;
@@ -1748,6 +1749,8 @@ static int do_reset(struct ibmvnic_adapter *adapter,
old_num_rx_queues = adapter->req_rx_queues;
old_num_tx_queues = adapter->req_tx_queues;
+ old_num_rx_slots = adapter->req_rx_add_entries_per_subcrq;
+ old_num_tx_slots = adapter->req_tx_entries_per_subcrq;
ibmvnic_cleanup(netdev);
@@ -1810,7 +1813,11 @@ static int do_reset(struct ibmvnic_adapter *adapter,
if (rc)
return rc;
} else if (adapter->req_rx_queues != old_num_rx_queues ||
- adapter->req_tx_queues != old_num_tx_queues) {
+ adapter->req_tx_queues != old_num_tx_queues ||
+ adapter->req_rx_add_entries_per_subcrq !=
+ old_num_rx_slots ||
+ adapter->req_tx_entries_per_subcrq !=
+ old_num_tx_slots) {
release_rx_pools(adapter);
release_tx_pools(adapter);
release_napi(adapter);
--
1.8.3.1
^ permalink raw reply related
* [PATCH net v2] net: don't keep lonely packets forever in the gro hash
From: Paolo Abeni @ 2018-11-21 17:21 UTC (permalink / raw)
To: netdev; +Cc: David S. Miller, Willem de Bruijn, Eric Dumazet
Eric noted that with UDP GRO and NAPI timeout, we could keep a single
UDP packet inside the GRO hash forever, if the related NAPI instance
calls napi_gro_complete() at an higher frequency than the NAPI timeout.
Willem noted that even TCP packets could be trapped there, till the
next retransmission.
This patch tries to address the issue, flushing the old packets -
those with a NAPI_GRO_CB age before the current jiffy - before scheduling
the NAPI timeout. The rationale is that such a timeout should be
well below a jiffy and we are not flushing packets eligible for sane GRO.
v1 -> v2:
- clarified the commit message and comment
RFC -> v1:
- added 'Fixes tags', cleaned-up the wording.
Reported-by: Eric Dumazet <eric.dumazet@gmail.com>
Fixes: 3b47d30396ba ("net: gro: add a per device gro flush timer")
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
net/core/dev.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/net/core/dev.c b/net/core/dev.c
index 066aa902d85c..ddc551f24ba2 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -5970,11 +5970,14 @@ bool napi_complete_done(struct napi_struct *n, int work_done)
if (work_done)
timeout = n->dev->gro_flush_timeout;
+ /* When the NAPI instance uses a timeout and keeps postponing
+ * it, we need to bound somehow the time packets are kept in
+ * the GRO layer
+ */
+ napi_gro_flush(n, !!timeout);
if (timeout)
hrtimer_start(&n->timer, ns_to_ktime(timeout),
HRTIMER_MODE_REL_PINNED);
- else
- napi_gro_flush(n, false);
}
if (unlikely(!list_empty(&n->poll_list))) {
/* If n->poll_list is not empty, we need to mask irqs */
--
2.17.2
^ permalink raw reply related
* [PATCH net] net/sched: act_police: add missing spinlock initialization
From: Davide Caratti @ 2018-11-21 17:23 UTC (permalink / raw)
To: Eric Dumazet, Jamal Hadi Salim, Cong Wang, Jiri Pirko,
David S. Miller
Cc: netdev, Ivan Vecera
commit f2cbd4852820 ("net/sched: act_police: fix race condition on state
variables") introduces a new spinlock, but forgets its initialization.
Ensure that tcf_police_init() initializes 'tcfp_lock' every time a 'police'
action is newly created, to avoid the following lockdep splat:
INFO: trying to register non-static key.
the code is fine but needs lockdep annotation.
turning off the locking correctness validator.
<...>
Call Trace:
dump_stack+0x85/0xcb
register_lock_class+0x581/0x590
__lock_acquire+0xd4/0x1330
? tcf_police_init+0x2fa/0x650 [act_police]
? lock_acquire+0x9e/0x1a0
lock_acquire+0x9e/0x1a0
? tcf_police_init+0x2fa/0x650 [act_police]
? tcf_police_init+0x55a/0x650 [act_police]
_raw_spin_lock_bh+0x34/0x40
? tcf_police_init+0x2fa/0x650 [act_police]
tcf_police_init+0x2fa/0x650 [act_police]
tcf_action_init_1+0x384/0x4c0
tcf_action_init+0xf6/0x160
tcf_action_add+0x73/0x170
tc_ctl_action+0x122/0x160
rtnetlink_rcv_msg+0x2a4/0x490
? netlink_deliver_tap+0x99/0x400
? validate_linkmsg+0x370/0x370
netlink_rcv_skb+0x4d/0x130
netlink_unicast+0x196/0x230
netlink_sendmsg+0x2e5/0x3e0
sock_sendmsg+0x36/0x40
___sys_sendmsg+0x280/0x2f0
? _raw_spin_unlock+0x24/0x30
? handle_pte_fault+0xafe/0xf30
? find_held_lock+0x2d/0x90
? syscall_trace_enter+0x1df/0x360
? __sys_sendmsg+0x5e/0xa0
__sys_sendmsg+0x5e/0xa0
do_syscall_64+0x60/0x210
entry_SYSCALL_64_after_hwframe+0x49/0xbe
RIP: 0033:0x7f1841c7cf10
Code: c3 48 8b 05 82 6f 2c 00 f7 db 64 89 18 48 83 cb ff eb dd 0f 1f 80 00 00 00 00 83 3d 8d d0 2c 00 00 75 10 b8 2e 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 31 c3 48 83 ec 08 e8 ae cc 00 00 48 89 04 24
RSP: 002b:00007ffcf9df4d68 EFLAGS: 00000246 ORIG_RAX: 000000000000002e
RAX: ffffffffffffffda RBX: 0000000000000001 RCX: 00007f1841c7cf10
RDX: 0000000000000000 RSI: 00007ffcf9df4dc0 RDI: 0000000000000003
RBP: 000000005bf56105 R08: 0000000000000002 R09: 00007ffcf9df8edc
R10: 00007ffcf9df47e0 R11: 0000000000000246 R12: 0000000000671be0
R13: 00007ffcf9df4e84 R14: 0000000000000008 R15: 0000000000000000
Fixes: f2cbd4852820 ("net/sched: act_police: fix race condition on state variables")
Reported-by: Cong Wang <xiyou.wangcong@gmail.com>
Signed-off-by: Davide Caratti <dcaratti@redhat.com>
---
net/sched/act_police.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/net/sched/act_police.c b/net/sched/act_police.c
index ee4665a5a022..37c9b8f0e10f 100644
--- a/net/sched/act_police.c
+++ b/net/sched/act_police.c
@@ -124,6 +124,7 @@ static int tcf_police_init(struct net *net, struct nlattr *nla,
return ret;
}
ret = ACT_P_CREATED;
+ spin_lock_init(&(to_police(*a)->tcfp_lock));
} else if (!ovr) {
tcf_idr_release(*a, bind);
return -EEXIST;
--
2.19.1
^ permalink raw reply related
* Re: [PATCH bpf-next] bpf: libbpf: retry program creation without the name
From: Stanislav Fomichev @ 2018-11-21 17:28 UTC (permalink / raw)
To: Quentin Monnet
Cc: Alexei Starovoitov, Daniel Borkmann, Stanislav Fomichev, netdev,
ast, vladum
In-Reply-To: <8bcd97bc-fffa-4cb7-2fcd-8433f56ea04d@netronome.com>
On 11/21, Quentin Monnet wrote:
> 2018-11-20 15:26 UTC-0800 ~ Stanislav Fomichev <sdf@fomichev.me>
> > On 11/20, Alexei Starovoitov wrote:
> >> On Wed, Nov 21, 2018 at 12:18:57AM +0100, Daniel Borkmann wrote:
> >>> On 11/21/2018 12:04 AM, Alexei Starovoitov wrote:
> >>>> On Tue, Nov 20, 2018 at 01:19:05PM -0800, Stanislav Fomichev wrote:
> >>>>> On 11/20, Alexei Starovoitov wrote:
> >>>>>> On Mon, Nov 19, 2018 at 04:46:25PM -0800, Stanislav Fomichev wrote:
> >>>>>>> [Recent commit 23499442c319 ("bpf: libbpf: retry map creation without
> >>>>>>> the name") fixed this issue for maps, let's do the same for programs.]
> >>>>>>>
> >>>>>>> Since commit 88cda1c9da02 ("bpf: libbpf: Provide basic API support
> >>>>>>> to specify BPF obj name"), libbpf unconditionally sets bpf_attr->name
> >>>>>>> for programs. Pre v4.14 kernels don't know about programs names and
> >>>>>>> return an error about unexpected non-zero data. Retry sys_bpf without
> >>>>>>> a program name to cover older kernels.
> >>>>>>>
> >>>>>>> Signed-off-by: Stanislav Fomichev <sdf@google.com>
> >>>>>>> ---
> >>>>>>> tools/lib/bpf/bpf.c | 10 ++++++++++
> >>>>>>> 1 file changed, 10 insertions(+)
> >>>>>>>
> >>>>>>> diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c
> >>>>>>> index 961e1b9fc592..cbe9d757c646 100644
> >>>>>>> --- a/tools/lib/bpf/bpf.c
> >>>>>>> +++ b/tools/lib/bpf/bpf.c
> >>>>>>> @@ -212,6 +212,16 @@ int bpf_load_program_xattr(const struct bpf_load_program_attr *load_attr,
> >>>>>>> if (fd >= 0 || !log_buf || !log_buf_sz)
> >>>>>>> return fd;
> >>>>>>>
> >>>>>>> + if (fd < 0 && errno == E2BIG && load_attr->name) {
> >>>>>>> + /* Retry the same syscall, but without the name.
> >>>>>>> + * Pre v4.14 kernels don't support prog names.
> >>>>>>> + */
> >>>>>>
> >>>>>> I'm afraid that will put unnecessary stress on the kernel.
> >>>>>> This check needs to be tighter.
> >>>>>> Like E2BIG and anything in the log_buf probably means that
> >>>>>> E2BIG came from the verifier and nothing to do with prog_name.
> >>>>>> Asking kernel to repeat is an unnecessary work.
> >>>>>>
> >>>>>> In general we need to think beyond this single prog_name field.
> >>>>>> There are bunch of other fields in bpf_load_program_xattr() and older kernels
> >>>>>> won't support them. Are we going to zero them out one by one
> >>>>>> and retry? I don't think that would be practical.
> >>>>> I general, we don't want to zero anything out. However,
> >>>>> for this particular problem the rationale is the following:
> >>>>> In commit 88cda1c9da02 we started unconditionally setting {prog,map}->name
> >>>>> from the 'higher' libbpfc layer which breaks users on the older kernels.
> >>>>>
> >>>>>> Also libbpf silently ignoring prog_name is not great for debugging.
> >>>>>> A warning is needed.
> >>>>>> But it cannot be done out of lib/bpf/bpf.c, since it's a set of syscall
> >>>>>> wrappers.
> >>>>>> Imo such "old kernel -> lets retry" feature should probably be done
> >>>>>> at lib/bpf/libbpf.c level. inside load_program().
> >>>>> For maps bpftools calls bpf_create_map_xattr directly, that's why
> >>>>> for maps I did the retry on the lower level (and why for programs I initially
> >>>>> thought about doing the same). However, in this case maybe asking
> >>>>> user to omit 'name' argument might be a better option.
> >>>>>
> >>>>> For program names, I agree, we might think about doing it on the higher
> >>>>> level (although I'm not sure whether we want to have different API
> >>>>> expectations, i.e. bpf_create_map_xattr ignoring the name and
> >>>>> bpf_load_program_xattr not ignoring the name).
> >>>>>
> >>>>> So given that rationale above, what do you think is the best way to
> >>>>> move forward?
> >>>>> 1. Same patch, but tighten the retry check inside bpf_load_program_xattr ?
> >>>>> 2. Move this retry logic into load_program and have different handling
> >>>>> for bpf_create_map_xattr vs bpf_load_program_xattr ?
> >>>>> 3. Do 2 and move the retry check for maps from bpf_create_map_xattr
> >>>>> into bpf_object__create_maps ?
> >>>>>
> >>>>> (I'm slightly leaning towards #3)
> >>>>
> >>>> me too. I think it's cleaner for maps to do it in
> >>>> bpf_object__create_maps().
> >>>> Originally bpf.c was envisioned to be a thin layer on top of bpf syscall.
> >>>> Whereas 'smart bits' would go into libbpf.c
> >>>
> >>> Can't we create in bpf_object__load() a small helper bpf_object__probe_caps()
> >>> which would figure this out _once_ upon start with a few things to probe for
> >>> availability in the underlying kernel for maps and programs? E.g. programs
> >>> it could try to inject a tiny 'r0 = 0; exit' snippet where we figure out
> >>> things like prog name support etc. Given underlying kernel doesn't change, we
> >>> would only try this once and it doesn't require fallback every time.
> >>
> >> +1. great idea!
> > Sounds good, let me try to do it.
> >
> > It sounds more like a recent LPC proposal/idea to have some sys_bpf option
> > to query BPF features. This new bpf_object__probe_caps can probably query
> > that in the future if we eventually add support for it.
> >
>
> Hi,
>
> LPC proposal indeed. I've been working on implementing this kind of
> probes in bpftool. I don't probe name support for now (but I can
> certainly add it), but I detect supported program types, map types,
> header functions, and a couple of other parameters. The idea (initially
> from Daniel) was to dump "#define" declarations that could later be
> included in a header file and used for a BPF project (or alternatively,
> JSON output).
Oh, nice, I didn't know someone was already working on it!
> I felt like bpftool was maybe a better place to do it, as the set of
> probes may grow large (all types, helpers, etc). It might have
> consequences on the running system: for example, if I don't raise the
> rlimit in bpftool before starting the probes, a good half of them fail
> because of consecutive program creation and reclaim delay for locked
> memory usage.
Should we aim for something like build system feature checks? Where we
preserve these probe results between bpftool/libbpf invocations so we
don't re-run them again?
> So should we start adding probes to libbpf and should I move mine to the
> lib as well, or should the one in the v3 of this series be directed to
> something like bpftool instead?
We need them (well, at least the name checks) for libbpf because that's
what we link against and what we use to load the programs, bpftool is
less of an issue right now. But my patch was mostly a hackish solution
until we get the real feature checks :-)
^ permalink raw reply
* [PATCH bpf-next v3] libbpf: make sure bpf headers are c++ include-able
From: Stanislav Fomichev @ 2018-11-21 17:29 UTC (permalink / raw)
To: netdev, ast, daniel; +Cc: yhs, Stanislav Fomichev
Wrap headers in extern "C", to turn off C++ mangling.
This simplifies including libbpf in c++ and linking against it.
v2 changes:
* do the same for btf.h
v3 changes:
* test_libbpf.cpp to test for possible future c++ breakages
Signed-off-by: Stanislav Fomichev <sdf@google.com>
---
tools/lib/bpf/Makefile | 15 ++++++++++++---
tools/lib/bpf/bpf.h | 9 +++++++++
tools/lib/bpf/btf.h | 8 ++++++++
tools/lib/bpf/libbpf.h | 9 +++++++++
tools/lib/bpf/test_libbpf.cpp | 18 ++++++++++++++++++
5 files changed, 56 insertions(+), 3 deletions(-)
create mode 100644 tools/lib/bpf/test_libbpf.cpp
diff --git a/tools/lib/bpf/Makefile b/tools/lib/bpf/Makefile
index 425b480bda75..1b4a683a00fc 100644
--- a/tools/lib/bpf/Makefile
+++ b/tools/lib/bpf/Makefile
@@ -66,7 +66,7 @@ ifndef VERBOSE
endif
FEATURE_USER = .libbpf
-FEATURE_TESTS = libelf libelf-mmap bpf reallocarray
+FEATURE_TESTS = libelf libelf-mmap bpf reallocarray cxx
FEATURE_DISPLAY = libelf bpf
INCLUDES = -I. -I$(srctree)/tools/include -I$(srctree)/tools/arch/$(ARCH)/include/uapi -I$(srctree)/tools/include/uapi
@@ -148,6 +148,12 @@ LIB_FILE := $(addprefix $(OUTPUT),$(LIB_FILE))
CMD_TARGETS = $(LIB_FILE)
+CXX_TEST_TARGET = $(OUTPUT)test_libbpf
+
+ifeq ($(feature-cxx), 1)
+ CMD_TARGETS += $(CXX_TEST_TARGET)
+endif
+
TARGETS = $(CMD_TARGETS)
all: fixdep all_cmd
@@ -175,6 +181,9 @@ $(OUTPUT)libbpf.so: $(BPF_IN)
$(OUTPUT)libbpf.a: $(BPF_IN)
$(QUIET_LINK)$(RM) $@; $(AR) rcs $@ $^
+$(OUTPUT)test_libbpf: test_libbpf.cpp $(OUTPUT)libbpf.a
+ $(QUIET_LINK)$(CXX) $^ -lelf -o $@
+
define do_install
if [ ! -d '$(DESTDIR_SQ)$2' ]; then \
$(INSTALL) -d -m 755 '$(DESTDIR_SQ)$2'; \
@@ -201,8 +210,8 @@ install: install_lib
$(Q)$(MAKE) -C $(srctree)/tools/build/feature/ clean >/dev/null
clean:
- $(call QUIET_CLEAN, libbpf) $(RM) *.o *~ $(TARGETS) *.a *.so .*.d .*.cmd \
- $(RM) LIBBPF-CFLAGS
+ $(call QUIET_CLEAN, libbpf) $(RM) $(TARGETS) $(CXX_TEST_TARGET) \
+ *.o *~ *.a *.so .*.d .*.cmd LIBBPF-CFLAGS
$(call QUIET_CLEAN, core-gen) $(RM) $(OUTPUT)FEATURE-DUMP.libbpf
diff --git a/tools/lib/bpf/bpf.h b/tools/lib/bpf/bpf.h
index 26a51538213c..9ea3aec82d8a 100644
--- a/tools/lib/bpf/bpf.h
+++ b/tools/lib/bpf/bpf.h
@@ -27,6 +27,10 @@
#include <stdbool.h>
#include <stddef.h>
+#ifdef __cplusplus
+extern "C" {
+#endif
+
#ifndef LIBBPF_API
#define LIBBPF_API __attribute__((visibility("default")))
#endif
@@ -128,4 +132,9 @@ LIBBPF_API int bpf_load_btf(void *btf, __u32 btf_size, char *log_buf,
LIBBPF_API int bpf_task_fd_query(int pid, int fd, __u32 flags, char *buf,
__u32 *buf_len, __u32 *prog_id, __u32 *fd_type,
__u64 *probe_offset, __u64 *probe_addr);
+
+#ifdef __cplusplus
+} /* extern "C" */
+#endif
+
#endif /* __LIBBPF_BPF_H */
diff --git a/tools/lib/bpf/btf.h b/tools/lib/bpf/btf.h
index b77e7080f7e7..5f3d7de850fc 100644
--- a/tools/lib/bpf/btf.h
+++ b/tools/lib/bpf/btf.h
@@ -6,6 +6,10 @@
#include <linux/types.h>
+#ifdef __cplusplus
+extern "C" {
+#endif
+
#ifndef LIBBPF_API
#define LIBBPF_API __attribute__((visibility("default")))
#endif
@@ -29,4 +33,8 @@ LIBBPF_API int btf__resolve_type(const struct btf *btf, __u32 type_id);
LIBBPF_API int btf__fd(const struct btf *btf);
LIBBPF_API const char *btf__name_by_offset(const struct btf *btf, __u32 offset);
+#ifdef __cplusplus
+} /* extern "C" */
+#endif
+
#endif /* __LIBBPF_BTF_H */
diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
index b1686a787102..74e57e041705 100644
--- a/tools/lib/bpf/libbpf.h
+++ b/tools/lib/bpf/libbpf.h
@@ -16,6 +16,10 @@
#include <sys/types.h> // for size_t
#include <linux/bpf.h>
+#ifdef __cplusplus
+extern "C" {
+#endif
+
#ifndef LIBBPF_API
#define LIBBPF_API __attribute__((visibility("default")))
#endif
@@ -335,4 +339,9 @@ int libbpf_nl_get_qdisc(int sock, unsigned int nl_pid, int ifindex,
libbpf_dump_nlmsg_t dump_qdisc_nlmsg, void *cookie);
int libbpf_nl_get_filter(int sock, unsigned int nl_pid, int ifindex, int handle,
libbpf_dump_nlmsg_t dump_filter_nlmsg, void *cookie);
+
+#ifdef __cplusplus
+} /* extern "C" */
+#endif
+
#endif /* __LIBBPF_LIBBPF_H */
diff --git a/tools/lib/bpf/test_libbpf.cpp b/tools/lib/bpf/test_libbpf.cpp
new file mode 100644
index 000000000000..abf3fc25c9fa
--- /dev/null
+++ b/tools/lib/bpf/test_libbpf.cpp
@@ -0,0 +1,18 @@
+/* SPDX-License-Identifier: (LGPL-2.1 OR BSD-2-Clause) */
+#include "libbpf.h"
+#include "bpf.h"
+#include "btf.h"
+
+/* do nothing, just make sure we can link successfully */
+
+int main(int argc, char *argv[])
+{
+ /* libbpf.h */
+ libbpf_set_print(NULL, NULL, NULL);
+
+ /* bpf.h */
+ bpf_prog_get_fd_by_id(0);
+
+ /* btf.h */
+ btf__new(NULL, 0, NULL);
+}
--
2.19.1.1215.g8438c0b245-goog
^ permalink raw reply related
* Re: [PATCH net v2] net: don't keep lonely packets forever in the gro hash
From: Willem de Bruijn @ 2018-11-21 17:33 UTC (permalink / raw)
To: Paolo Abeni; +Cc: Network Development, David Miller, Eric Dumazet
In-Reply-To: <818e16990bf166f53fe1bbac9ee5df6f38063e86.1542803002.git.pabeni@redhat.com>
On Wed, Nov 21, 2018 at 12:21 PM Paolo Abeni <pabeni@redhat.com> wrote:
>
> Eric noted that with UDP GRO and NAPI timeout, we could keep a single
> UDP packet inside the GRO hash forever, if the related NAPI instance
> calls napi_gro_complete() at an higher frequency than the NAPI timeout.
> Willem noted that even TCP packets could be trapped there, till the
> next retransmission.
> This patch tries to address the issue, flushing the old packets -
> those with a NAPI_GRO_CB age before the current jiffy - before scheduling
> the NAPI timeout. The rationale is that such a timeout should be
> well below a jiffy and we are not flushing packets eligible for sane GRO.
>
> v1 -> v2:
> - clarified the commit message and comment
>
> RFC -> v1:
> - added 'Fixes tags', cleaned-up the wording.
>
> Reported-by: Eric Dumazet <eric.dumazet@gmail.com>
> Fixes: 3b47d30396ba ("net: gro: add a per device gro flush timer")
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
Acked-by: Willem de Bruijn <willemb@google.com>
Thanks for making those small changes.
^ permalink raw reply
* Re: [PATCH iproute2] ipnetns: parse nsid as a signed integer
From: Stephen Hemminger @ 2018-11-21 17:37 UTC (permalink / raw)
To: Nicolas Dichtel; +Cc: netdev, dsahern, joe, daniel
In-Reply-To: <20181121094427.19007-1-nicolas.dichtel@6wind.com>
On Wed, 21 Nov 2018 10:44:27 +0100
Nicolas Dichtel <nicolas.dichtel@6wind.com> wrote:
> Don't confuse the user, nsid is a signed integer, this kind of command
> should return an error: 'ip netns set foo 0xffffffff'.
>
> Also, a valid value is a positive value. To let the kernel chooses a value,
> the keyword 'auto' must be used.
>
> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
Thanks for catching this. Next time add a Fixes: tag if possible.
Applied.
^ permalink raw reply
* Re: [PATCH iproute2] bpf: initialise map symbol before retrieving and comparing its type
From: Stephen Hemminger @ 2018-11-21 17:38 UTC (permalink / raw)
To: Quentin Monnet
Cc: David Ahern, Yonghong Song, Alexei Starovoitov, Daniel Borkmann,
netdev, oss-drivers
In-Reply-To: <1542677187-25432-1-git-send-email-quentin.monnet@netronome.com>
On Tue, 20 Nov 2018 01:26:27 +0000
Quentin Monnet <quentin.monnet@netronome.com> wrote:
> In order to compare BPF map symbol type correctly in regard to the
> latest LLVM, commit 7a04dd84a7f9 ("bpf: check map symbol type properly
> with newer llvm compiler") compares map symbol type to both NOTYPE and
> OBJECT. To do so, it first retrieves the type from "sym.st_info" and
> stores it into a temporary variable.
>
> However, the type is collected from the symbol "sym" before this latter
> symbol is actually updated. gelf_getsym() is called after that and
> updates "sym", and when comparison with OBJECT or NOTYPE happens it is
> done on the type of the symbol collected in the previous passage of the
> loop (or on an uninitialised symbol on the first passage). This may
> eventually break map collection from the ELF file.
>
> Fix this by assigning the type to the temporary variable only after the
> call to gelf_getsym().
>
> Fixes: 7a04dd84a7f9 ("bpf: check map symbol type properly with newer llvm compiler")
> Reported-by: Ron Philip <ron.philip@netronome.com>
> Signed-off-by: Quentin Monnet <quentin.monnet@netronome.com>
> Reviewed-by: Jiong Wang <jiong.wang@netronome.com>
> ---
> lib/bpf.c | 10 +++++++---
> 1 file changed, 7 insertions(+), 3 deletions(-)
>
Applied.
^ permalink raw reply
* [PATCH bpf-next 0/2] libbpf: ABI versioning
From: Andrey Ignatov @ 2018-11-21 17:40 UTC (permalink / raw)
To: netdev; +Cc: Andrey Ignatov, ast, daniel, kernel-team
This patch set adds ABI versioning to libbpf.
Patch 1 adds version script and has more details on why it's needed.
Patch 2 adds simple check that all global symbols are versioned.
Andrey Ignatov (2):
libbpf: Add version script for DSO
libbpf: Verify versioned symbols
tools/lib/bpf/Makefile | 23 +++++++-
tools/lib/bpf/libbpf.map | 120 +++++++++++++++++++++++++++++++++++++++
2 files changed, 141 insertions(+), 2 deletions(-)
create mode 100644 tools/lib/bpf/libbpf.map
--
2.17.1
^ permalink raw reply
* [PATCH bpf-next 2/2] libbpf: Verify versioned symbols
From: Andrey Ignatov @ 2018-11-21 17:40 UTC (permalink / raw)
To: netdev; +Cc: Andrey Ignatov, ast, daniel, kernel-team
In-Reply-To: <cover.1542821624.git.rdna@fb.com>
Since ABI versioning info is kept separately from the code it's easy to
forget to update it while adding a new API.
Add simple verification that all global symbols exported with LIBBPF_API
are versioned in libbpf.map version script.
The idea is to check that number of global symbols in libbpf-in.o, that
is the input to the linker, matches with number of unique versioned
symbols in libbpf.so, that is the output of the linker. If these numbers
don't match, it may mean some symbol was not versioned and make will
fail.
"Unique" means that if a symbol is present in more than one version of
ABI due to ABI changes, it'll be counted once.
Another option to calculate number of global symbols in the "input"
could be to count number of LIBBPF_ABI entries in C headers but it seems
to be fragile.
Example of output when a symbol is missing in version script:
...
LD libbpf-in.o
LINK libbpf.a
LINK libbpf.so
Warning: Num of global symbols in libbpf-in.o (115) does NOT match
with num of versioned symbols in libbpf.so (114). Please make sure all
LIBBPF_API symbols are versioned in libbpf.map.
make: *** [check_abi] Error 1
Signed-off-by: Andrey Ignatov <rdna@fb.com>
---
tools/lib/bpf/Makefile | 19 ++++++++++++++++++-
1 file changed, 18 insertions(+), 1 deletion(-)
diff --git a/tools/lib/bpf/Makefile b/tools/lib/bpf/Makefile
index d76c41fa2d39..742ef0d5210e 100644
--- a/tools/lib/bpf/Makefile
+++ b/tools/lib/bpf/Makefile
@@ -147,13 +147,18 @@ BPF_IN := $(OUTPUT)libbpf-in.o
LIB_FILE := $(addprefix $(OUTPUT),$(LIB_FILE))
VERSION_SCRIPT := libbpf.map
+GLOBAL_SYM_COUNT = $(shell readelf -s $(BPF_IN) | \
+ awk '/GLOBAL/ && /DEFAULT/ && !/UND/ {s++} END{print s}')
+VERSIONED_SYM_COUNT = $(shell readelf -s $(OUTPUT)libbpf.so | \
+ grep -Eo '[^ ]+@LIBBPF_' | cut -d@ -f1 | sort -u | wc -l)
+
CMD_TARGETS = $(LIB_FILE)
TARGETS = $(CMD_TARGETS)
all: fixdep all_cmd
-all_cmd: $(CMD_TARGETS)
+all_cmd: $(CMD_TARGETS) check
$(BPF_IN): force elfdep bpfdep
@(test -f ../../include/uapi/linux/bpf.h -a -f ../../../include/uapi/linux/bpf.h && ( \
@@ -177,6 +182,18 @@ $(OUTPUT)libbpf.so: $(BPF_IN)
$(OUTPUT)libbpf.a: $(BPF_IN)
$(QUIET_LINK)$(RM) $@; $(AR) rcs $@ $^
+check: check_abi
+
+check_abi: $(OUTPUT)libbpf.so
+ @if [ "$(GLOBAL_SYM_COUNT)" != "$(VERSIONED_SYM_COUNT)" ]; then \
+ echo "Warning: Num of global symbols in $(BPF_IN)" \
+ "($(GLOBAL_SYM_COUNT)) does NOT match with num of" \
+ "versioned symbols in $^ ($(VERSIONED_SYM_COUNT))." \
+ "Please make sure all LIBBPF_API symbols are" \
+ "versioned in $(VERSION_SCRIPT)." >&2; \
+ exit 1; \
+ fi
+
define do_install
if [ ! -d '$(DESTDIR_SQ)$2' ]; then \
$(INSTALL) -d -m 755 '$(DESTDIR_SQ)$2'; \
--
2.17.1
^ permalink raw reply related
* [PATCH bpf-next 1/2] libbpf: Add version script for DSO
From: Andrey Ignatov @ 2018-11-21 17:40 UTC (permalink / raw)
To: netdev; +Cc: Andrey Ignatov, ast, daniel, kernel-team
In-Reply-To: <cover.1542821624.git.rdna@fb.com>
More and more projects use libbpf and one day it'll likely be packaged
and distributed as DSO and that requires ABI versioning so that both
compatible and incompatible changes to ABI can be introduced in a safe
way in the future without breaking executables dynamically linked with a
previous version of the library.
Usual way to do ABI versioning is version script for the linker. Add
such a script for libbpf. All global symbols currently exported via
LIBBPF_API macro are added to the version script libbpf.map.
The version name LIBBPF_0.0.1 is constructed from the name of the
library + version specified by $(LIBBPF_VERSION) in Makefile.
Version script does not duplicate the work done by LIBBPF_API macro, it
rather complements it. The macro is used at compile time and can be used
by compiler to do optimization that can't be done at link time, it is
purely about global symbol visibility. The version script, in turn, is
used at link time and takes care of ABI versioning. Both techniques are
described in details in [1].
Whenever ABI is changed in the future, version script should be changed
appropriately.
[1] https://www.akkadia.org/drepper/dsohowto.pdf
Signed-off-by: Andrey Ignatov <rdna@fb.com>
---
tools/lib/bpf/Makefile | 4 +-
tools/lib/bpf/libbpf.map | 120 +++++++++++++++++++++++++++++++++++++++
2 files changed, 123 insertions(+), 1 deletion(-)
create mode 100644 tools/lib/bpf/libbpf.map
diff --git a/tools/lib/bpf/Makefile b/tools/lib/bpf/Makefile
index 425b480bda75..d76c41fa2d39 100644
--- a/tools/lib/bpf/Makefile
+++ b/tools/lib/bpf/Makefile
@@ -145,6 +145,7 @@ include $(srctree)/tools/build/Makefile.include
BPF_IN := $(OUTPUT)libbpf-in.o
LIB_FILE := $(addprefix $(OUTPUT),$(LIB_FILE))
+VERSION_SCRIPT := libbpf.map
CMD_TARGETS = $(LIB_FILE)
@@ -170,7 +171,8 @@ $(BPF_IN): force elfdep bpfdep
$(Q)$(MAKE) $(build)=libbpf
$(OUTPUT)libbpf.so: $(BPF_IN)
- $(QUIET_LINK)$(CC) --shared $^ -o $@
+ $(QUIET_LINK)$(CC) --shared -Wl,--version-script=$(VERSION_SCRIPT) \
+ $^ -o $@
$(OUTPUT)libbpf.a: $(BPF_IN)
$(QUIET_LINK)$(RM) $@; $(AR) rcs $@ $^
diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
new file mode 100644
index 000000000000..9fe416b68c7d
--- /dev/null
+++ b/tools/lib/bpf/libbpf.map
@@ -0,0 +1,120 @@
+LIBBPF_0.0.1 {
+ global:
+ bpf_btf_get_fd_by_id;
+ bpf_create_map;
+ bpf_create_map_in_map;
+ bpf_create_map_in_map_node;
+ bpf_create_map_name;
+ bpf_create_map_node;
+ bpf_create_map_xattr;
+ bpf_load_btf;
+ bpf_load_program;
+ bpf_load_program_xattr;
+ bpf_map__btf_key_type_id;
+ bpf_map__btf_value_type_id;
+ bpf_map__def;
+ bpf_map_delete_elem;
+ bpf_map__fd;
+ bpf_map_get_fd_by_id;
+ bpf_map_get_next_id;
+ bpf_map_get_next_key;
+ bpf_map__is_offload_neutral;
+ bpf_map_lookup_and_delete_elem;
+ bpf_map_lookup_elem;
+ bpf_map__name;
+ bpf_map__next;
+ bpf_map__pin;
+ bpf_map__prev;
+ bpf_map__priv;
+ bpf_map__reuse_fd;
+ bpf_map__set_ifindex;
+ bpf_map__set_priv;
+ bpf_map__unpin;
+ bpf_map_update_elem;
+ bpf_object__btf_fd;
+ bpf_object__close;
+ bpf_object__find_map_by_name;
+ bpf_object__find_map_by_offset;
+ bpf_object__find_program_by_title;
+ bpf_object__kversion;
+ bpf_object__load;
+ bpf_object__name;
+ bpf_object__next;
+ bpf_object__open;
+ bpf_object__open_buffer;
+ bpf_object__open_xattr;
+ bpf_object__pin;
+ bpf_object__pin_maps;
+ bpf_object__pin_programs;
+ bpf_object__priv;
+ bpf_object__set_priv;
+ bpf_object__unload;
+ bpf_object__unpin_maps;
+ bpf_object__unpin_programs;
+ bpf_obj_get;
+ bpf_obj_get_info_by_fd;
+ bpf_obj_pin;
+ bpf_perf_event_read_simple;
+ bpf_prog_attach;
+ bpf_prog_detach;
+ bpf_prog_detach2;
+ bpf_prog_get_fd_by_id;
+ bpf_prog_get_next_id;
+ bpf_prog_load;
+ bpf_prog_load_xattr;
+ bpf_prog_query;
+ bpf_program__fd;
+ bpf_program__is_kprobe;
+ bpf_program__is_perf_event;
+ bpf_program__is_raw_tracepoint;
+ bpf_program__is_sched_act;
+ bpf_program__is_sched_cls;
+ bpf_program__is_socket_filter;
+ bpf_program__is_tracepoint;
+ bpf_program__is_xdp;
+ bpf_program__load;
+ bpf_program__next;
+ bpf_program__nth_fd;
+ bpf_program__pin;
+ bpf_program__pin_instance;
+ bpf_program__prev;
+ bpf_program__priv;
+ bpf_program__set_expected_attach_type;
+ bpf_program__set_ifindex;
+ bpf_program__set_kprobe;
+ bpf_program__set_perf_event;
+ bpf_program__set_prep;
+ bpf_program__set_priv;
+ bpf_program__set_raw_tracepoint;
+ bpf_program__set_sched_act;
+ bpf_program__set_sched_cls;
+ bpf_program__set_socket_filter;
+ bpf_program__set_tracepoint;
+ bpf_program__set_type;
+ bpf_program__set_xdp;
+ bpf_program__title;
+ bpf_program__unload;
+ bpf_program__unpin;
+ bpf_program__unpin_instance;
+ bpf_prog_test_run;
+ bpf_raw_tracepoint_open;
+ bpf_set_link_xdp_fd;
+ bpf_task_fd_query;
+ bpf_verify_program;
+ btf__fd;
+ btf__find_by_name;
+ btf__free;
+ btf_get_from_id;
+ btf__name_by_offset;
+ btf__new;
+ btf__resolve_size;
+ btf__resolve_type;
+ btf__type_by_id;
+ libbpf_attach_type_by_name;
+ libbpf_get_error;
+ libbpf_prog_type_by_name;
+ libbpf_set_print;
+ libbpf_strerror;
+ local:
+ *;
+};
--
2.17.1
^ permalink raw reply related
* Re: [PATCH net v2] net: don't keep lonely packets forever in the gro hash
From: Eric Dumazet @ 2018-11-21 17:42 UTC (permalink / raw)
To: Paolo Abeni, netdev; +Cc: David S. Miller, Willem de Bruijn
In-Reply-To: <818e16990bf166f53fe1bbac9ee5df6f38063e86.1542803002.git.pabeni@redhat.com>
On 11/21/2018 09:21 AM, Paolo Abeni wrote:
> Eric noted that with UDP GRO and NAPI timeout, we could keep a single
> UDP packet inside the GRO hash forever, if the related NAPI instance
> calls napi_gro_complete() at an higher frequency than the NAPI timeout.
Acked-by: Eric Dumazet <edumazet@google.com>
^ permalink raw reply
* [PATCH net-next 0/3] tcp: take a bit more care of backlog stress
From: Eric Dumazet @ 2018-11-21 17:52 UTC (permalink / raw)
To: David S . Miller
Cc: netdev, Jean-Louis Dupond, Neal Cardwell, Yuchung Cheng,
Eric Dumazet, Eric Dumazet
While working on the SACK compression issue Jean-Louis Dupond
reported, we found that his linux box was suffering very hard
from tail drops on the socket backlog queue, because the opposite
TCP stack was ont implementing latest RFC recommendations.
First patch is a cleanup
Second patch is attempting coalescing when a new packet must
be added to the backlog queue. Cooking bigger skbs helps
to keep backlog list smaller and speeds its handling when
user thread finally releases the socket lock.
Third patch is implementing head drop as a last resort.
Head drops are generally better for optimal TCP behavior.
Eric Dumazet (3):
tcp: remove hdrlen argument from tcp_queue_rcv()
tcp: implement coalescing on backlog queue
tcp: implement head drops in backlog queue
include/uapi/linux/snmp.h | 1 +
net/ipv4/proc.c | 1 +
net/ipv4/tcp_input.c | 13 +++---
net/ipv4/tcp_ipv4.c | 89 ++++++++++++++++++++++++++++++++++++---
4 files changed, 91 insertions(+), 13 deletions(-)
--
2.19.1.1215.g8438c0b245-goog
^ permalink raw reply
* [PATCH net-next 1/3] tcp: remove hdrlen argument from tcp_queue_rcv()
From: Eric Dumazet @ 2018-11-21 17:52 UTC (permalink / raw)
To: David S . Miller
Cc: netdev, Jean-Louis Dupond, Neal Cardwell, Yuchung Cheng,
Eric Dumazet, Eric Dumazet
In-Reply-To: <20181121175240.6075-1-edumazet@google.com>
Only one caller needs to pull TCP headers, so lets
move __skb_pull() to the caller side.
Signed-off-by: Eric Dumazet <edumazet@google.com>
---
net/ipv4/tcp_input.c | 13 ++++++-------
1 file changed, 6 insertions(+), 7 deletions(-)
diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index edaaebfbcd4693ef6689f3f4c71733b3888c7c2c..e0ad7d3825b5945049c099171ce36e5c8bb9ba99 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -4602,13 +4602,12 @@ static void tcp_data_queue_ofo(struct sock *sk, struct sk_buff *skb)
}
}
-static int __must_check tcp_queue_rcv(struct sock *sk, struct sk_buff *skb, int hdrlen,
- bool *fragstolen)
+static int __must_check tcp_queue_rcv(struct sock *sk, struct sk_buff *skb,
+ bool *fragstolen)
{
int eaten;
struct sk_buff *tail = skb_peek_tail(&sk->sk_receive_queue);
- __skb_pull(skb, hdrlen);
eaten = (tail &&
tcp_try_coalesce(sk, tail,
skb, fragstolen)) ? 1 : 0;
@@ -4659,7 +4658,7 @@ int tcp_send_rcvq(struct sock *sk, struct msghdr *msg, size_t size)
TCP_SKB_CB(skb)->end_seq = TCP_SKB_CB(skb)->seq + size;
TCP_SKB_CB(skb)->ack_seq = tcp_sk(sk)->snd_una - 1;
- if (tcp_queue_rcv(sk, skb, 0, &fragstolen)) {
+ if (tcp_queue_rcv(sk, skb, &fragstolen)) {
WARN_ON_ONCE(fragstolen); /* should not happen */
__kfree_skb(skb);
}
@@ -4719,7 +4718,7 @@ static void tcp_data_queue(struct sock *sk, struct sk_buff *skb)
goto drop;
}
- eaten = tcp_queue_rcv(sk, skb, 0, &fragstolen);
+ eaten = tcp_queue_rcv(sk, skb, &fragstolen);
if (skb->len)
tcp_event_data_recv(sk, skb);
if (TCP_SKB_CB(skb)->tcp_flags & TCPHDR_FIN)
@@ -5585,8 +5584,8 @@ void tcp_rcv_established(struct sock *sk, struct sk_buff *skb)
NET_INC_STATS(sock_net(sk), LINUX_MIB_TCPHPHITS);
/* Bulk data transfer: receiver */
- eaten = tcp_queue_rcv(sk, skb, tcp_header_len,
- &fragstolen);
+ __skb_pull(skb, tcp_header_len);
+ eaten = tcp_queue_rcv(sk, skb, &fragstolen);
tcp_event_data_recv(sk, skb);
--
2.19.1.1215.g8438c0b245-goog
^ permalink raw reply related
* [PATCH net-next 2/3] tcp: implement coalescing on backlog queue
From: Eric Dumazet @ 2018-11-21 17:52 UTC (permalink / raw)
To: David S . Miller
Cc: netdev, Jean-Louis Dupond, Neal Cardwell, Yuchung Cheng,
Eric Dumazet, Eric Dumazet
In-Reply-To: <20181121175240.6075-1-edumazet@google.com>
In case GRO is not as efficient as it should be or disabled,
we might have a user thread trapped in __release_sock() while
softirq handler flood packets up to the point we have to drop.
This patch balances work done from user thread and softirq,
to give more chances to __release_sock() to complete its work.
This also helps if we receive many ACK packets, since GRO
does not aggregate them.
Signed-off-by: Eric Dumazet <edumazet@google.com>
Tested-by: Jean-Louis Dupond <jean-louis@dupond.be>
Cc: Neal Cardwell <ncardwell@google.com>
Cc: Yuchung Cheng <ycheng@google.com>
---
include/uapi/linux/snmp.h | 1 +
net/ipv4/proc.c | 1 +
net/ipv4/tcp_ipv4.c | 75 +++++++++++++++++++++++++++++++++++----
3 files changed, 71 insertions(+), 6 deletions(-)
diff --git a/include/uapi/linux/snmp.h b/include/uapi/linux/snmp.h
index f80135e5feaa886000009db6dff75b2bc2d637b2..86dc24a96c90ab047d5173d625450facd6c6dd79 100644
--- a/include/uapi/linux/snmp.h
+++ b/include/uapi/linux/snmp.h
@@ -243,6 +243,7 @@ enum
LINUX_MIB_TCPREQQFULLDROP, /* TCPReqQFullDrop */
LINUX_MIB_TCPRETRANSFAIL, /* TCPRetransFail */
LINUX_MIB_TCPRCVCOALESCE, /* TCPRcvCoalesce */
+ LINUX_MIB_TCPBACKLOGCOALESCE, /* TCPBacklogCoalesce */
LINUX_MIB_TCPOFOQUEUE, /* TCPOFOQueue */
LINUX_MIB_TCPOFODROP, /* TCPOFODrop */
LINUX_MIB_TCPOFOMERGE, /* TCPOFOMerge */
diff --git a/net/ipv4/proc.c b/net/ipv4/proc.c
index 70289682a6701438aed99a00a9705c39fa4394d3..c3610b37bb4ce665b1976d8cc907b6dd0de42ab9 100644
--- a/net/ipv4/proc.c
+++ b/net/ipv4/proc.c
@@ -219,6 +219,7 @@ static const struct snmp_mib snmp4_net_list[] = {
SNMP_MIB_ITEM("TCPRenoRecoveryFail", LINUX_MIB_TCPRENORECOVERYFAIL),
SNMP_MIB_ITEM("TCPSackRecoveryFail", LINUX_MIB_TCPSACKRECOVERYFAIL),
SNMP_MIB_ITEM("TCPRcvCollapsed", LINUX_MIB_TCPRCVCOLLAPSED),
+ SNMP_MIB_ITEM("TCPBacklogCoalesce", LINUX_MIB_TCPBACKLOGCOALESCE),
SNMP_MIB_ITEM("TCPDSACKOldSent", LINUX_MIB_TCPDSACKOLDSENT),
SNMP_MIB_ITEM("TCPDSACKOfoSent", LINUX_MIB_TCPDSACKOFOSENT),
SNMP_MIB_ITEM("TCPDSACKRecv", LINUX_MIB_TCPDSACKRECV),
diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
index 795605a2327504b8a025405826e7e0ca8dc8501d..401e1d1cb904a4c7963d8baa419cfbf178593344 100644
--- a/net/ipv4/tcp_ipv4.c
+++ b/net/ipv4/tcp_ipv4.c
@@ -1619,12 +1619,10 @@ int tcp_v4_early_demux(struct sk_buff *skb)
bool tcp_add_backlog(struct sock *sk, struct sk_buff *skb)
{
u32 limit = sk->sk_rcvbuf + sk->sk_sndbuf;
-
- /* Only socket owner can try to collapse/prune rx queues
- * to reduce memory overhead, so add a little headroom here.
- * Few sockets backlog are possibly concurrently non empty.
- */
- limit += 64*1024;
+ struct skb_shared_info *shinfo;
+ const struct tcphdr *th;
+ struct sk_buff *tail;
+ unsigned int hdrlen;
/* In case all data was pulled from skb frags (in __pskb_pull_tail()),
* we can fix skb->truesize to its real value to avoid future drops.
@@ -1636,6 +1634,71 @@ bool tcp_add_backlog(struct sock *sk, struct sk_buff *skb)
skb_dst_drop(skb);
+ if (unlikely(tcp_checksum_complete(skb))) {
+ bh_unlock_sock(sk);
+ __TCP_INC_STATS(sock_net(sk), TCP_MIB_CSUMERRORS);
+ __TCP_INC_STATS(sock_net(sk), TCP_MIB_INERRS);
+ return true;
+ }
+
+ /* Attempt coalescing to last skb in backlog, even if we are
+ * above the limits.
+ * This is okay because skb capacity is limited to MAX_SKB_FRAGS.
+ */
+ th = (const struct tcphdr *)skb->data;
+ hdrlen = th->doff * 4;
+ shinfo = skb_shinfo(skb);
+
+ if (!shinfo->gso_size)
+ shinfo->gso_size = skb->len - hdrlen;
+
+ if (!shinfo->gso_segs)
+ shinfo->gso_segs = 1;
+
+ tail = sk->sk_backlog.tail;
+ if (tail &&
+ TCP_SKB_CB(tail)->end_seq == TCP_SKB_CB(skb)->seq &&
+#ifdef CONFIG_TLS_DEVICE
+ tail->decrypted == skb->decrypted &&
+#endif
+ !memcmp(tail->data + sizeof(*th), skb->data + sizeof(*th),
+ hdrlen - sizeof(*th))) {
+ bool fragstolen;
+ int delta;
+
+ __skb_pull(skb, hdrlen);
+ if (skb_try_coalesce(tail, skb, &fragstolen, &delta)) {
+ TCP_SKB_CB(tail)->end_seq = TCP_SKB_CB(skb)->end_seq;
+ TCP_SKB_CB(tail)->ack_seq = TCP_SKB_CB(skb)->ack_seq;
+ TCP_SKB_CB(tail)->tcp_flags |= TCP_SKB_CB(skb)->tcp_flags;
+
+ if (TCP_SKB_CB(skb)->has_rxtstamp) {
+ TCP_SKB_CB(tail)->has_rxtstamp = true;
+ tail->tstamp = skb->tstamp;
+ skb_hwtstamps(tail)->hwtstamp = skb_hwtstamps(skb)->hwtstamp;
+ }
+
+ /* Not as strict as GRO. We only need to carry mss max value */
+ skb_shinfo(tail)->gso_size = max(shinfo->gso_size,
+ skb_shinfo(tail)->gso_size);
+
+ skb_shinfo(tail)->gso_segs += shinfo->gso_segs;
+
+ sk->sk_backlog.len += delta;
+ __NET_INC_STATS(sock_net(sk),
+ LINUX_MIB_TCPBACKLOGCOALESCE);
+ kfree_skb_partial(skb, fragstolen);
+ return false;
+ }
+ __skb_push(skb, hdrlen);
+ }
+
+ /* Only socket owner can try to collapse/prune rx queues
+ * to reduce memory overhead, so add a little headroom here.
+ * Few sockets backlog are possibly concurrently non empty.
+ */
+ limit += 64*1024;
+
if (unlikely(sk_add_backlog(sk, skb, limit))) {
bh_unlock_sock(sk);
__NET_INC_STATS(sock_net(sk), LINUX_MIB_TCPBACKLOGDROP);
--
2.19.1.1215.g8438c0b245-goog
^ permalink raw reply related
* [PATCH net-next 3/3] tcp: implement head drops in backlog queue
From: Eric Dumazet @ 2018-11-21 17:52 UTC (permalink / raw)
To: David S . Miller
Cc: netdev, Jean-Louis Dupond, Neal Cardwell, Yuchung Cheng,
Eric Dumazet, Eric Dumazet
In-Reply-To: <20181121175240.6075-1-edumazet@google.com>
Under high stress, and if GRO or coalescing does not help,
we better make room in backlog queue to be able to keep latest
packet coming.
This generally helps fast recovery, given that we often receive
packets in order.
Signed-off-by: Eric Dumazet <edumazet@google.com>
Tested-by: Jean-Louis Dupond <jean-louis@dupond.be>
Cc: Neal Cardwell <ncardwell@google.com>
Cc: Yuchung Cheng <ycheng@google.com>
---
net/ipv4/tcp_ipv4.c | 14 ++++++++++++++
1 file changed, 14 insertions(+)
diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
index 401e1d1cb904a4c7963d8baa419cfbf178593344..36c9d715bf2aa7eb7bf58b045bfeb85a2ec1a696 100644
--- a/net/ipv4/tcp_ipv4.c
+++ b/net/ipv4/tcp_ipv4.c
@@ -1693,6 +1693,20 @@ bool tcp_add_backlog(struct sock *sk, struct sk_buff *skb)
__skb_push(skb, hdrlen);
}
+ while (sk_rcvqueues_full(sk, limit)) {
+ struct sk_buff *head;
+
+ head = sk->sk_backlog.head;
+ if (!head)
+ break;
+ sk->sk_backlog.head = head->next;
+ if (!head->next)
+ sk->sk_backlog.tail = NULL;
+ skb_mark_not_on_list(head);
+ sk->sk_backlog.len -= head->truesize;
+ kfree_skb(head);
+ __NET_INC_STATS(sock_net(sk), LINUX_MIB_TCPBACKLOGDROP);
+ }
/* Only socket owner can try to collapse/prune rx queues
* to reduce memory overhead, so add a little headroom here.
* Few sockets backlog are possibly concurrently non empty.
--
2.19.1.1215.g8438c0b245-goog
^ permalink raw reply related
* Re: [PATCH net] sctp: hold transport before accessing its asoc in sctp_hash_transport
From: Marcelo Ricardo Leitner @ 2018-11-21 17:53 UTC (permalink / raw)
To: Xin Long; +Cc: Neil Horman, network dev, linux-sctp, davem
In-Reply-To: <CADvbK_dChrmjeH9AcVvAkC6wsHnapvd+ddNF1-CxcuT02N=SCQ@mail.gmail.com>
On Wed, Nov 21, 2018 at 03:47:33PM +0900, Xin Long wrote:
> On Wed, Nov 21, 2018 at 9:46 AM Marcelo Ricardo Leitner
> <marcelo.leitner@gmail.com> wrote:
> >
> > On Tue, Nov 20, 2018 at 07:52:48AM -0500, Neil Horman wrote:
> > > On Tue, Nov 20, 2018 at 07:09:16PM +0800, Xin Long wrote:
> > > > In sctp_hash_transport, it dereferences a transport's asoc only under
> > > > rcu_read_lock. Without holding the transport, its asoc could be freed
> > > > already, which leads to a use-after-free panic.
> > > >
> > > > A similar fix as Commit bab1be79a516 ("sctp: hold transport before
> > > > accessing its asoc in sctp_transport_get_next") is needed to hold
> > > > the transport before accessing its asoc in sctp_hash_transport.
> > > >
> > > > Fixes: cd2b70875058 ("sctp: check duplicate node before inserting a new transport")
> > > > Reported-by: syzbot+0b05d8aa7cb185107483@syzkaller.appspotmail.com
> > > > Signed-off-by: Xin Long <lucien.xin@gmail.com>
> > > > ---
> > > > net/sctp/input.c | 7 ++++++-
> > > > 1 file changed, 6 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/net/sctp/input.c b/net/sctp/input.c
> > > > index 5c36a99..69584e9 100644
> > > > --- a/net/sctp/input.c
> > > > +++ b/net/sctp/input.c
> > > > @@ -896,11 +896,16 @@ int sctp_hash_transport(struct sctp_transport *t)
> > > > list = rhltable_lookup(&sctp_transport_hashtable, &arg,
> > > > sctp_hash_params);
> > > >
> > > > - rhl_for_each_entry_rcu(transport, tmp, list, node)
> > > > + rhl_for_each_entry_rcu(transport, tmp, list, node) {
> > > > + if (!sctp_transport_hold(transport))
> > > > + continue;
> > > > if (transport->asoc->ep == t->asoc->ep) {
> > > > + sctp_transport_put(transport);
> > > > rcu_read_unlock();
> > > > return -EEXIST;
> > > > }
> > > > + sctp_transport_put(transport);
> > > > + }
> > > > rcu_read_unlock();
> > > >
> > > > err = rhltable_insert_key(&sctp_transport_hashtable, &arg,
> > > > --
> > > > 2.1.0
> > > >
> > > >
> > >
> > > something doesn't feel at all right about this. If we are inserting a transport
> > > to an association, it would seem to me that we should have at least one user of
> > > the association (i.e. non-zero refcount). As such it seems something is wrong
> > > with the association refcount here. At the very least, if there is a case where
> > > an association is being removed while a transport is being added, the better
> > > solution would be to ensure that sctp_association_destroy goes through a
> > > quiescent point prior to unhashing transports from the list, to ensure that
> > > there is no conflict with the add operation above.
> Changing to do call_rcu(&transport->rcu, sctp_association_destroy) can
> work for this case.
> But it means asoc and socket (taking the port) will have to wait for a
> grace period, which is not expected. We seemed to have talked about
> this before, Marcelo?
Yes. This would cause it to linger longer and cause bind conflicts
meanwhile.
Note that we already have sctp_transport_destroy_rcu(), so this would
be a 2nd grace period.
>
> >
> > Consider that the rhl_for_each_entry_rcu() is traversing the global
> > rhashtable, and that it may operate on unrelated transports/asocs.
> > E.g., transport->asoc in the for() is potentially different from the
> > asoc under socket lock.
> >
> > The core of the fix is at:
> > + if (!sctp_transport_hold(transport))
> > + continue;
> > If we can get a hold, the asoc will be available for dereferencing in
> > subsequent lines. Otherwise, move on.
> >
> > With that, the patch makes sense to me.
> >
> > Although I would prefer if we come up with a better way to do this
> > jump, or even avoid the jump. We are only comparing pointers here and,
> > if we had asoc->ep cached on sctp_transport itself, we could avoid the
> > atomics here.
> Right, but it's another u64.
Strictly speaking, a pointer :-) (32bits, on 32bits archs)
But just an idea. It would cost one additional pointer per transport
but saves the atomics and also one extra dereference per iteration.
>
> >
> > This change, in the next patch on sctp_epaddr_lookup_transport, will
> > hurt performance as that is called in datapath. Rhashtable will help
> > on keeping entry lists to a size, but still.
> This loop is not long normally, will only a few atomic operations hurt
Right.
> a noticeable performance?
I guess we can't know without actually testing this.
^ permalink raw reply
* [PATCH net-next 0/2] net: bridge: add an option to disabe linklocal learning
From: Nikolay Aleksandrov @ 2018-11-22 4:29 UTC (permalink / raw)
To: netdev; +Cc: Nikolay Aleksandrov, andrew, roopa, bridge, davem
Hi,
This set adds a new bridge option which can control learning from
link-local packets, by default learning is on to be consistent and avoid
breaking users expectations. If the new no_linklocal_learn option is
enabled then the bridge will stop learning from link-local packets.
In order to save space for future boolean options, patch 01 adds a new
bool option API that uses a bitmask to control boolean options. The
bridge is by far the largest netlink attr user and we keep adding simple
boolean options which waste nl attr ids and space. We're not directly
mapping these to the in-kernel bridge flags because some might require
more complex configuration changes (e.g. if we were to add the per port
vlan stats now, it'd require multiple checks before changing value).
Any new bool option needs to be handled by both br_boolopt_toggle and get
in order to be able to retrieve its state later. All such options are
automatically exported via netlink. The behaviour of setting such
options is consistent with netlink option handling when a missing
option is being set (silently ignored), e.g. when a newer iproute2 is used
on older kernel.
Thanks,
Nik
Nikolay Aleksandrov (2):
net: bridge: add support for user-controlled bool options
net: bridge: add no_linklocal_learn bool option
include/uapi/linux/if_bridge.h | 21 ++++++++++
include/uapi/linux/if_link.h | 1 +
net/bridge/br.c | 72 ++++++++++++++++++++++++++++++++++
net/bridge/br_input.c | 4 +-
net/bridge/br_netlink.c | 17 +++++++-
net/bridge/br_private.h | 7 ++++
net/bridge/br_sysfs_br.c | 22 +++++++++++
net/core/rtnetlink.c | 2 +-
8 files changed, 143 insertions(+), 3 deletions(-)
--
2.17.2
^ permalink raw reply
* Re: selftests/bpf :get_cgroup_id_user: File not found: /sys/kernel/debug/tracing/events/syscalls/sys_enter_nanosleep/id
From: Y Song @ 2018-11-21 17:53 UTC (permalink / raw)
To: naresh.kamboju; +Cc: linux-kselftest, netdev, Yonghong Song, Daniel Borkmann
In-Reply-To: <CA+G9fYuruRUVVqR669UCB4vjAh5H==Ry3_sbFMQytMu+BZme7w@mail.gmail.com>
On Wed, Nov 21, 2018 at 3:44 AM Naresh Kamboju
<naresh.kamboju@linaro.org> wrote:
>
> Kselftest bpf get_cgroup_id_user is failed on all devices.
>
> selftests: bpf: get_cgroup_id_user
> main:PASS:setup_cgroup_environment
> main:PASS:create_and_get_cgroup
> main:PASS:join_cgroup
> main:PASS:bpf_prog_load
> main:PASS:bpf_find_map
> main:PASS:bpf_find_map
> main:FAIL:open err -1 errno 2
> not ok 1..15 selftests: bpf: get_cgroup_id_user [FAIL]
>
> The strace output shows,
> This expected file not found,
> "/sys/kernel/debug/tracing/events/syscalls/sys_enter_nanosleep/id"
>
> bpf(BPF_MAP_UPDATE_ELEM, {map_fd=5, key=0x7fff0c68c138,
> value=0x7fff0c68c13c, flags=BPF_ANY}, 72) = 0
> open(\"/sys/kernel/debug/tracing/events/syscalls/sys_enter_nanosleep/id\",
> O_RDONLY) = -1 ENOENT (No such file or directory)
> write(1, \"main:FAIL:open err -1 errno 2\n\", 30main:FAIL:open err -1 errno 2
>
> Am I missing any pre-requirement ?
You probably miss kernel config CONFIG_FTRACE_SYSCALLS.
>
> Best regards
> Naresh Kamboju
^ permalink raw reply
* Re: [PATCH net] net/sched: act_police: add missing spinlock initialization
From: Cong Wang @ 2018-11-21 18:02 UTC (permalink / raw)
To: Davide Caratti
Cc: Eric Dumazet, Jamal Hadi Salim, Jiri Pirko, David Miller,
Linux Kernel Network Developers, Ivan Vecera
In-Reply-To: <070f7a50d9fe76cb459b28d03fa322ce3dd59cb1.1542819002.git.dcaratti@redhat.com>
On Wed, Nov 21, 2018 at 9:24 AM Davide Caratti <dcaratti@redhat.com> wrote:
>
> commit f2cbd4852820 ("net/sched: act_police: fix race condition on state
> variables") introduces a new spinlock, but forgets its initialization.
> Ensure that tcf_police_init() initializes 'tcfp_lock' every time a 'police'
> action is newly created, to avoid the following lockdep splat:
Acked-by: Cong Wang <xiyou.wangcong@gmail.com>
^ permalink raw reply
* Re: [PATCH net-next 2/4] netns: add support of NETNSA_TARGET_NSID
From: David Ahern @ 2018-11-21 18:05 UTC (permalink / raw)
To: Nicolas Dichtel, davem; +Cc: netdev
In-Reply-To: <20181121110124.5501-3-nicolas.dichtel@6wind.com>
On 11/21/18 4:01 AM, Nicolas Dichtel wrote:
> static int rtnl_net_dumpid(struct sk_buff *skb, struct netlink_callback *cb)
> {
> - struct net *net = sock_net(skb->sk);
> struct rtnl_net_dump_cb net_cb = {
> - .net = net,
> + .tgt_net = sock_net(skb->sk),
> .skb = skb,
> .cb = cb,
> .idx = 0,
> .s_idx = cb->args[0],
> };
> + int err = 0;
>
> - if (cb->strict_check &&
> - nlmsg_attrlen(cb->nlh, sizeof(struct rtgenmsg))) {
> - NL_SET_ERR_MSG(cb->extack, "Unknown data in network namespace id dump request");
> - return -EINVAL;
> + if (cb->strict_check) {
> + err = rtnl_valid_dump_net_req(cb->nlh, skb->sk, &net_cb, cb);
> + if (err < 0)
> + goto end;
> }
>
> - spin_lock_bh(&net->nsid_lock);
> - idr_for_each(&net->netns_ids, rtnl_net_dumpid_one, &net_cb);
> - spin_unlock_bh(&net->nsid_lock);
> + spin_lock_bh(&net_cb.tgt_net->nsid_lock);
> + idr_for_each(&net_cb.tgt_net->netns_ids, rtnl_net_dumpid_one, &net_cb);
> + spin_unlock_bh(&net_cb.tgt_net->nsid_lock);
>
> cb->args[0] = net_cb.idx;
> - return skb->len;
> +end:
> + if (net_cb.ref_net)
> + put_net(net_cb.tgt_net);
That is going to lead to confusion -- you check that ref_net is set put
do a put on tgt_net. Other places using a target namespace use the nsid
as the key to whether a put_net is needed.
> + return err < 0 ? err : skb->len;
> }
>
> static void rtnl_net_notifyid(struct net *net, int cmd, int id)
>
^ permalink raw reply
* Re: [PATCH net v2] net/sched: act_police: fix race condition on state variables
From: Cong Wang @ 2018-11-21 18:05 UTC (permalink / raw)
To: Davide Caratti
Cc: Eric Dumazet, Jamal Hadi Salim, Jiri Pirko, David Miller,
Linux Kernel Network Developers, Ivan Vecera
In-Reply-To: <541a3b91af953326fa8f8fe046f0c284274377ff.1542748323.git.dcaratti@redhat.com>
On Tue, Nov 20, 2018 at 1:19 PM Davide Caratti <dcaratti@redhat.com> wrote:
> @@ -257,25 +261,28 @@ static int tcf_police_act(struct sk_buff *skb, const struct tc_action *a,
> }
>
> now = ktime_get_ns();
> - toks = min_t(s64, now - p->tcfp_t_c, p->tcfp_burst);
> + spin_lock_bh(&police->tcfp_lock);
> + toks = min_t(s64, now - police->tcfp_t_c, p->tcfp_burst);
I don't think you need to disable BH here.
^ permalink raw reply
* Re: [PATCH net-next 4/4] netns: enable to dump full nsid translation table
From: David Ahern @ 2018-11-21 18:09 UTC (permalink / raw)
To: Nicolas Dichtel, davem; +Cc: netdev
In-Reply-To: <20181121110124.5501-5-nicolas.dichtel@6wind.com>
On 11/21/18 4:01 AM, Nicolas Dichtel wrote:
> diff --git a/net/core/net_namespace.c b/net/core/net_namespace.c
> index 92730905886c..fc568cd0b560 100644
> --- a/net/core/net_namespace.c
> +++ b/net/core/net_namespace.c
> @@ -740,7 +740,7 @@ static int rtnl_net_get_size(void)
> }
>
> static int rtnl_net_fill(struct sk_buff *skb, u32 portid, u32 seq, int flags,
> - int cmd, int nsid)
> + int cmd, int nsid, bool add_ref, int ref_nsid)
> {
> struct nlmsghdr *nlh;
> struct rtgenmsg *rth;
> @@ -755,6 +755,9 @@ static int rtnl_net_fill(struct sk_buff *skb, u32 portid, u32 seq, int flags,
> if (nla_put_s32(skb, NETNSA_NSID, nsid))
> goto nla_put_failure;
>
> + if (add_ref && nla_put_s32(skb, NETNSA_CURRENT_NSID, ref_nsid))
> + goto nla_put_failure;
Why not use ref_nsid >= 0 and drop the add_ref argument?
^ permalink raw reply
* Re: [Patch net-next 2/2] net: dump whole skb data in netdev_rx_csum_fault()
From: Cong Wang @ 2018-11-21 18:17 UTC (permalink / raw)
To: Eric Dumazet
Cc: Linux Kernel Network Developers, Herbert Xu, Eric Dumazet,
David Miller
In-Reply-To: <a1e1b48b-68af-16f6-2442-b96718f6ae00@gmail.com>
On Wed, Nov 21, 2018 at 5:05 AM Eric Dumazet <eric.dumazet@gmail.com> wrote:
>
>
>
> On 11/20/2018 06:13 PM, Cong Wang wrote:
> > Currently, we only dump a few selected skb fields in
> > netdev_rx_csum_fault(). It is not suffient for debugging checksum
> > fault. This patch introduces skb_dump() which dumps skb mac header,
> > network header and its whole skb->data too.
> >
> > Cc: Herbert Xu <herbert@gondor.apana.org.au>
> > Cc: Eric Dumazet <edumazet@google.com>
> > Cc: David Miller <davem@davemloft.net>
> > Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
> > ---
>
>
> > + print_hex_dump(level, "skb data: ", DUMP_PREFIX_OFFSET, 16, 1,
> > + skb->data, skb->len, false);
>
> As I mentioned to David, we want all the bytes that were maybe already pulled
>
> (skb->head starting point, not skb->data)
Hmm, with mac header and network header, it is effectively from skb->head, no?
Is there anything between skb->head and mac header?
>
> Also we will miss the trimmed bytes if there were padding data.
> And it seems the various bugs we have are all tied to the pulled or trimmed bytes.
>
Unless I miss something, the tailing padding data should be in range
[iphdr->tot_len, skb->len]. No?
Thanks
^ permalink raw reply
* Re: [Patch net-next 2/2] net: dump whole skb data in netdev_rx_csum_fault()
From: Eric Dumazet @ 2018-11-21 18:26 UTC (permalink / raw)
To: Cong Wang; +Cc: Eric Dumazet, netdev, Herbert Xu, David Miller
In-Reply-To: <CAM_iQpXO1Etf0489ssLoGGg9bgtZWuYSYb25Si913aOi5pgsBQ@mail.gmail.com>
On Wed, Nov 21, 2018 at 10:17 AM Cong Wang <xiyou.wangcong@gmail.com> wrote:
>
> On Wed, Nov 21, 2018 at 5:05 AM Eric Dumazet <eric.dumazet@gmail.com> wrote:
> >
> >
> >
> > On 11/20/2018 06:13 PM, Cong Wang wrote:
> > > Currently, we only dump a few selected skb fields in
> > > netdev_rx_csum_fault(). It is not suffient for debugging checksum
> > > fault. This patch introduces skb_dump() which dumps skb mac header,
> > > network header and its whole skb->data too.
> > >
> > > Cc: Herbert Xu <herbert@gondor.apana.org.au>
> > > Cc: Eric Dumazet <edumazet@google.com>
> > > Cc: David Miller <davem@davemloft.net>
> > > Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
> > > ---
> >
> >
> > > + print_hex_dump(level, "skb data: ", DUMP_PREFIX_OFFSET, 16, 1,
> > > + skb->data, skb->len, false);
> >
> > As I mentioned to David, we want all the bytes that were maybe already pulled
> >
> > (skb->head starting point, not skb->data)
>
> Hmm, with mac header and network header, it is effectively from skb->head, no?
> Is there anything between skb->head and mac header?
Oh, I guess we wanted a single hex dump, or we need some user program
to be able to
rebuild from different memory zones the original CHECKSUM_COMPLETE value.
>
> >
> > Also we will miss the trimmed bytes if there were padding data.
> > And it seems the various bugs we have are all tied to the pulled or trimmed bytes.
> >
>
> Unless I miss something, the tailing padding data should be in range
> [iphdr->tot_len, skb->len]. No?
Not after we did the pskb_trim_rcsum() call, since it has effectively
reduced skb->len by the number of padding bytes.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox