lkml.org 
[lkml]   [2009]   [Aug]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: Is adding requeue_delayed_work() a good idea
    Now I noticed I forgot to CC Dmitry yesterday...

    On 08/24, Roland Dreier wrote:
    >
    > > > In my particular case it doesn't really matter. In the queued case it
    > > > could leave it to run whenever it gets to the head of the workqueue. In
    > > > the already running case then I think the timer should be reset. The
    > > > main point is that if I do requeue_delayed_work() I want to make sure
    > > > the work runs all the way through from the beginning at some point in
    > > > the future. The pattern I have in mind is something like:
    > > >
    > > > spin_lock_irqsave(&mydata_lock);
    > > > new_timeout = add_item_to_timeout_list();
    > > > requeue_delayed_work(wq, &process_timeout_list_work, new_timeout);
    > > > spin_unlock_irqsave(&mydata_lock);
    > > >
    > > > so if the process_timeout_list_work runs early or twice it doesn't
    > > > matter; I just want to make sure that the work runs from the beginning
    > > > and sees the new item I added to the list at some point after the
    > > > requeue.
    > >
    > > Hmm. But, asuming that process_timeout_list_work->func() takes mydata_lock
    > > too, you can just use queue_delayed_work() ?
    > >
    > > process_timeout_list_work can't miss new items, queue_delayed_work()
    > > can only fail if dwork is pending and its ->func has not started yet.
    >
    > Maybe I misunderstand the code or misunderstand you,

    No, sorry. I misunderstood you (and sorry for delays btw).

    I have read "I just want to make sure" above but forgot you also need
    to shorten the timeout.

    OK, in this case I think we have a simple solution,

    // like cancel_delayed_work, but uses del_timer().
    // this means, if it returns 0 the timer function may be
    // running and the queueing is in progress. The caller
    // can't rely on flush_workqueue/etc
    static inline int __cancel_delayed_work(struct delayed_work *work)
    {
    int ret;

    ret = del_timer(&work->timer);
    if (ret)
    work_clear_pending(&work->work);
    return ret;
    }

    Now, you can do

    spin_lock_irqsave(&mydata_lock);
    new_timeout = add_item_to_timeout_list();

    __cancel_delayed_work(&process_timeout_list_work);
    queue_delayed_work(wq, &process_timeout_list_work, new_timeout);

    spin_unlock_irqsave(&mydata_lock);

    If queue_delayed_work() fails, this means that WORK_STRUCT_PENDING is set,
    dwork->work is already queued or the queueing is in progress. In both
    cases it will run "soon" as if we just called queue_work(&dwork->work).

    But this assumes nobody else does queue_delayed_work(dwork, HUGE_DELAY) in
    parallel, otherwise we can lose the race and another caller can setup
    HUGE_DELAY timeout.

    In particular, if process_timeout_list_work->func() itself uses
    queue_delay_work() to re-arm itself we can race. Bu t I think it is always
    possible to do something to synchronize with work->func, for example
    work->func() can re-arm itself _before_ it scans timeout_list (under the
    same lock). This way, if re-queue code above fails because work->func()
    wins, work->func() must see the new additions to timeout_list.

    Can this work for you?

    Oleg.



    \
     
     \ /
      Last update: 2009-08-25 11:45    [W:0.022 / U:187.676 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site