From owner-freebsd-current@FreeBSD.ORG Wed Aug 17 20:05:03 2005 Return-Path: X-Original-To: freebsd-current@FreeBSD.org Delivered-To: freebsd-current@FreeBSD.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id C54A416A44F; Wed, 17 Aug 2005 20:05:03 +0000 (GMT) (envelope-from scottl@samsco.org) Received: from pooker.samsco.org (pooker.samsco.org [168.103.85.57]) by mx1.FreeBSD.org (Postfix) with ESMTP id 5511C43D46; Wed, 17 Aug 2005 20:05:02 +0000 (GMT) (envelope-from scottl@samsco.org) Received: from [127.0.0.1] (pooker.samsco.org [168.103.85.57]) (authenticated bits=0) by pooker.samsco.org (8.13.3/8.13.3) with ESMTP id j7HKFt4V060784; Wed, 17 Aug 2005 14:15:55 -0600 (MDT) (envelope-from scottl@samsco.org) Message-ID: <43039851.3020401@samsco.org> Date: Wed, 17 Aug 2005 14:04:33 -0600 From: Scott Long User-Agent: Mozilla/5.0 (X11; U; FreeBSD i386; en-US; rv:1.7.8) Gecko/20050615 X-Accept-Language: en-us, en MIME-Version: 1.0 To: John Baldwin References: <20050811220017.A72944@hades.admin.frm2> <200508171037.42042.jhb@FreeBSD.org> <43037604.6030407@samsco.org> <200508171509.48946.jhb@FreeBSD.org> In-Reply-To: <200508171509.48946.jhb@FreeBSD.org> Content-Type: text/plain; charset=us-ascii; format=flowed Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-2.8 required=3.8 tests=ALL_TRUSTED autolearn=failed version=3.0.2 X-Spam-Checker-Version: SpamAssassin 3.0.2 (2004-11-16) on pooker.samsco.org Cc: freebsd-current@FreeBSD.org Subject: Re: 6.0-BETA2: taskqueue_drain for if_xl.c:2796 X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Discussions about the use of FreeBSD-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 17 Aug 2005 20:05:04 -0000 John Baldwin wrote: > On Wednesday 17 August 2005 01:38 pm, Scott Long wrote: > >>John Baldwin wrote: >> >>>On Tuesday 16 August 2005 01:21 pm, John Baldwin wrote: >>> >>>>On Monday 15 August 2005 02:22 am, Scott Long wrote: >>>> >>>>>John Baldwin wrote: >>>>> >>>>>>On Thursday 11 August 2005 07:07 pm, Scott Long wrote: >>>>> >>>>>I still don't see what the point is of having a function that only >>>>>removes pending tasks from the queue if it doesn't also drain in >>>>>progress tasks. Simple example: >>>>> >>>>>CPU1 calls xl_stop. The XL_LOCK gets taken. At the same time, CPU2 >>>>>calls taskqueue_swi, which pops the queue and calls xl_rxeof_task. It >>>>>tries to take the XL_LOCK but instead sleeps (or adaptively spins) >>>>>because CPU1 already has it. CPU1 calls taskqueue_stop, which doesn't >>>>>do anything because the task was alrady popped off. >>>>> >>>>>Now, at some point, CPU1 **MUST** drop the XL_LOCK and let CPU2 continue >>>>>that task to completion. There simply is no way around that, right? So >>>>>why muddle the API with a function that really doesn't solve any >>>>>problems. The argument that being able to call taskqueue_stop with >>>>>locks held is a convenience doesn't hold water. You'll still need to >>>>>drop locks in order to be sure that a mess isn't created. >>>> >>>>Well, you need to think through this longer. During a normal stop, we >>>>don't care if the task runs later if it is just going to stop anyway, >>>>which it will with the IFF_DRV_RUNNING checks I added (which it needs >>>>anyway even if you don't add a taskqueue_stop()). See, when CPU2 >>>>eventually runs the xl_rxeof_task, it will drop the lock and return >>>>instantly because the xl_stop() function clears IFF_DRV_RUNNING in >>>>addition to calling taskqueue_stop(). >>> >>>Actually, I thought through this longer some myself on the way home and >>>because of the RUNNING checks, the taskqueue_stop() in xl_stop() isn't >>>actually needed for correct operation. It's merely an optimization for >>>the case that the task is sitting in the queue, but the code will work >>>fine without it with the taskqueue_drain() moved to xl_detach() and the >>>IFF_DRV_RUNNING checks added. Thus, if you just really hate >>>taskqueue_stop() or don't think the optimization is worth it, I can leave >>>it out. I kind of would like to call it in xl_detach() at least so that >>>detaching xl0 won't be dependent on another driver's task deciding to >>>yield. However, if I moved it there I could add blocking to it. I could >>>also just use the existing taskqueue_drain() and not worry about having >>>to wait for other tasks to finish. I can't call taskqueue_drain() in >>>xl_stop() though since xl_stop() is called indirectly from xl_intr(). >> >>You can't force detach while data is in flight, at least not right now. >>That is a motivation behind the recent ifnet changes, but it still has a >>long way to go. And regardless of the rest of the stack being able to >>handle forced detach, the driver needs to be written in a way so that >>it's own resources aren't orphaned, which again means making sure that >>blocked threads are allowed to run to completion, which again means not >>having the luxury of being fast and loose with locks when trying to >>drain those tasks. > > > I'm not running "fast and loose" with locks per se. Mostly I'm just > optimizing the case where we stop the interface (i.e. detach or ifconfig > down) and the task hasn't even started running yet so that it can just > discard the task from the queue without running it rather than waiting until > it gets a chance to run so it can immediately abort. > > >>I do think that taskqueue_stop() is a muddying of the API. If anyone >>else has a comment on this (BDE?) I would be very interested to hear it. > > > *shrug* It's already confusing in that callout_drain() does both a stop and > then wait kind of like pthread_cancel() whereas taskqueue_drain() just waits > until the task has run and finished without trying to cancel it like > pthread_join(). I'd actually probably be inclined to rename callout_drain() > to something else if we wanted to resolve that specific instance of confusion > personally. > True, consistency would be nice =-) Would you rather change taskqueue_drain() to be more like callout_drain()? Scott