From owner-freebsd-hackers Sun Jun 23 14:59: 5 2002 Delivered-To: freebsd-hackers@freebsd.org Received: from prism.flugsvamp.com (66-191-112-47.mad.wi.charter.com [66.191.112.47]) by hub.freebsd.org (Postfix) with ESMTP id 99CAF37B44B for ; Sun, 23 Jun 2002 14:58:40 -0700 (PDT) Received: (from jlemon@localhost) by prism.flugsvamp.com (8.11.6/8.11.6) id g5NLw9c49030; Sun, 23 Jun 2002 16:58:09 -0500 (CDT) (envelope-from jlemon) Date: Sun, 23 Jun 2002 16:58:09 -0500 (CDT) From: Jonathan Lemon Message-Id: <200206232158.g5NLw9c49030@prism.flugsvamp.com> To: dillon@apollo.backplane.com, hackers@freebsd.org Subject: Re: Bug in wakeup() (stable and current) ? X-Newsgroups: local.mail.freebsd-hackers In-Reply-To: References: Organization: Cc: Sender: owner-freebsd-hackers@FreeBSD.ORG Precedence: bulk List-ID: List-Archive: (Web Archive) List-Help: (List Instructions) List-Subscribe: List-Unsubscribe: X-Loop: FreeBSD.ORG In article you write: >:I'm pretty sure you only need to 'goto restart' if you call into >:maybe_resched() as someone else may have manipulated the queues. >: >:The 'restart' label is only in there for restarting in case one of >:the functions called may change the lists, if we restart _every_ >:time we'll traverse the same procs where p->p_wchan != ident over >:and over needlessly. >: >:-Alfred > > Look at the code carefully. It's *removing* the element from the list, > the conditionally restarting rather then removing the element from the > list and unconditionally restarting. The only reason it works at all > is because sys/queue.h does not clear out the pointers in the node > that was just removed. The code is just plain wrong, though, because > the queue mechanisms make no such (documented) guarentee. Looks like the original damage happened in r1.21, where the temporary variable (used to hold the next item on the list) was replaced by a dereference through the pointer of the item that was just removed. The code works simply because it relies TAILQ_REMOVE() not changing the tqe_next pointer. I suppose that this should either be documented, or the loop changed back to use a temp variable: for (td = TAILQ_FIRST(qp); td != NULL; td = tdq) { tdq = TAILQ_NEXT(td, td_slpq); ... } -- Jonathan To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-hackers" in the body of the message