Re: cn_queue.c

[Evgeniy Polyakov <johnpol@2ka.mipt.ru>]

[-- Attachment #1: Type: text/plain, Size: 1850 bytes --]

On Fri, 2005-04-01 at 15:12 +0400, Evgeniy Polyakov wrote:

> > > cn_queue_wrapper() atomically increments cbq->cb->refcnt if runs, so it
> > > will
> > > be caught in 
> > > while (atomic_read(&cbq->cb->refcnt)) 
> > >   msleep(1000);
> > > in cn_queue_free_callback().
> > > If it does not run, then all will be ok.
> > 
> > But there's a time window on entry to cn_queue_wrapper() where the recsount
> > hasn't been incremented yet, and there's no locking.  If
> > cn_queue_free_callback() inspects the refcount in that window it will free
> > the cn_callback_entry() while cn_queue_wrapper() is playing with it?
> 
> If we already run cn_queue_wrapper() [even before refcnt incrementing,
> probably it is not even needed there], then cancel_delayed_work() will
> sleep,
> since appropriate timer will be deleted in del_timer_sync(), which 
> will wait untill it is finished on the different CPU.
> 
> > > Btw, it looks like comments for del_timer_sync() and cancel_delayed_work
> > > ()
> > > are controversial - del_timer_sync() says that pending timer
> > > can not run on different CPU after returning, 
> > > but cancel_delayed_work() says, that work to be cancelled still 
> > > can run after returning.
> > 
> > Not controversial - the timer can have expired and have been successfully
> > deleted but the work_struct which the timer handler scheduled is still
> > pending, or has just started to run.
> 
> Ugh, I see now.
> There are two levels of work deferring  - into cpu_workqueue_struct, 
> and, in case of delayed work, into work->timer.
> 
> 
> Yes, I need to place flush_workqueue() in cn_queue_free_callback();
> 

That actullay NULLifies above sentence about sleeping in
cancel_delayed_work().

-- 
        Evgeniy Polyakov

Crash is better than data corruption -- Arthur Grabowski

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]