Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH] task_work: return -EBUSY when adding same work
To:     David Laight <David.Laight@ACULAB.COM>,
        yaozhenguo <yaozhenguo1@gmail.com>,
        "oleg@redhat.comm" <oleg@redhat.comm>
Cc:     "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "yaozhenguo@jd.com" <yaozhenguo@jd.com>
References: <20210709122712.42844-1-yaozhenguo1@gmail.com>
 <872612b5-b9c6-43aa-a167-1c204d0f1c5a@kernel.dk>
 <3079d213543c4d398d96031e6da26c82@AcuMS.aculab.com>
From:   Jens Axboe <axboe@kernel.dk>
Message-ID: <eddfda56-bfd7-8e5a-1abd-13a162e90d35@kernel.dk>
Date:   Tue, 13 Jul 2021 14:22:10 -0600
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.10.0
MIME-Version: 1.0
In-Reply-To: <3079d213543c4d398d96031e6da26c82@AcuMS.aculab.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 7/13/21 4:41 AM, David Laight wrote:
> From: Jens Axboe
>> Sent: 09 July 2021 15:18
> ...
>>>   */
>>>  int task_work_add(struct task_struct *task, struct callback_head *work,
>>>  		  enum task_work_notify_mode notify)
>>> @@ -41,6 +41,8 @@ int task_work_add(struct task_struct *task, struct callback_head *work,
>>>  		head = READ_ONCE(task->task_works);
>>>  		if (unlikely(head == &work_exited))
>>>  			return -ESRCH;
>>> +		if (unlikely(head == work))
>>> +			return -EBUSY;
>>>  		work->next = head;
>>>  	} while (cmpxchg(&task->task_works, head, work) != head);
>>
>> I don't think there's anything conceptually wrong with this patch, but
>> it makes me think that you hit this condition. It's really a bug in the
>> caller, of course, is a WARN_ON_ONCE() warranted here? And who was the
>> caller?
> 
> How can the caller know that the task is on the queue?

It's similar to double adding a list entry to a list. It's the callers
responsibility to make sure that doesn't happen. The item happens to be
per-task here, but that doesn't really change the mechanics of it.

> There will be a race condition just before the work function
> is called and/or just after it returns that the caller
> can't detect.
> The check needs to be done atomically with the code that
> removes the work item from the list.

We're not polluting task_work with caller specific code like that, as
far as I'm concerned. Fix it in the caller. By the time ->func is run,
the item is off the original list and it can get re-added just fine. If
the event triggers after removing from the list but before ->func is
called, I don't see a problem with that. The caller just wants to ensure
that func is run, which it will be.

If the caller needs stronger guarantees than that, then it should
implement them separately on top of task_work.

-- 
Jens Axboe