From owner-freebsd-net Fri Dec 7 8:15:38 2001 Delivered-To: freebsd-net@freebsd.org Received: from critter.freebsd.dk (critter.freebsd.dk [212.242.86.163]) by hub.freebsd.org (Postfix) with ESMTP id 82C3B37B405; Fri, 7 Dec 2001 08:15:28 -0800 (PST) Received: from critter.freebsd.dk (localhost [127.0.0.1]) by critter.freebsd.dk (8.11.6/8.11.6) with ESMTP id fB7GE0j22767; Fri, 7 Dec 2001 17:14:00 +0100 (CET) (envelope-from phk@critter.freebsd.dk) To: arch@freebsd.org, net@freebsd.org Subject: Request to back out Luigis polled-net patch in -stable. Date: Fri, 07 Dec 2001 17:14:00 +0100 Message-ID: <22765.1007741640@critter.freebsd.dk> From: Poul-Henning Kamp Sender: owner-freebsd-net@FreeBSD.ORG Precedence: bulk List-ID: List-Archive: (Web Archive) List-Help: (List Instructions) List-Subscribe: List-Unsubscribe: X-Loop: FreeBSD.org ------- Forwarded Message To: core@freebsd.org Subject: Request to back out Luigis polled-net patch in -stable. From: Poul-Henning Kamp Date: Fri, 07 Dec 2001 17:13:24 +0100 Message-ID: <22749.1007741604@critter.freebsd.dk> Sender: phk@critter.freebsd.dk I have not read the entire patch in details, but have focused on the part of the code which I have special insight into. My complaint is therefore mainly based on one single file: src/sys/kern/kern_clock.c which has been spammed with a lot of stuff which has absolutely no business being there. Just for starters: Adding this to sys/kern/kern_clock.c: +#ifdef DEVICE_POLLING +#include /* for vm_page_zero_idle() */ +#include /* needed by sys/if.h */ +#include /* for IFF_* flags */ +#include /* for NETISR_POLL */ +[...] +#endif /* DEVICE_POLLING */ None of this as any business here, and it certainly doesn't even look remotely possible for this to work on !i386 platforms. Most of the rest of the addition is code which should not be in sys/kern in the first place but sys/net. Just because it is #ifdef DEVICE_POLLING doesn't mean it can violate our architecture. Furthermore, according to the commit message, this was approved by jkh in his role of release-engineer, but I find it disturbing that the approval seems to have happened without any technical or architectural review of the proposed patch. Finally, I thought a significant change like this would have to go into -current at least at the same time, but preferably before it goes into -stable. Considering the repeated claims of +10% performance for this suite of changes, there is a very good chance that people will pick it up and use it, leaving us with a big problem when 5.0-R time comes around and we have to explain A) why the feature isn't there, and B) why performance is worse. Summary: I request that luigis patch gets backed out for the following reasons: 1. The patch is architecturally a mess. 2. The approval seems to have happened purely on "glossy sales-brochure" backing without any technical or architectural input. 3. It didn't go into -current first and has received no shakeout before it went into -stable. I am _NOT_ saying that the idea is bad, it sounds pretty intelligent to me, but in that case it is even more important to not mess it up. I would also like to point to the parallel piece of code: Jun-Itohs ALTQ for which he reliably has maintained a patch relative to the 4.X branch and which despite various peoples requests have not haphazardly been committed into -stable. And in that context one should not forget that ALTQ has a lot longer and better trackrecord of high quality than Luigis poll-code, or any of Luigis code for that matter. - -- Poul-Henning Kamp | UNIX since Zilog Zeus 3.20 phk@FreeBSD.ORG | TCP/IP since RFC 956 FreeBSD committer | BSD since 4.3-tahoe Never attribute to malice what can adequately be explained by incompetence. ------- End of Forwarded Message To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-net" in the body of the message