From owner-freebsd-net@FreeBSD.ORG Thu Jun 11 20:52:10 2009 Return-Path: Delivered-To: freebsd-net@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id B2A24106566C; Thu, 11 Jun 2009 20:52:10 +0000 (UTC) (envelope-from bz@FreeBSD.org) Received: from mail.cksoft.de (mail.cksoft.de [195.88.108.3]) by mx1.freebsd.org (Postfix) with ESMTP id 147A88FC16; Thu, 11 Jun 2009 20:52:09 +0000 (UTC) (envelope-from bz@FreeBSD.org) Received: from localhost (amavis.fra.cksoft.de [192.168.74.71]) by mail.cksoft.de (Postfix) with ESMTP id C0D5C41C49C; Thu, 11 Jun 2009 22:35:06 +0200 (CEST) X-Virus-Scanned: amavisd-new at cksoft.de Received: from mail.cksoft.de ([195.88.108.3]) by localhost (amavis.fra.cksoft.de [192.168.74.71]) (amavisd-new, port 10024) with ESMTP id sNSX2JseoUBr; Thu, 11 Jun 2009 22:35:06 +0200 (CEST) Received: by mail.cksoft.de (Postfix, from userid 66) id 3ED9F41C71D; Thu, 11 Jun 2009 22:35:06 +0200 (CEST) Received: from maildrop.int.zabbadoz.net (maildrop.int.zabbadoz.net [10.111.66.10]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) by mail.int.zabbadoz.net (Postfix) with ESMTP id 382E34448E6; Thu, 11 Jun 2009 20:33:28 +0000 (UTC) Date: Thu, 11 Jun 2009 20:33:28 +0000 (UTC) From: "Bjoern A. Zeeb" X-X-Sender: bz@maildrop.int.zabbadoz.net To: Andrew Gallatin , Pyun YongHyeon , Jack F Vogel , Andrew Thompson , Michael Tuexen Message-ID: <20090611184555.J22887@maildrop.int.zabbadoz.net> X-OpenPGP-Key: 0x14003F198FEFA3E77207EE8D2B58B8F83CCF1842 MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed Cc: FreeBSD net mailing list Subject: Ethernet NIC drivers depending unconditionally on INET X-BeenThere: freebsd-net@freebsd.org X-Mailman-Version: 2.1.5 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, 11 Jun 2009 20:52:10 -0000 Hi, as announced in my other mail to current@ and net@ I added the mandatory "INET" dependency to 12 Ethernet NIC drivers. Additionally I am adding if_bridge here as well. Those drivers should be fixed and once done the (mandatory) inet dependency should be removed again from sys/conf/files for that driver. If you have questions, need help, compile testing, ... feel free to mail me. I won't be able to test things as I do not own all of the different cases but I can review patches, .. Generally speaking upfront: if one of the drivers mentioned here or not, supportes offload capabilities for rx or tx of v4 or v6, for tcp, udp, sctp, .. they need to be properly hidden under the apropriate #ifdefs. Everything that needs v4 belongs under #ifdef INET. Everything that needs v6 belongs under #ifdef INET6. Upper layer protocols like TCP, UDP, SCTP should be at least #if defined (INET) || defined(INET6). In case of SCTP probebly #ifdef (SCTP) as well. You will need to include opt_inet.h, opt_inet6.h, opt_sctp.h in those cases and at least the buildkernel (not building the module alone) part should properly check them. For single module builds we are kind of lost here and I think we usually enabled things by default as they are in GENERIC. Another general note that scared me while looking at CSUM_TSO and IFCAP_TSO4|IFCAP_TSO6. Almost none of the drivers actually checks that it is an IPv4 packet before grabiing the IP header, TCP header, ... How do we actually know that it is an IPv4 packet in all those drivers? Should we do a IP Version fiel check? I think em(4) is one of those that does. I hope this will improve with tuexen's work on the CSUM flags. In case you are maintaining another driver but cannot find it on the list - be sure we'll find you during the 9.x lifecycle in case you do not properly handle things;-) if_age: ---------------------------------------- I identifed two in_pseudo() calls that are INET specific. The problem is that thi is part of the (IPv4) TSO code and it seems to be mandatory for that case. The proper fix seems to be to but that block under #ifdef INET and in case INET is not defined to also disable announcing or permitting to set these features entirely as we won't be able to handle them or use them anyway. if_alc: ---------------------------------------- There is one in_pseudo(); Apart from that things seem identical to if_age. if_ale: ---------------------------------------- There is one in_pseudo(); Apart from that things seem identical to if_age. if_em: ---------------------------------------- There is one in_pseudo() in em_tso_setup() that prevented the driver from being linked into the kernel; the entire code unfortunately has at least one more place of IP/IPV6 distinction for cksum offloading em_transmit_checksum_setup() that is not properly handled either. In either case the feature should be disbaled and not be announced if the address family is not there to handle the code. Note: tuexen is working on a proper set of v4/v6 definitions for the csum offloading, so we will be able to do this more fine grained in the future. For now not having INET6 would penalize INET as well. if_igb: ---------------------------------------- igb_tso_setup() has one in_pseudo() call. See if_age for that. It also has dependencies on the entire LRO framework like tcp_lro_free(), tcp_lro_rx(), tcp_lro_flush(), tcp_lro_init(). The LRO framework is on the IPv6TODO list. For the moment if there is no INET LRO should be completely compiled out and not advertised removing the TUNABLE and SYSCTL and changing the default to 0. Be prepared in case LRO will arrive for IPv6. Side note: calling tcp_lro_free() on something that hadn't been initialized for example because lro was enabled by the sysctl after itniialization, can that be a problem? Maybe tcp_lro_free() should just return in case cntl is NULL? if_fxp: ---------------------------------------- There is one in_pseudo(); Apart from that things seem identical to if_age. ixgbe: ---------------------------------------- There is one in_pseudo() in ixgbe_tso_setup(). See if_igb for that. There is an arp_ifinit() call in ixgbe_ioctl() that already seems to check for the proper address family and only needs the #ifdefs around like the ioctl in if_em has. The same that applies for if_igb about LRO applies here as well. if_jme: ---------------------------------------- Two in_pseudo() calls; see if_age. if_msk: ---------------------------------------- An in_cksum_skip() call. Almost like if_age but this seems to be further conditional on driver flags and CSUM_TCP that seem to be set independently of TSO. I am not entirely sure if there is a proper solution or if we might be forced to return an error in that case in case there is not INET support. if_mxge: ---------------------------------------- mxge_rx_csum() has one in_pseudo(). The function and callers already seem to know how do deal with results in case the csum can't be validated. So this should be a simple #ifdef INET wrapping here; side note: the tcpudp_csum variables in the callers are not needed. side note: huge inlining going on there;) mxge_lro_flush() has another call to in_pseudo(). As with if_igb/ixgbe if there is no INET there should be no LRO for now, the capabilities not advertised, etc. Be prepared in case LRO will arrive for IPv6. if_sk: ---------------------------------------- sk_rxcksum() has an in_addword() call. It seems that sk_rxcksum is only used if IFCAP_RXCSUM is on. So similar things like to TSO should be done; hiding it under #ifdef INET and not advertising the feature etc. if_txp.c: ---------------------------------------- Now this is an interesting case as txp_download_fw_section() is using in_cksum() to validate the firmware csum before downloading it to the card it seems. I am not sure how to bes work around that one. if_bridge: ---------------------------------------- if_bridge is porobably mostly INET6 clean but doesn't have any INEt checks. It's just a matter of going through the code and adding proper #ifdef INET handling the simple cases and see what's left. This is just work; in case you are bored, read this mail to the end and would like to do something, know the difference between IPv4 and IPv6 is 2 and thinking of C doesn't make you think of scale of C major in first place, you are welcome to send patches:-) Regards, Bjoern -- Bjoern A. Zeeb The greatest risk is not taking one.