On Mon, Apr 23, 2007 at 08:33:12PM +0400, Oleg Nesterov wrote: > On 04/23, Jarek Poplawski wrote: > > > > On Fri, Apr 20, 2007 at 09:08:36PM +0400, Oleg Nesterov wrote: > > > > > > First, this flag should be cleared after return from > > > cancel_rearming_delayed_work(). > > > > I think this flag, if at all, probably should be cleared only > > consciously by the owner of a work, maybe as a schedule_xxx_work > > parameter, (but shouldn't be used from work handlers for rearming). > > Mostly it should mean: we are closing (and have no time to chase > > our work)... > > This will change the API. Currently it is possible to do: > > cancel_delayed_work(dwork); > schedule_delayed_work(dwork, delay); > > and we have such a code. With the change you propose this can't work.
Not necessarily: this all was only a concept and schedule_xxx_work could be also a new function, if needed. > > > > Also, we should add a lot of nasty checks to workqueue.c > > > > Checking a flag isn't nasty - it's clear. IMHO current way of checking, > > whether cancel succeeded, is nasty. > > > > > > > > I _think_ we can re-use WORK_STRUCT_PENDING to improve this interface. > > > Note that if we set WORK_STRUCT_PENDING, the work can't be queued, and > > > dwork->timer can't be started. The only problem is that it is not so > > > trivial to avoid races. > > > > If there were no place, it would be better, then current way. > > But WORK_STRUCT_PENDING couldn't be used for some error checking, > > as it's now. > > Look, > > void cancel_rearming_delayed_work(struct delayed_work *dwork) > { > struct work_struct *work = &dwork->work; > struct cpu_workqueue_struct *cwq = get_wq_data(work); > struct workqueue_struct *wq; > const cpumask_t *cpu_map; > int retry; > int cpu; > > if (!cwq) > return; > > retry: > spin_lock_irq(&cwq->lock); > list_del_init(&work->entry); > __set_bit(WORK_STRUCT_PENDING, work_data_bits(work)); > retry = try_to_del_timer_sync(&dwork->timer) < 0; > spin_unlock_irq(&cwq->lock); > > if (unlikely(retry)) > goto retry; > > // the work can't be re-queued and the timer can't > // be re-started due to WORK_STRUCT_PENDING > > wq = cwq->wq; > cpu_map = wq_cpu_map(wq); > > for_each_cpu_mask(cpu, *cpu_map) > wait_on_work(per_cpu_ptr(wq->cpu_wq, cpu), work); > > work_clear_pending(work); > } > > I think this almost works, except: > > - we should change run_workqueue() to call work_clear_pending() > under cwq->lock. I'd like to avoid this. > > - this is racy wrt cpu-hotplug. We should re-check get_wq_data() > when we take the lock. This is easy. > > - we should factor out the common code with cancel_work_sync(). > > I may be wrong, still had no time to concentrate on this with a "clear head". > May be tomorrow. This looks fine. Of course, it requires to remove some debugging currently done with _PENDING flag and it's hard to estimate this all before you do more, but it should be more foreseeable than current way. But the races with _PENDING could be really "funny" without locking it everywhere. BTW - are a few locks more a real problem, while serving a "sleeping" path? And I don't think there is any reason to hurry... > > > > > - for a work function: to stop execution as soon as possible, > > > > even without completing the usual job, at first possible check. > > > > > > I doubt we need this "in general". It is easy to add some flag to the > > > work_struct's container and check it in work->func() when needed. > > > > Yes, but currently you cannot to behave like this e.g. with > > "rearming" work. > > Why? OK, it's not impossible, but needs some bothering: if I simply set some flag and my work function exits before rearming - cancel_rearming_delayed_work can loop. > > > And maybe a common api could save some work. > > May be you are right, but still I don't think we should introduce > the new flag to implement this imho not-so-useful feature. Maybe you are right. Probably some code should be analysed, to check how often such activities are needed. Cheers, Jarek P. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/