From mboxrd@z Thu Jan 1 00:00:00 1970 From: John Fastabend Subject: Re: [PATCH] net: Revert "net_sched: no need to free qdisc in RCU callback" Date: Wed, 20 Dec 2017 15:40:49 -0800 Message-ID: <1fe575ba-6206-1795-9478-abdf05457c98@gmail.com> References: <20171220200919.6233.48192.stgit@john-Precision-Tower-5810> <20171220135959.3ff075ac@cakuba.netronome.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Cc: xiyou.wangcong@gmail.com, jiri@resnulli.us, davem@davemloft.net, netdev@vger.kernel.org, eric.dumazet@gmail.com To: Jakub Kicinski Return-path: Received: from mail-pg0-f67.google.com ([74.125.83.67]:43576 "EHLO mail-pg0-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755846AbdLTXlE (ORCPT ); Wed, 20 Dec 2017 18:41:04 -0500 Received: by mail-pg0-f67.google.com with SMTP id b18so12530192pgv.10 for ; Wed, 20 Dec 2017 15:41:04 -0800 (PST) In-Reply-To: <20171220135959.3ff075ac@cakuba.netronome.com> Content-Language: en-US Sender: netdev-owner@vger.kernel.org List-ID: On 12/20/2017 01:59 PM, Jakub Kicinski wrote: > On Wed, 20 Dec 2017 12:09:19 -0800, John Fastabend wrote: >> RCU grace period is needed for lockless qdiscs added in the commit >> c5ad119fb6c09 ("net: sched: pfifo_fast use skb_array"). >> >> It is needed now that qdiscs may be lockless otherwise we risk >> free'ing a qdisc that is still in use from datapath. Additionally, >> push list cleanup into RCU callback. Otherwise we risk the datapath >> adding skbs during removal. >> >> Fixes: c5ad119fb6c09 ("net: sched: pfifo_fast use skb_array") >> Signed-off-by: John Fastabend > > Seems like this revert may be too heavy handed: > > # ./tools/testing/selftests/bpf/test_offload.py --log /tmp/log > Test destruction of generic XDP... > Test TC non-offloaded... > Test TC non-offloaded isn't getting bound... > Test TC offloads are off by default... > Test TC offload by default... > Test TC cBPF bytcode tries offload by default... > Test TC cBPF unbound bytecode doesn't offload... > Test TC offloads work... > FAIL: TC filter did not load with TC offloads enabled > > And it's triggering: > > WARNING: CPU: 15 PID: 1853 at ../drivers/net/netdevsim/bpf.c:372 nsim_bpf_uninit+0x2e/0x41 [netdevsim] > > Which is: > > 368 void nsim_bpf_uninit(struct netdevsim *ns) > 369 { > 370 WARN_ON(!list_empty(&ns->bpf_bound_progs)); > 371 WARN_ON(ns->xdp_prog); >>> 372 WARN_ON(ns->bpf_offloaded); > 373 } > > (Meaning the offload was not stopped by the stack before ndo_uninit.) > Dang. So offload code depends on destroy being called on a qdisc to in turn destroy the filters and unbind any offloads. I was hoping I could get away with tearing down live qdiscs without too much work. Looks like not. Note the fixes tag was bogus nothing is actually broken in current code until a lockless qdisc with classes shows up. .John