Re: [PATCH] make cancel_rearming_delayed_work() reliable

Tejun Heo
2007-05-15T08:27:22Z

Oleg Nesterov wrote:
>> Yeap, I've used the term 'clearing' as more of a logical term - making
>> the pointer invalid, but is there any reason why we can't clear the
>> pointer itself?
>
> Because this breaks cancel_work_sync(work), it needs get_wq_data(work) for
> wait_on_work(work).

Right. That's the other user of cached work->wq_data pointer.

> So, try_to_grab_pending() should check "VALID && pointers equal" atomically.
> We can't do "if (VALID && cwq == get_wq_data(work))". We should do something
> like this
>
> (((long)cwq) | VALID | PENDING) == atomic_long_read(&work->data)
>
> Yes? I need to think more about this.

I don't think the test for PENDING should be atomic too. cwq pointer
and VALID is one package. PENDING lives its own life as a atomic bit
switch.

>> I didn't really get the smp_mb__before_spinlock() thing. How are you
>> planning to use it? spinlock() is already a read barrier. What do you
>> gain by making it also a write barrier?
>
> As I said, we can shift set_wq_data() from insert_work() to __queue_work(),

OIC.

> this needs very minor changes.
>
> BTW, in _theory_, spinlock() is not a read barrier, yes?

It actually is.

> From Documentation/memory-barriers.txt
>
> Memory operations that occur before a LOCK operation may appear to happen
> after it completes.

Which means that spin_lock() isn't a write barrier. lock is read
barrier, unlock is write barrier. Otherwise, locking doesn't make much
sense. If we're going the barrier way, I think we're better off with
explicit smp_wmb(). It's only barrier() on x86/64.

Thanks.

--
tejun