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=-6.7 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,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 608D8C433ED for ; Sat, 8 May 2021 03:05:22 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 444B0613C5 for ; Sat, 8 May 2021 03:05:22 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230431AbhEHDGU (ORCPT ); Fri, 7 May 2021 23:06:20 -0400 Received: from mail.kernel.org ([198.145.29.99]:33584 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230399AbhEHDGR (ORCPT ); Fri, 7 May 2021 23:06:17 -0400 Received: by mail.kernel.org (Postfix) with ESMTPSA id AF4646112F; Sat, 8 May 2021 03:05:15 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1620443117; bh=UCWz0d1P680+8OLROOU7w4JlmYYVS6cncSAWi1+HSM4=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=VdR3exQSHYmPJOJ9LOTeCOOKI6FLRVGU6O/onZW5RqvCHiAZHqIvcnIT8NOv0nYJA GUlWOpc83MHytIbWhBIJ+C97V6uv2sWWSldCJl3Kmrd+yCWwaoBGON3QCe71QYsh1R sbQRPVwgdv1a+QBfJ78zQnPEUdU+VhmoiW/B6XX0RDwY2aL+sZXyCk2YuAhHcCUHDr 4/LSS647o+3VlFBcgvRtkRXBjomRp/T3DTGll4l9Ml28Dux7KWjlgUdbVK1vAhkOyL j6xnlYDC7eS8OZLb0usW/YJ2GyrCYhTvk94kBMNFU8IWwAyq+FUon1nYHACKY0rVXw hb9pgAznglEzw== Date: Fri, 7 May 2021 20:05:14 -0700 From: Jakub Kicinski To: Yunsheng Lin Cc: , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , Subject: Re: [PATCH net v5 1/3] net: sched: fix packet stuck problem for lockless qdisc Message-ID: <20210507200514.0567ef27@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com> In-Reply-To: <83ff1619-5ca3-1e60-3a41-0faff1566569@huawei.com> References: <1620266264-48109-1-git-send-email-linyunsheng@huawei.com> <1620266264-48109-2-git-send-email-linyunsheng@huawei.com> <20210507165703.70771c55@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com> <83ff1619-5ca3-1e60-3a41-0faff1566569@huawei.com> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Sat, 8 May 2021 10:55:19 +0800 Yunsheng Lin wrote: > >> + * the flag set after releasing lock and reschedule the > >> + * net_tx_action() to do the dequeuing. > > > > I don't understand why MISSED is checked before the trylock. > > Could you explain why it can't be tested directly here? > The initial thinking was: > Just like the set_bit() before the second trylock, If MISSED is set > before first trylock, it means other thread has set the MISSED flag > for this thread before doing the first trylock, so that this thread > does not need to do the set_bit(). > > But the initial thinking seems over thinking, as thread 3' setting the > MISSED before the second trylock has ensure either thread 3' second > trylock returns ture or thread 2 holding the lock will see the MISSED > flag, so thread 1 can do the test_bit() before or after the first > trylock, as below: > > thread 1 thread 2 thread 3 > holding q->seqlock > first trylock failed first trylock failed > unlock q->seqlock > test_bit(MISSED) return false > test_bit(MISSED) return false > and not reschedule > set_bit(MISSED) > trylock success > test_bit(MISSED) retun ture > and not retry second trylock > > If the above is correct, it seems we could: > 1. do test_bit(MISSED) before the first trylock to avoid doing the > first trylock for contended case. > or > 2. do test_bit(MISSED) after the first trylock to avoid doing the > test_bit() for un-contended case. > > Which one do you prefer? No strong preference but testing after the trylock seems more obvious as it saves the temporary variable. For the contended case could we potentially move or add a MISSED test before even the first try_lock()? I'm not good at optimizing things, but it could save us the atomic op, right? (at least on x86)