| Message ID | 20230110091409.2962-1-sensor1010@163.com |
|---|---|
| State | New |
| Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org>
Delivered-To: ouuuleilei@gmail.com
Received: by 2002:a5d:4e01:0:0:0:0:0 with SMTP id p1csp2645667wrt;
Tue, 10 Jan 2023 01:29:22 -0800 (PST)
X-Google-Smtp-Source:
AMrXdXt0xTVQvvaI1MVUGUXDaDUFVpKKl6jZmOfIZq1t8xH8A1w255zRS0z1H0FJZHZR9nyPbOoc
X-Received: by 2002:a17:907:1759:b0:7ad:d250:b903 with SMTP id
lf25-20020a170907175900b007add250b903mr74286530ejc.56.1673342961854;
Tue, 10 Jan 2023 01:29:21 -0800 (PST)
ARC-Seal: i=1; a=rsa-sha256; t=1673342961; cv=none;
d=google.com; s=arc-20160816;
b=zGr2+1/PX4z05gNPveozykhK4fCpcvTDbGr9PsX5vd06NWCl6sw0fgu1OAOf6gx40n
NR5a4xnm0xZHUrMxKrwjMbDicZJGoPqAhir9WwOA+TmijCJBGYGQxyjgUZWtkcMjKgGe
b5G7o5tx6ZbZTrRXc11sMH8VMt5BzJlRupf1OEIpbKiGC2NsLeYGIpwXcpL1ZDWVcNTg
DBT3Ep5RzNsmpVtV184oDAs+3WLVPTp5el6HMMKFblSmwKTVVkaCNuTgrB3pFHfXLlSI
yGa6yB390s8FjaLF+fCIcXDIoR9XbCUR4PuiSBnpH42SuNRbytMCY3GufV0Km/Q0BNGr
kLqw==
ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com;
s=arc-20160816;
h=list-id:precedence:content-transfer-encoding:mime-version
:message-id:date:subject:cc:to:from:dkim-signature;
bh=Em2JFVD2u/iA9XuWQueDH1iR1dHDBEeEW9F0fobBqK8=;
b=yigyLx+pVOVamrARv/YQvSkL2/WvGKV4WwAIv5OaJ0H4DLM0QdUJmgwKjah0tPz6mx
xCtZyi8pEGTZCpJZKgdvXN1M7uEjobOpmkZEhWfPl/C4P3btrhId3Ih2+tYNNATzwoqv
xeDER+mhNc8BX4Y2frcLNCTjPnSQrch9j3WJHpL7vxPHVLpne7dW1AYji8TBr5rDwiAZ
AOvdDejOLVtpzVfbdLDdb2QLWs9EinERnhoQMJ6L3AErg0+SYqfRlMxo2OyaWnhZzOdF
Ee5ZPbkYeyXEhNpEkJhaI26XMdaVYzkWHiIRerkIcG8wHxeWFGXEbPERKSJ02is1p1Da
YEKw==
ARC-Authentication-Results: i=1; mx.google.com;
dkim=pass header.i=@163.com header.s=s110527 header.b=o036YDsC;
spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org
designates 2620:137:e000::1:20 as permitted sender)
smtp.mailfrom=linux-kernel-owner@vger.kernel.org;
dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=163.com
Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20])
by mx.google.com with ESMTP id
hv15-20020a17090760cf00b007c0d40cd759si12144778ejc.827.2023.01.10.01.28.58;
Tue, 10 Jan 2023 01:29:21 -0800 (PST)
Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org
designates 2620:137:e000::1:20 as permitted sender)
client-ip=2620:137:e000::1:20;
Authentication-Results: mx.google.com;
dkim=pass header.i=@163.com header.s=s110527 header.b=o036YDsC;
spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org
designates 2620:137:e000::1:20 as permitted sender)
smtp.mailfrom=linux-kernel-owner@vger.kernel.org;
dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=163.com
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
id S231991AbjAJJQq (ORCPT <rfc822;syz17693488234@gmail.com>
+ 99 others); Tue, 10 Jan 2023 04:16:46 -0500
Received: from lindbergh.monkeyblade.net ([23.128.96.19]:32816 "EHLO
lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
with ESMTP id S231902AbjAJJQO (ORCPT
<rfc822;linux-kernel@vger.kernel.org>);
Tue, 10 Jan 2023 04:16:14 -0500
Received: from m12.mail.163.com (m12.mail.163.com [220.181.12.197])
by lindbergh.monkeyblade.net (Postfix) with ESMTP id 236D150F4B;
Tue, 10 Jan 2023 01:16:04 -0800 (PST)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=163.com;
s=s110527; h=From:Subject:Date:Message-Id:MIME-Version:
Content-Type; bh=Em2JFVD2u/iA9XuWQueDH1iR1dHDBEeEW9F0fobBqK8=;
b=o036YDsCb1nERLIPl3fNaHZoESyd6FhjyfxtmFYZcr1K/bMOfgT/t/nxk89u5f
VeaFd6Vuxi8HLS9jC4FDbEKgCQx9DYxy97MVZKscY7RPTt+lHwJtHJ96mDHSvBbi
INnkDtkkXT8zPI8LpbS09cFCp2+Rk8XWM8X8JSHhMf958=
Received: from localhost.localdomain (unknown [114.107.205.23])
by zwqz-smtp-mta-g2-0 (Coremail) with SMTP id
_____wDXwC1kLL1j4dlPAA--.32863S4;
Tue, 10 Jan 2023 17:14:47 +0800 (CST)
From: =?utf-8?b?5p2O5ZOy?= <sensor1010@163.com>
To: davem@davemloft.net, edumazet@google.com, kuba@kernel.org,
pabeni@redhat.com, bigeasy@linutronix.de, imagedong@tencent.com,
kuniyu@amazon.com, petrm@nvidia.com
Cc: netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
=?utf-8?b?5p2O5ZOy?= <sensor1010@163.com>
Subject: [PATCH v1] net/dev.c : Remove redundant state settings after waking
up
Date: Tue, 10 Jan 2023 01:14:09 -0800
Message-Id: <20230110091409.2962-1-sensor1010@163.com>
X-Mailer: git-send-email 2.17.1
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
X-CM-TRANSID: _____wDXwC1kLL1j4dlPAA--.32863S4
X-Coremail-Antispam: 1Uf129KBjDUn29KB7ZKAUJUUUUU529EdanIXcx71UUUUU7v73
VFW2AGmfu7bjvjm3AaLaJ3UbIYCTnIWIevJa73UjIFyTuYvj4R9_-mUUUUU
X-Originating-IP: [114.107.205.23]
X-CM-SenderInfo: 5vhq20jurqiii6rwjhhfrp/xtbBogzyq1aEHGxrVAAAsq
X-Spam-Status: No, score=-1.9 required=5.0 tests=BAYES_00,DKIM_SIGNED,
DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_ENVFROM_END_DIGIT,
FREEMAIL_FROM,RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_PASS autolearn=ham
autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?=
X-GMAIL-THRID: =?utf-8?q?1754627269404560119?=
X-GMAIL-MSGID: =?utf-8?q?1754627269404560119?=
|
| Series |
[v1] net/dev.c : Remove redundant state settings after waking up
|
|
Commit Message
Lizhe
Jan. 10, 2023, 9:14 a.m. UTC
the task status has been set to TASK_RUNNING in shcedule(),
no need to set again here
Signed-off-by: 李哲 <sensor1010@163.com>
---
net/core/dev.c | 1 -
1 file changed, 1 deletion(-)
Comments
On Tue, Jan 10, 2023 at 10:15 AM 李哲 <sensor1010@163.com> wrote: > > the task status has been set to TASK_RUNNING in shcedule(), > no need to set again here Changelog is rather confusing, this does not match the patch, which removes one set_current_state(TASK_INTERRUPTIBLE); TASK_INTERRUPTIBLE != TASK_RUNNING Patch itself looks okay (but has nothing to do with thread state after schedule()), you should have CC Wei Wang because she authored commit cb038357937e net: fix race between napi kthread mode and busy poll > > Signed-off-by: 李哲 <sensor1010@163.com> > --- > net/core/dev.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/net/core/dev.c b/net/core/dev.c > index b76fb37b381e..4bd2d4b954c9 100644 > --- a/net/core/dev.c > +++ b/net/core/dev.c > @@ -6580,7 +6580,6 @@ static int napi_thread_wait(struct napi_struct *napi) > schedule(); > /* woken being true indicates this thread owns this napi. */ > woken = true; > - set_current_state(TASK_INTERRUPTIBLE); > } > __set_current_state(TASK_RUNNING); > > -- > 2.17.1 >
On Tue, 10 Jan 2023 10:29:20 +0100 Eric Dumazet wrote: > > the task status has been set to TASK_RUNNING in shcedule(), > > no need to set again here > > Changelog is rather confusing, this does not match the patch, which > removes one set_current_state(TASK_INTERRUPTIBLE); > > TASK_INTERRUPTIBLE != TASK_RUNNING > > Patch itself looks okay (but has nothing to do with thread state after > schedule()), > you should have CC Wei Wang because she > authored commit cb038357937e net: fix race between napi kthread mode > and busy poll AFAIU this is the semi-idiomatic way of handling wait loops. It's not schedule() that may set the task state to TASK_RUNNING, it's whoever wakes the process and makes the "wait condition" true. In this case - test_bit(NAPI_STATE_SCHED, &napi->state) I vote to not futz with this logic.
I was not able to see the entire changelog, but I don't think > - set_current_state(TASK_INTERRUPTIBLE); is redundant. It makes sure that if the previous if statement: if (test_bit(NAPI_STATE_SCHED_THREADED, &napi->state) || woken) is false, this napi thread yields the CPU to other threads waiting to be run by calling schedule(). And the napi thread gets into running state again after the next wake_up_process() is called from ____napi_schedule(). On Tue, Jan 10, 2023 at 4:30 PM Jakub Kicinski <kuba@kernel.org> wrote: > > On Tue, 10 Jan 2023 10:29:20 +0100 Eric Dumazet wrote: > > > the task status has been set to TASK_RUNNING in shcedule(), > > > no need to set again here > > > > Changelog is rather confusing, this does not match the patch, which > > removes one set_current_state(TASK_INTERRUPTIBLE); > > > > TASK_INTERRUPTIBLE != TASK_RUNNING > > > > Patch itself looks okay (but has nothing to do with thread state after > > schedule()), > > you should have CC Wei Wang because she > > authored commit cb038357937e net: fix race between napi kthread mode > > and busy poll > > AFAIU this is the semi-idiomatic way of handling wait loops. > It's not schedule() that may set the task state to TASK_RUNNING, > it's whoever wakes the process and makes the "wait condition" true. > In this case - test_bit(NAPI_STATE_SCHED, &napi->state) > > I vote to not futz with this logic.
On 2023-01-10 16:42:56 [-0800], Wei Wang wrote: > I was not able to see the entire changelog, but I don't think > > - set_current_state(TASK_INTERRUPTIBLE); > is redundant. > > It makes sure that if the previous if statement: > if (test_bit(NAPI_STATE_SCHED_THREADED, &napi->state) || woken) > is false, this napi thread yields the CPU to other threads waiting to > be run by calling schedule(). It made sense in the beginning but now the suggested patch is a clean up. First the `woken' parameter was added in commit cb038357937ee ("net: fix race between napi kthread mode and busy poll") and then the `napi_disable_pending' check was removed in commit 27f0ad71699de ("net: fix hangup on napi_disable for threaded napi") which renders the code to: | while (!kthread_should_stop()) { | if (test_bit(NAPI_STATE_SCHED_THREADED, &napi->state) || woken) { | WARN_ON(!list_empty(&napi->poll_list)); | __set_current_state(TASK_RUNNING); | return 0; | } | | schedule(); | /* woken being true indicates this thread owns this napi. */ | woken = true; | set_current_state(TASK_INTERRUPTIBLE); | } | __set_current_state(TASK_RUNNING); so when you get out of schedule() woken is set and even if NAPI_STATE_SCHED_THREADED is not set, the while() loop is left due to `woken = true'. So changing state to TASK_INTERRUPTIBLE makes no sense since it will be set back to TASK_RUNNING cycles later. Sebastian
On Wed, 11 Jan 2023 08:32:42 +0100 Sebastian Andrzej Siewior wrote: > It made sense in the beginning but now the suggested patch is a clean > up. First the `woken' parameter was added in commit > cb038357937ee ("net: fix race between napi kthread mode and busy poll") > > and then the `napi_disable_pending' check was removed in commit > 27f0ad71699de ("net: fix hangup on napi_disable for threaded napi") > > which renders the code to: > | while (!kthread_should_stop()) { > | if (test_bit(NAPI_STATE_SCHED_THREADED, &napi->state) || woken) { > | WARN_ON(!list_empty(&napi->poll_list)); > | __set_current_state(TASK_RUNNING); > | return 0; > | } > | > | schedule(); > | /* woken being true indicates this thread owns this napi. */ > | woken = true; > | set_current_state(TASK_INTERRUPTIBLE); > | } > | __set_current_state(TASK_RUNNING); > > so when you get out of schedule() woken is set and even if > NAPI_STATE_SCHED_THREADED is not set, the while() loop is left due to > `woken = true'. So changing state to TASK_INTERRUPTIBLE makes no sense > since it will be set back to TASK_RUNNING cycles later. Ah, fair point, forgot about the woken optimization.
On Wed, Jan 11, 2023 at 10:21 AM Jakub Kicinski <kuba@kernel.org> wrote: > > On Wed, 11 Jan 2023 08:32:42 +0100 Sebastian Andrzej Siewior wrote: > > It made sense in the beginning but now the suggested patch is a clean > > up. First the `woken' parameter was added in commit > > cb038357937ee ("net: fix race between napi kthread mode and busy poll") > > > > and then the `napi_disable_pending' check was removed in commit > > 27f0ad71699de ("net: fix hangup on napi_disable for threaded napi") > > > > which renders the code to: > > | while (!kthread_should_stop()) { > > | if (test_bit(NAPI_STATE_SCHED_THREADED, &napi->state) || woken) { > > | WARN_ON(!list_empty(&napi->poll_list)); > > | __set_current_state(TASK_RUNNING); > > | return 0; > > | } > > | > > | schedule(); > > | /* woken being true indicates this thread owns this napi. */ > > | woken = true; > > | set_current_state(TASK_INTERRUPTIBLE); > > | } > > | __set_current_state(TASK_RUNNING); > > > > so when you get out of schedule() woken is set and even if > > NAPI_STATE_SCHED_THREADED is not set, the while() loop is left due to > > `woken = true'. So changing state to TASK_INTERRUPTIBLE makes no sense > > since it will be set back to TASK_RUNNING cycles later. > > Ah, fair point, forgot about the woken optimization. Agree. I think it is OK to remove this, since woken is set, and this function will set TASK_RUNNING and return 0. And the next time, it will enter with TASK_INTERRUPTIBLE and woken = false.
On Wed, 1 Mar 2023 23:32:50 +0800 (CST) lizhe wrote: > HI : > if want to merge this patch into the main line, what should i do ? Stop sending HTML emails please, the list only accepts plain text. Read this: https://www.kernel.org/doc/html/latest/process/maintainer-netdev.html Improve the commit message and the subject line (look at other related commits to find examples). Repost when appropriate (net-next is closed).
diff --git a/net/core/dev.c b/net/core/dev.c index b76fb37b381e..4bd2d4b954c9 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -6580,7 +6580,6 @@ static int napi_thread_wait(struct napi_struct *napi) schedule(); /* woken being true indicates this thread owns this napi. */ woken = true; - set_current_state(TASK_INTERRUPTIBLE); } __set_current_state(TASK_RUNNING);