Re: [PATCH] make cancel_rearming_delayed_work() reliable

[Tejun Heo <htejun@gmail.com>]

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