Date: Fri, 07 Dec 2001 17:14:00 +0100 From: Poul-Henning Kamp <phk@critter.freebsd.dk> To: arch@freebsd.org, net@freebsd.org Subject: Request to back out Luigis polled-net patch in -stable. Message-ID: <22765.1007741640@critter.freebsd.dk>
next in thread | raw e-mail | index | archive | help
------- Forwarded Message To: core@freebsd.org Subject: Request to back out Luigis polled-net patch in -stable. From: Poul-Henning Kamp <phk@freebsd.org> 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 <i386/include/md_var.h> /* for vm_page_zero_idle() */ +#include <sys/socket.h> /* needed by sys/if.h */ +#include <net/if.h> /* for IFF_* flags */ +#include <net/netisr.h> /* 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?22765.1007741640>