From owner-freebsd-net@FreeBSD.ORG Thu Dec 5 23:10:31 2013 Return-Path: Delivered-To: freebsd-net@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id 5602BF9B; Thu, 5 Dec 2013 23:10:31 +0000 (UTC) Received: from mail-qa0-x233.google.com (mail-qa0-x233.google.com [IPv6:2607:f8b0:400d:c00::233]) (using TLSv1 with cipher ECDHE-RSA-RC4-SHA (128/128 bits)) (No client certificate requested) by mx1.freebsd.org (Postfix) with ESMTPS id E19B61371; Thu, 5 Dec 2013 23:10:30 +0000 (UTC) Received: by mail-qa0-f51.google.com with SMTP id o15so51607qap.17 for ; Thu, 05 Dec 2013 15:10:30 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:sender:in-reply-to:references:date:message-id:subject :from:to:content-type; bh=jAK6VOKg3hHSGafZSkKKlWuv5na1bc7HdO1VllnyIj4=; b=tOE0j8P8w/2uYvUv9eFVoN8O7gSQGKlRyyx1vSwcGg5r92wP84fSDwVUoEMZTl010o sITmErzYkSCdDAPsU2ek1IiekIm+yGmxpSlOyB0A9IJeH03jmULYxbWwxMAvnKbcG94/ WwbAKM3U/QiC0n/gfvekenMJu6R2VqR7pIPcFeLkOP6QF5hHWLs1Z9SMhyAMM3vvHXXr yPF2AQ0Lv5TgMe51oBz2dVqSRxpJihu6PkdWKm8UQulmbIjOD+j9fDBXfEL56QNyMdrb 0ACmljiCMezsmKFkHc519Naoza2JZR2Cs9lf7NT90wizH/I0Wao/EzwTY5dLOXD1s9o3 LPtA== MIME-Version: 1.0 X-Received: by 10.49.17.232 with SMTP id r8mr625571qed.74.1386285030050; Thu, 05 Dec 2013 15:10:30 -0800 (PST) Sender: adrian.chadd@gmail.com Received: by 10.224.53.200 with HTTP; Thu, 5 Dec 2013 15:10:29 -0800 (PST) In-Reply-To: <20131205223711.GB55638@funkthat.com> References: <521B9C2A-EECC-4412-9F68-2235320EF324@lurchi.franken.de> <20131202022338.GA3500@michelle.cdnetworks.com> <20131203021658.GC2981@michelle.cdnetworks.com> <20131205223711.GB55638@funkthat.com> Date: Thu, 5 Dec 2013 15:10:29 -0800 X-Google-Sender-Auth: uIGcF04FtMbrYJKt4ij-9g8qE7s Message-ID: Subject: Re: A small fix for if_em.c, if_igb.c, if_ixgbe.c From: Adrian Chadd To: Adrian Chadd , Michael Tuexen , Yong-Hyeon Pyun , Jack F Vogel , "freebsd-net@freebsd.org list" Content-Type: text/plain; charset=ISO-8859-1 X-BeenThere: freebsd-net@freebsd.org X-Mailman-Version: 2.1.17 Precedence: list List-Id: Networking and TCP/IP with FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 05 Dec 2013 23:10:31 -0000 On 5 December 2013 14:37, John-Mark Gurney wrote: > Adrian Chadd wrote this message on Thu, Dec 05, 2013 at 14:01 -0800: >> On 5 December 2013 13:05, Michael Tuexen >> wrote: >> >> > Just to be clear: This would mean that xxx_transmit() would return >> > an error even if the packet provided in the call xxx_transmit() is >> > enqueued and not dropped? >> > This would also be problem with the current SCTP stack. >> >> I think it'll return an error only if: >> >> * it queued the frame to the tail of the drbd; >> * it then tried to transmit a frame from the head of the drbd; >> * it failed to transmit the first frame in the drbd and it couldn't >> put it back into the queue for whatever reason. >> >> So I think it should be "ok enough" for both TCP and SCTP. > > IMO it should only return an error if the specific frame failed to be > sent or queued. If you cannot determine at return time if the frame > failed to be transmitted/queued, then it should return success. For the long term solution, I agree. > In the above case, if there were other frames queued ahead, and the > first one failed, then it sounds like the frame may eventually be sent > and we will end up sending a duplicate frame. Right. We should also fix this properly. I think the right thing, long term, is something like this; * xxx_mq_start_locked() returns whether the head frame was transmitted or not; * the if_transmit() entry point(s) return whether the given frame was queued to the software queue or not; * the if_transmit() entry point(s) ignore the return value of xxx_mq_start_locked(), as the stack _should_ handle the case of a frame handed to the driver but dropped. So, I'd like to get Michael to first test fixing up xxx_mq_start_locked() to only return an error if it failed to transmit a frame and the frame was dropped. Then, once we get feedback from that, I was going to propose that we also do what Michael initially did - and that's ignore the error from calling xxx_mq_start_locked(). Followed, hopefully, with some comments explaining how this all holds together. How's that sound? -adrian