Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933181AbcDKNiZ (ORCPT ); Mon, 11 Apr 2016 09:38:25 -0400 Received: from bastet.se.axis.com ([195.60.68.11]:37345 "EHLO bastet.se.axis.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932583AbcDKNiV (ORCPT ); Mon, 11 Apr 2016 09:38:21 -0400 Message-ID: <570BA8C7.1000905@axis.com> Date: Mon, 11 Apr 2016 15:38:15 +0200 From: Lars Persson User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Icedove/31.8.0 MIME-Version: 1.0 To: Eric Dumazet , Lars Persson CC: , , , Subject: Re: [PATCH net v2] net: sched: do not requeue a NULL skb References: <1460355869-13539-1-git-send-email-larper@axis.com> <1460380981.6473.544.camel@edumazet-glaptop3.roam.corp.google.com> In-Reply-To: <1460380981.6473.544.camel@edumazet-glaptop3.roam.corp.google.com> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit X-Originating-IP: [10.0.5.55] X-ClientProxiedBy: XBOX02.axis.com (10.0.5.16) To XBOX02.axis.com (10.0.5.16) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 1987 Lines: 71 On 04/11/2016 03:23 PM, Eric Dumazet wrote: > On Mon, 2016-04-11 at 08:24 +0200, Lars Persson wrote: >> A failure in validate_xmit_skb_list() triggered an unconditional call >> to dev_requeue_skb with skb=NULL. This slowly grows the queue >> discipline's qlen count until all traffic through the queue stops. >> >> By introducing a NULL check in dev_requeue_skb it was also necessary >> to make the __netif_schedule call conditional to avoid scheduling an >> empty queue. >> >> Fixes: 55a93b3ea780 ("qdisc: validate skb without holding lock") >> Signed-off-by: Lars Persson >> --- >> net/sched/sch_generic.c | 11 +++++++---- >> 1 file changed, 7 insertions(+), 4 deletions(-) >> >> diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c >> index f18c350..4e6a79c 100644 >> --- a/net/sched/sch_generic.c >> +++ b/net/sched/sch_generic.c >> @@ -47,10 +47,13 @@ EXPORT_SYMBOL(default_qdisc_ops); >> >> static inline int dev_requeue_skb(struct sk_buff *skb, struct Qdisc *q) >> { >> - q->gso_skb = skb; >> - q->qstats.requeues++; >> - q->q.qlen++; /* it's still part of the queue */ >> - __netif_schedule(q); >> + if (skb) { >> + q->gso_skb = skb; >> + q->qstats.requeues++; >> + q->q.qlen++; /* it's still part of the queue */ >> + } >> + if (qdisc_qlen(q)) >> + __netif_schedule(q); >> >> return 0; >> } > > > Please always CC patch author when fixing a bug. > > Why adding the if (qdisc_qlen(q)) extra test ? > > This seems unrelated to the bug fix, and probably should be part of a > second patch targeting net-next tree. I though it would be prudent because the queue can be non-empty even for the case of skb=NULL. So should it be there in this patch, another patch or not at all ? > > Also please add a likely() clause > > if (likely(skb)) { > q->gso_skb = skb; > q->qstats.requeues++; > q->q.qlen++; /* it's still part of the queue */ > __netif_schedule(q); > } Will fix. > Thanks ! > > > > >