From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-0.8 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 59031C3A5A6 for ; Thu, 19 Sep 2019 19:14:00 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 2C02C20678 for ; Thu, 19 Sep 2019 19:14:00 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="CZGSHDsy" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S2392327AbfISTN7 (ORCPT ); Thu, 19 Sep 2019 15:13:59 -0400 Received: from mail-pl1-f195.google.com ([209.85.214.195]:44090 "EHLO mail-pl1-f195.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S2390859AbfISTN7 (ORCPT ); Thu, 19 Sep 2019 15:13:59 -0400 Received: by mail-pl1-f195.google.com with SMTP id q15so2033585pll.11 for ; Thu, 19 Sep 2019 12:13:58 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=W5DGy7JzEyGPCwlKkv2bH+TeKMC3s+WOSUvOMWryOKo=; b=CZGSHDsyChkg94i3PwCNrEuh54m2C9yWMOe+Ot5zxXF6gWdfk2VSYvB0LXvhnxNFZZ f3xT+E/yWSrvRIJkQ02VqcP0WdaPlC+2LOoT4X4UMRjKTWtHAXkTH5s/D39V2/LU9sTC 6MVeaZWfxVv1gBEq30XNe+RiLQh8bhzdFVHUA1y3V4ozMIgrkbFsRbjhFPH7FBOXnHJN VcYUiAoYj926VD9YVBIaHuXq6oeSdv6RJ3382aHKo9XOZANwBWGxsc6C6TZtJXvG0JwV jgqWeaOcitouwqUc6TS6/MkxYtK69oY4kCm2VQ/qujpUycX4l3aCXg73jmDZg2+DXVyC 0E0A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=W5DGy7JzEyGPCwlKkv2bH+TeKMC3s+WOSUvOMWryOKo=; b=NBW1yLZKqkQ0LRRtheFZO1STXfO3gMMeQu9rY2icstemD9C8h3bt/XpaNxU8te1UIP vwq0IjumtnZ+PNyKqnNZUtSTtr06+JP+luJTcA831wzL6BzxnCKoCW/mw1fm/JLJ/I+S rbDSwK5/qvoo8CC8fzBzCBPA+VCLZakG5rUexE+TQFq6PV0q9QR5wfkpQ/HSt4KRENXs 1Zs1AP1v29zqyxd4m5vQtgYiegOOk1uv8Litt9J1XFbss/mnV0MsGgHZP9XISC8G5wqE lWBL1sbVR1XWFkgVqtI9ysihXhGrlnXKKNkHJzUak4/0zrCeqmYpctKLLNXRNJgg4OAU IoIg== X-Gm-Message-State: APjAAAXOcG4krqgQsEZA5Ds4s1iJoAYrfU5vY9TvCPJg1wAoZqzNeOE+ qvC31ccN+/G0oVdIrSSEMRPgnzsj4AkIXdAG4QE= X-Google-Smtp-Source: APXvYqzvVu0gMDS7qsipBBTDp0Mj8hH7AYBd71WN6kvnfWhvG3+yJK3aEx259hya/f3eXG2KFewaF6ayGZyrUkKEICY= X-Received: by 2002:a17:902:a586:: with SMTP id az6mr11959180plb.12.1568920438031; Thu, 19 Sep 2019 12:13:58 -0700 (PDT) MIME-Version: 1.0 References: <20190918073201.2320-1-vladbu@mellanox.com> In-Reply-To: From: Cong Wang Date: Thu, 19 Sep 2019 12:13:46 -0700 Message-ID: Subject: Re: [PATCH net 0/3] Fix Qdisc destroy issues caused by adding fine-grained locking to filter API To: Vlad Buslov Cc: Linux Kernel Network Developers , Jamal Hadi Salim , Jiri Pirko , David Miller Content-Type: text/plain; charset="UTF-8" Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org On Thu, Sep 19, 2019 at 1:53 AM Vlad Buslov wrote: > > > On Thu 19 Sep 2019 at 01:50, Cong Wang wrote: > > On Wed, Sep 18, 2019 at 12:32 AM Vlad Buslov wrote: > >> > >> TC filter API unlocking introduced several new fine-grained locks. The > >> change caused sleeping-while-atomic BUGs in several Qdiscs that call cls > >> APIs which need to obtain new mutex while holding sch tree spinlock. This > >> series fixes affected Qdiscs by ensuring that cls API that became sleeping > >> is only called outside of sch tree lock critical section. > > > > Sorry I just took a deeper look. It seems harder than just moving it > > out of the critical section. > > > > qdisc_destroy() calls ops->reset() which usually purges queues, > > I don't see how it is safe to move it out of tree spinlock without > > respecting fast path. > > > > What do you think? > > Hmm, maybe we can split qdisc destruction in two stage process for > affected qdiscs? Rough sketch: > > 1. Call qdisc_reset() (or qdisc_purge_queue()) on qdisc that are being > deleted under sch tree lock protection. > > 2. Call new qdisc_put_empty() function after releasing the lock. This > function would implement same functionality as a regular qdisc_put() > besides resetting the Qdisc and freeing skb in its queues (already > done by qdisc_reset()) > > In fact, affected queues already do the same or something similar: > > - htb_change_class() calls qdisc_purge_queue() that calls qdisc_reset(), > which makes reset inside qdisc_destroy() redundant. > > - multiq_tune() calls qdisc_tree_flush_backlog() that has the same > implementation as qdisc_purge_queue() minus actually resetting the > Qdisc. Can we substitute first function with the second one here? > > - sfb_change() - same as multiq_tune(). > > Do you think that would work? I think they have to call qdisc_purge_queue() or whatever that calls qdisc_reset() to reset all queues including qdisc->gso_skb and qdisc->skb_bad_txq before releasing sch tree lock. qdisc_tree_flush_backlog() is not sufficient.