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:05:11 -0800 Message-ID: References: <20171220200919.6233.48192.stgit@john-Precision-Tower-5810> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Cc: Jiri Pirko , David Miller , Jakub Kicinski , Linux Kernel Network Developers , Eric Dumazet To: Cong Wang Return-path: Received: from mail-pl0-f66.google.com ([209.85.160.66]:45283 "EHLO mail-pl0-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755691AbdLTXF2 (ORCPT ); Wed, 20 Dec 2017 18:05:28 -0500 Received: by mail-pl0-f66.google.com with SMTP id o2so9879789plk.12 for ; Wed, 20 Dec 2017 15:05:27 -0800 (PST) In-Reply-To: Content-Language: en-US Sender: netdev-owner@vger.kernel.org List-ID: On 12/20/2017 02:41 PM, Cong Wang wrote: > On Wed, Dec 20, 2017 at 12:09 PM, 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. > > What about qdisc_graft() -> dev_deactivate() -> synchronize_net() ? > It doesn't work with your "lockless" patches? > Well this is only in the 'parent == NULL' case otherwise we call cops->graft(). Most sch_* seem to use qdisc_replace and this uses sch_tree_lock(). The only converted qdisc mq and mqprio at this point don't care though and do their own dev_deactivate/activate. So its not fixing anything in the above mentioned commit. I still think it will need to be done eventually. If it resolves the miniq case it seems like a good idea. Although per Jakub's comment perhaps I pulled too much into the RCU handler.