From owner-freebsd-net@FreeBSD.ORG Tue Nov 20 15:21:53 2012 Return-Path: Delivered-To: freebsd-net@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [69.147.83.52]) by hub.freebsd.org (Postfix) with ESMTP id 3059C90A; Tue, 20 Nov 2012 15:21:53 +0000 (UTC) (envelope-from fodillemlinkarim@gmail.com) Received: from mail-ia0-f182.google.com (mail-ia0-f182.google.com [209.85.210.182]) by mx1.freebsd.org (Postfix) with ESMTP id B5C1D8FC12; Tue, 20 Nov 2012 15:21:52 +0000 (UTC) Received: by mail-ia0-f182.google.com with SMTP id x2so5448827iad.13 for ; Tue, 20 Nov 2012 07:21:46 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=message-id:date:from:user-agent:mime-version:to:cc:subject :references:in-reply-to:content-type:content-transfer-encoding; bh=GhF0+nXmzhszx2tOXSWZdSrTOX+049W228Zpnt8GWKk=; b=WS1HDJSjSJnMtvsZs+i/W1cMQb4fssni2vanen2bgbihex/iXnl2zWLE7L5dXDupej JRLDPIrpFGc3FJgIkaLFUBoun+xS2ZX3BTz28klwLkidrKl98Wc1NUrEMKpCfZPBB79a djhmsmbrqkyle4LqMfc24kkSqtcjV8jjKneFonGnMVbMwGa23bmF7utup9oMnq9oiyd0 Gi4b9KwkETzgJ7oB65gKM1mVNkGo0kP6xlBJZyDl9/1THDxsD5abtaXNgEooTuJYa475 DnrrfhkLvTJ+8U6gwDT1Zi71c/QbPIbpmZM4xAMwBMejDXaEbeNzpnpBqCVKkRfRdoSa M8yA== Received: by 10.50.178.8 with SMTP id cu8mr10523759igc.5.1353424906190; Tue, 20 Nov 2012 07:21:46 -0800 (PST) Received: from [192.168.1.73] ([208.85.112.101]) by mx.google.com with ESMTPS id rd10sm9650361igb.1.2012.11.20.07.21.44 (version=SSLv3 cipher=OTHER); Tue, 20 Nov 2012 07:21:45 -0800 (PST) Message-ID: <50AB9FFE.70401@gmail.com> Date: Tue, 20 Nov 2012 10:21:34 -0500 From: Karim Fodil-Lemelin User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:16.0) Gecko/20121026 Thunderbird/16.0.2 MIME-Version: 1.0 To: Gleb Smirnoff Subject: Re: igb diver crashes in head@241037 References: <50AA8F24.7080604@gmail.com> <20121120111833.GC67660@FreeBSD.org> In-Reply-To: <20121120111833.GC67660@FreeBSD.org> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Cc: Karim Fodil-Lemelin , jfv@FreeBSD.org, freebsd-net@FreeBSD.org X-BeenThere: freebsd-net@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: Networking and TCP/IP with FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 20 Nov 2012 15:21:53 -0000 Gleb, On 20/11/2012 6:18 AM, Gleb Smirnoff wrote: > Karim, > > On Mon, Nov 19, 2012 at 02:57:24PM -0500, Karim Fodil-Lemelin wrote: > K> While testing the latest igb driver in CURRENT I came across an issue > K> with igb_mq_start(). More specifically this code: > K> > K> ... > K> > K> struct mbuf *pm = NULL; > K> /* > K> ** Try to queue first to avoid > K> ** out-of-order delivery, but > K> ** settle for it if that fails > K> */ > K> if (m && drbr_enqueue(ifp, txr->br, m)) > K> pm = m; > K> err = igb_mq_start_locked(ifp, txr, pm); > K> > K> ... > K> > K> > K> The problem comes from the fact that drbr_enqueue() can return an error > K> and delete the mbuf as seen in drbr_enqueue(): > K> > K> ... > K> error = buf_ring_enqueue(br, m); > K> if (error) > K> m_freem(m); > K> ... > > Good catch! > > K> diff --git a/sys/dev/e1000/if_igb.c b/sys/dev/e1000/if_igb.c > K> index 1318910..be1719a 100644 > K> --- a/sys/dev/e1000/if_igb.c > K> +++ b/sys/dev/e1000/if_igb.c > K> @@ -961,15 +961,7 @@ igb_mq_start(struct ifnet *ifp, struct mbuf *m) > K> que = &adapter->queues[i]; > K> if (((txr->queue_status & IGB_QUEUE_DEPLETED) == 0) && > K> IGB_TX_TRYLOCK(txr)) { > K> - struct mbuf *pm = NULL; > K> - /* > K> - ** Try to queue first to avoid > K> - ** out-of-order delivery, but > K> - ** settle for it if that fails > K> - */ > K> - if (m && drbr_enqueue(ifp, txr->br, m)) > K> - pm = m; > K> - err = igb_mq_start_locked(ifp, txr, pm); > K> + err = igb_mq_start_locked(ifp, txr, m); > K> IGB_TX_UNLOCK(txr); > K> } else { > K> err = drbr_enqueue(ifp, txr->br, m); > > Well, the idea to prevent out-of-order delivery is important. TCP > suffers a lot when this happens. > > I'd suggest the following code: > > if (m) > drbr_enqueue(ifp, txr->br, m); > err = igb_mq_start_locked(ifp, txr, NULL); > > Which eventually leads us to all invocations of igb_mq_start_locked() called > with third argument as NULL. This allows us to simplify this function. > > Patch for review attached. > > In my case I use ALTQ and in igb_mq_start_locked() it calls drbr_needs_enqueue() which always return 1 with ALTQ. So I did not see the out of order issue. I do think your patch makes sense though and I'm also thinking that em_mq_start() could also benefit from the same logic. Regards, Karim.