On 8/11/20 2:10 AM, Oleg Nesterov wrote:
> On 08/11, Peter Zijlstra wrote:
>>
>> On Tue, Aug 11, 2020 at 09:14:02AM +0200, Oleg Nesterov wrote:
>>> On 08/11, Peter Zijlstra wrote:
>>>>
>>>> On Tue, Aug 11, 2020 at 08:45:16AM +0200, Oleg Nesterov wrote:
>>>>>
>>>>> ->jobctl is always modified with ->siglock held, do we really need
>>>>> WRITE_ONCE() ?
>>>>
>>>> In theory, yes. The compiler doesn't know about locks, it can tear
>>>> writes whenever it feels like it.
>>>
>>> Yes, but why does this matter? Could you spell please?
>>
>> Ah, well, that I don't konw. Why do we need the READ_ONCE() ?
>>
>> It does:
>>
>>> + if (!(task->jobctl & JOBCTL_TASK_WORK) &&
>>> + lock_task_sighand(task, &flags)) {
>>
>> and the lock_task_sighand() implies barrier(), so I thought the reason
>> for the READ_ONCE() was load-tearing, and then we need WRITE_ONCE() to
>> avoid store-tearing.
>
> I don't think we really need READ_ONCE() for correctness, compiler can't
> reorder this LOAD with cmpxchg() above, and I think we don't care about
> load-tearing.
>
> But I guess we need READ_ONCE() or data_race() to shut kcsan up.
Thanks, reading through this thread makes me feel better. I agree that
we'll need READ_ONCE() just to shut up analyzers.
I'd really like to get this done at the same time as the io_uring
change. Are you open to doing the READ_ONCE() based JOBCTL_TASK_WORK
addition for 5.9? Alternatively we can retain the 1/2 patch from this
series and I'll open-code it in io_uring, but seems pointless as
io_uring is the only user of TWA_SIGNAL in the kernel anyway.
--
Jens Axboe
